-
Notifications
You must be signed in to change notification settings - Fork 601
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enabled creating multiple instances of OHHTTPStubs #243
base: master
Are you sure you want to change the base?
Conversation
… defaultSessionConfiguration] and +[NSURLSessionConfiguration ephemeralSessionConfiguration]
Changing |
I don't see it being used in public interface anywhere. But I agree, this does not make a big difference, I'll revert it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @nickolas-pohilets, thanks a lot for your contribution! 👍
This contains some significant changes that will require us some time to review thoroughly.
I am not that familiar with the inner workings of the library and I think we will need the input of @AliSoftware here.
But after a quick look at it, I did not see a test taking advantage of the major feature you are introducing, meaning stubbing with multiple instances of OHHTTPStubs
. The only place I see an instance used is in test_NSURLSessionDefaultConfig_customInstance
, but you do not add stubs to it.
Maybe you could add a test exposing that feature? 😉
Next, about the documentation, I think we need to update some of it if we introduce this because the old documentation would be misleading.
Take +[OHHTTPStubs removeAllStubs]
for example. In your implementation, it has a whole other meaning because it does not "remove all stubs" but only "remove all the stubs of the shared instance". We need to warn the user about that.
By the way, you need to add a documentation block for the new object level API on OHHTTPStubs (e.g. - [OHHTTPStubsremoveStub:]
).
Thanks again for your work!
if ([NSURLSessionConfiguration class] && [NSURLSession class]) | ||
{ | ||
BOOL wasEnabled = [OHHTTPStubs isEnabled]; | ||
XCTAssert(wasEnabled, @"Stubs are expected to be on by default"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use XCTAssertTrue
instead of XCTAssert
.
BOOL wasEnabled = [OHHTTPStubs isEnabled]; | ||
XCTAssert(wasEnabled, @"Stubs are expected to be on by default"); | ||
[OHHTTPStubs setEnabled:NO]; | ||
XCTAssert(![OHHTTPStubs isEnabled], @"isEnabled should report NO after turning off"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use XCTAssertFalse
instead of XCTAssert
.
I'll try to take a look at that in a few weeks after all my conference talks are done 😉 But just wondering how that would work with the auto-installation feature of How is that gonna affect the usability of |
Checklist
Description
Enabled creating multiple instances of OHHTTPStubs to be able to have
NSURLSession
's with different stub configurations.This is implemented by using proxy objects that represent class of OHHTTPProtocol and allocated but not initialized instance of OHHTTPProtocol. Those proxy objects inject pointer to instance of OHHTTPStubs into instance of OHHTTPProtocol.
Motivation and Context
This should enable writing an integrational test that tests that all components in the subsystem use specified session configuration and don't create their own or use NSURLConnection.
Extra
+[OHHTTPStubs isEnabled]
not being respected forNSURLSessionConfiguration
.+[OHHTTPStubs sharedInstance]
into '+[OHHTTPStubs defaultInstance]` to make it more clear that other instances can be created as well.