-
Notifications
You must be signed in to change notification settings - Fork 602
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
Adds support for HTTPBody during testing when using NSURLSession #166
Adds support for HTTPBody during testing when using NSURLSession #166
Conversation
* by the time the request arrives to OHHTTPStubs, the HTTPBody of the NSURLRequest | ||
* can't be accessed anymore. | ||
* | ||
* You can use this method to retrieve the HTTPBody for testing and |
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.
for testing and
...?
Cool, looks interesting ! Don't forget to add an entry in the CHANGELOG to credit yourself. I'm going on vacation for two weeks so I won't have time to check that PR until I return. |
method_exchangeImplementations(originalMethod, swizzledMethod); | ||
} | ||
|
||
NSLog(@"Enabled method swizzeling setHTTPBody: on NSMutableURLRequest"); |
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.
You should probably remove the NSLogs now that you've debugged its proper integration?
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.
The idea was to make sure this never ends up in any code unnoticed. However, swizzling is used in other parts of OHHTTPStubs
, too, and the framework should generally only be used for testing and the likes, so I agree, this can probably be removed.
Thanks for getting back so quickly! Enjoy your vacation, we will improve this PR in the meantime. |
5c14c10
to
46797a3
Compare
If merged, you'd probably want to...
|
if (HTTPBody) { | ||
[NSURLProtocol setProperty:HTTPBody forKey:OHHTTPStubs_HTTPBodyKey inRequest:self]; | ||
} else { | ||
// unfortunately resetting does not work properly as the NSURLSession also uses this to reset the property |
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.
If setting it to nil
doesn't work, wouldn't it at least be an acceptable workaround to set it ton an empty NSData
instead?
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.
The reason we can't do that is that the session calls this method with nil which lead to the initial problem. If we touch the protocol property here we lose the benefit of this PR. The usual story is this:
- App sets up mutable request
- App sets body
- URL session for whatever reason resets it
- OHHTTP handles it
If we reset the property in step 3 again, we don't have access in step 4.
I believe that in production code it rarely happens that you set the body and then later need to reset it (at least missing support for this is the lesser bug, compared to not supporting the body IMHO). We can't really differentiate between the callers here unfortunately (to my runtime knowledge).
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.
Oh I see, right.
Didn't understand your code comment at first (wrongly thought you meant "setting it to nil doesn't work"), but makes total sense now 👍
Finally had time to check that PR 🎉 (sorry it took so long) Please address the above comments, then rebase the branch on top of master — because as master has evolved since your PR, your branch is now in conflict (probably only because of the CHANGELOG entry, so not a complex conflict to fix 😉) Otherwise I'm 👍 |
FYI, in general I use a special So when you'll be rebasing, you'll see that |
Ping @felixLam ?
|
Sorry about the delay, we had a busy week and @felixLam was on holidays. We'll tackle it next week! |
np, thx |
e2a96f2
to
4874d3a
Compare
@AliSoftware I made the requested changes and rebased on master |
Thx 👍 (I don't know what's wrong with travis not reporting the test results on this PR suddenly… will investigate and merge once ok) |
@AliSoftware there was a "minor" outage on GitHub today. |
@felixLam Ah good point that was probably it. Tested locally and force-merged onto master, (let wait for travis to check master now just to be sure… 😉) |
@felixLam In an effort to try and follow Moya's Contributor Guidelines, and to thank your for your recent contribution to |
@AliSoftware Thank you very much. Credit where credit is due: @shagedorn did most of the heavy lifting here with the method swizzling. |
Right, added @shagedorn as a contributor as well just now 😉 |
How should we #import the ability to utilize this new code? I can't seem to build it with just Note, I'm not using cocoapods to install. I'm dragging the xcodeproj into a frameworks group in my own xcode project. that's how I've always used OHHTTPStubs. Thanks in advance |
@mikelupo This PR should not require any changes on your end as far as I remember and should just magically work. |
Thanks for the quick reply. Now that I've resolved that, it still won't build unless I import:
In my test, I'm using the new method call like this:
Mike -----Original Message----- @mikelupo The should not require any changes in your end as far as I remember and should just magically work. |
@felixLam @shagedorn while doing the 5.1.0 release, I just realized that maybe I merged that PR too quickly. I mean, it should work, but:
That doesn't seem much work to do to make it right, so do you think you'd be able to throw a quick PR to add that soon, so I can include it properly in the upcoming 5.1.0 release? Thx! |
Absolutely. Will look at this tomorrow.
|
Cool Thx 👍 |
…Unit Test to be added for #166 soon)
ping @felixLam ? |
@mikelupo the I'm working on the remaining issues (rename & tests) right now and will submit a PR later today. |
That's sadly a legacy issue, because when I created Now that That's because of all these reasons that the umbrella header being But anyway, using the modular import Looking fwd to your PR @shagedorn so that we can release the next version 👍 |
@AliSoftware Sorry for not following through with my promise on Friday. We had an unscheduled workload due to a rejection ;) |
np I know those situations 😉 |
This addresses the issue described in https://github.com/AliSoftware/OHHTTPStubs/wiki/Testing-for-the-request-body-in-your-stubs and #52
It also avoids unnecessary changes to the code under test as the setter and getter for
HTTPBody
can still be used without change in behavior (as far as one can tell).The new getter
-[NSURLRequest OHHTTPStubs_HTTPBody]
can be used in your[OHHTTPStubs stubRequestsPassingTest:withStubResponse:]
to test theHTTPBody
.One known issue is that if you set the HTTPBody and later reset it to nil (for whatever reason),
OHHTTPStubs_HTTPBody
will still return the last non-nil value.Thanks to @shagedorn for the help on method swizzling.