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

Add support for NSURLSession #34

Closed
wants to merge 7 commits into from
Closed

Conversation

ndonald2
Copy link
Contributor

@ndonald2 ndonald2 commented Oct 4, 2013

Adds support for stubbing requests made using NSURLSession.
Updates AFNetworking in the UnitTests to v2.0
Unit test for AFHTTPSessionManager

@AliSoftware The one caveat is that the entire static lib must be built to target 7.0+ for my changes to be compiled. Alternatively it can be compiled directly from source in a parent project that targets 7.0+.

Please let me know if you'd prefer if that were handled differently.

@AliSoftware
Copy link
Owner

Thx for the pull request. I will study it this weekend, probably as soon as tomorrow.

Regarding the mentioned caveats

Don't confuse __IPHONE_OS_VERSION_MAX_ALLOWED (the version of the SDK you compile with) and __IPHONE_OS_VERSION_MIN_REQUIRED (the deployment target of the project / minimum iOS version required for the built app).

In my opinion, and according to Apple's "SDK Compatibility Guide", OHHTTPStubs should:

  • build and include the code related to NSURLSession if __IPHONE_OS_VERSION_MAX_ALLOWED >= 70000 (that is, when building with SDK 7, where the NSURLSession class is known in the headers and libs), whatever the Deployment Target is (even iOS5 or iOS6)
  • conditionally use this code at runtime by encapsulating it into tests like if ([NSURLSession class]) and respondsToSelector: calls. This way the code related to NSURLSession is included as soon as you build with SDK7, but is used at runtime only if the end user runs iOS7+ (but not when running on an iPhone/iPad still using iOS6).

Read the SDK Compatibility Guide for more info if needed

Concerning the comment in your last commit message

xcshareddata is specially designed to contain what should be shared on a repository, contrary to xcuserdata which is to be excluded. In that case, it seems reasonable to me to keep it in the repo. Disabling tests directly in the scheme is designed for tests that should be disabled because of their nature (they are not to be automated, or they are work in progress, or they are not relevant anymore, etc). If you want to execute only one test, for exemple during the development phase when you test your NSURLSession unit test, Xcode5 now allows to execute tests one by one, so you shouldn't need to disable the tests in the scheme anyway. So IMHO we should keep it in the repo.

} withStubResponse:^OHHTTPStubsResponse *(NSURLRequest *request) {
return [[OHHTTPStubsResponse responseWithData:expectedResponse statusCode:200 headers:@{@"Content-Type" : @"application/json"}]
requestTime:kRequestTime responseTime:kResponseTime];
}];
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for information, for the sake of consistency you could also have used the commodity method responseWithJSONObject:statusCode:headers: here instead (defined in OHHTTPStubsResponse+JSON.h) which does the NSJSONSerialization and adds the Content-Type header for you:

NSDictionary *expectedResponseDict = @{@"Success" : @"Yes"};
[OHHTTPStubs stubRequestsPassingTest:^BOOL(NSURLRequest *request) {
    return YES;
} withStubResponse:^OHHTTPStubsResponse *(NSURLRequest *request) {
    return [[OHHTTPStubsResponse responseWithJSONObject:expectedResponseDict statusCode:200 headers:nil]
            requestTime:kRequestTime responseTime:kResponseTime];
}];

It doesn't change anything but is more readable IMHO. I don't know if you were aware of that commodity method ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AliSoftware thanks for the tip. That method actually had a bug where the headers were not set correctly if you pass nil, but I fixed it in my pull request branch.

@ndonald2
Copy link
Contributor Author

ndonald2 commented Oct 4, 2013

@AliSoftware Thanks for taking a look, I will try to fix up these few things and push here.

@ndonald2
Copy link
Contributor Author

ndonald2 commented Oct 4, 2013

Also I'm not sure why the Travis build check is failing - does it not support iOS7 yet?

@AliSoftware
Copy link
Owner

Indeed Travis build fails because it hasn't upgraded to Xcode5 yet (see Travis-CI issue here).

But actually that will help us verify that the code is still able to be build with Xcode 4.5.2 (version used by Travis). Currently the Travis Build is failing because Xcode4/SDK6 doesn't know about NSURLSessionConfiguration class and its protocolClasses method, but as explained in our discussion we should use #if __IPHONE_OS_VERSION_MAX_ALLOWED conditional to make it buildable using Xcode4 anyway and make it pass Travis build even while they still use Xcode4 ;)

@ndonald2
Copy link
Contributor Author

ndonald2 commented Oct 5, 2013

@AliSoftware Made the changes you pointed out, ready for another review. Thanks!

[OHHTTPStubs removeAllStubs];
}

// Normal stubbing should work fine for the shared session
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't understand why, do you have an explanation why this would works out of the box?

Does this sharedSession object implicitly use the protocols registered with [NSURLProtocol registerClass:], contrary to when you use [NSURLSession sessionWithConfiguration:[NSURLSessionConfiguration defaultSessionConfiguration]]?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the record it seems to be the case — [NSURLSession sharedSession] using classes registered via [NSURLProtocol registerClass:] and not at all the classes present in its .configuration.protocolClasses.
Too bad there is nothing at all about it in the Apple documentation 😒

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's because [NSURLSession sharedSession] is the default session for the app, which is always created and initialized implicitly on application launch:

The shared session uses the currently set global NSURLCache, NSHTTPCookieStorage, and NSURLCredentialStorage objects and is based on the default configuration.

Therefore any "global" Foundation networking methods, including [NSURLProtocol registerClass:], will affect the shared session.

Also since it is impossible to configure sharedSession with an instance of NSURLSessionConfiguration, there would otherwise be no way to register URLProtocol handler classes, since you cannot modify the configuration after the session is created:

Changing mutable values within the configuration object has no effect on the current session, but you can create a new session with the modified configuration object.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep it totally make sense, just too bad it's not documented in the [NSURLSession sharedSession] documentation.

So in summary, you can change the behavior and NSURLProtocol classes between the various calls on [NSURLSession sharedSession] and the requests WILL be affected by those changes:

[[NSURLSession sharedSession] dataTaskWithURL:… completionHandler:…]; // 1
[NSURLProtocol registerClass:…]; // This NSURLProtocol class will be taken into account for the next dataTask below
[[NSURLSession sharedSession] dataTaskWithURL:… completionHandler:…]; // 2
[NSURLProtocol unregisterClass:…]; // The NSURLProtocol class will not be taken into account anomore for the next dataTask below
[[NSURLSession sharedSession] dataTaskWithURL:… completionHandler:…]; // 3 same as 1

That's handy and useful for sure, and an easy way to migrate fro NSURLConnection to NSURLSession, but should really be documented as this behavior for sharedSession is the opposite of the behavior described for other NSURLSession created using an NSURLSessionConfiguration whose behavior can't be changed once created!


So Apple should probably document this specific behavior of [NSURLSession sharedSession] better, for example like this instead [suggestion] :

The shared session is based on the default configuration but uses the currently set global NSURLCache, NSHTTPCookieStorage, and NSURLCredentialStorage objects and the currently registered NSURLProtocol classes. Contrary to NSURLSession based on a NSURLSessionConfiguration, requests sent using the sharedSession uses settings for the NSURLCache, NSHTTPCookieStorage, NSURLCredentialStorage and NSURLProtocol classes that are interpreted at the time the requests are made (and not at the time the session is created)

@AliSoftware
Copy link
Owner

Merged by commit ede2aa4 😃

Let me know if you are OK with my modifications.

@AliSoftware AliSoftware closed this Oct 6, 2013
@AliSoftware
Copy link
Owner

Note that the Travis build still fails, but this is due to AFNetworking 2.0 code.
AFN2 uses __IPHONE_OS_VERSION_MIN_REQUIRED everywhere, where it should use __IPHONE_OS_VERSION_MAX_ALLOWED, leading to build failure because Travis-CI is still running Xcode 4 (and SDK 6.0) and not yet running Xcode 5 / SDK 7.0.

You're welcome to vote for a fix in the AFNetworking repository, there are plenty of issues and people angry about it too and who want that changed so it works and build properly… (see 1410/1412/1436/1437)

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.

3 participants