Skip to content
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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

nickolas-pohilets
Copy link

Checklist

  • [+] I've checked that all new and existing tests pass
  • [+] I've updated the documentation if necessary
  • [+] I've added an entry in the CHANGELOG to credit myself

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

  • Moved implementation detail classes into separate files for better code readability.
  • Fixed +[OHHTTPStubs isEnabled] not being respected for NSURLSessionConfiguration.
  • Renamed +[OHHTTPStubs sharedInstance] into '+[OHHTTPStubs defaultInstance]` to make it more clear that other instances can be created as well.

@Ashton-W
Copy link
Collaborator

Changing sharedInstance is a source breaking change. It's a little needless IMO

@nickolas-pohilets
Copy link
Author

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.

Copy link
Collaborator

@Liquidsoul Liquidsoul left a 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");
Copy link
Collaborator

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");
Copy link
Collaborator

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.

@AliSoftware
Copy link
Owner

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 OHHTTPStubs (the swizzling of NSURLSessionConfiguration methods to auto-inject the OHHTTPStubs sharedInstance into them so that people have nothing to do — behaviour which we want to keep), does that mean that we need a way to remove the sharedInstance and replace it with a custom instance from an NSURLSessionConfiguration for people wanting to create them manually?

How is that gonna affect the usability of OHHTTPStubs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants