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

Multiple requests to the same URI/method return results as recorded #16

Closed
wants to merge 2 commits into from
Closed

Multiple requests to the same URI/method return results as recorded #16

wants to merge 2 commits into from

Conversation

jdoxey
Copy link

@jdoxey jdoxey commented Dec 22, 2013

Hi Dustin,

First of all, thanks for creating VCRURLConnection! We've loved using VCR on ruby projects and were excited to find someone maintaining a objective-c version. We're using it for all of our acceptance tests on our new mobile app.

We noticed that multiple requests to the same URI/method would return the same result. This pull request is a fix that we're using from our forked version. We'd love to get your feedback on this approach and ultimately have this functionality contributed back into your version.

We're under the pump at the moment trying to get a release out, ideally we'd like to clean up and test the logic in recordingForRequestKey:key some more. We thought we'd reach out to you about the general approach first.

Thanks again, we look forward to your response.

@dstnbrkr
Copy link
Owner

Hi @jdoxey,

Thanks and excited to hear you're using it for acceptance testing! I'll take a look at this and get back to you asap. Thanks!

@jdoxey
Copy link
Author

jdoxey commented Jan 3, 2014

Hi @dstnbrkr, I've found a few issues with my commit. It looks like the status isn't being recorded properly, and the order is upside-down. I'm looking into these now.

@jdoxey
Copy link
Author

jdoxey commented Jan 3, 2014

I've added a new commit to this pull request that fixes these issues for us. Turns out my initial solution was never recording hits to multiple URLs, it only worked while replaying JSON files I'd hand crafted.

I had to add a new method [VCR startRecordingDuplicates] so it knows wether a duplicate URL should be recorded or replayed. (The name needs some work.)

The changes are starting to be quite significant. I'd love to get your input on the overall approach.

Cheers,
John

@dstnbrkr
Copy link
Owner

dstnbrkr commented Jan 3, 2014

Excellent - thanks for putting this together. I'm just about to cut a release to add NSURLSession support - I'll take a look right after that.

@dstnbrkr
Copy link
Owner

Apologies for my slow response. I've given this some thought and I believe dynamic responses would be a better solution for a case like http://time.com/utc. If recordings supported variables and there was a way to pass values (or blocks that yield values) to the cassette - then VCR could interpolate those values at playback time.

So in the case of time.com/utc, there could be a block that yields [NSDate date] whose string gets replayed every time time.com/utc is replayed (rather than being limited to n recordings of a pre-set time).

Thanks for taking the time to think this through and identifying the need here - I do believe you are correct that VCRURLConnection needs a way to provide changing/dynamic responses to a single URI.

@jdoxey
Copy link
Author

jdoxey commented Jan 17, 2014

That might be a nice option, but I think it's important to be able to record and play back simply.

+ (void)startRecordingDuplicates {
[VCR start];
recordingDuplicates = YES;
}
Copy link
Owner

Choose a reason for hiding this comment

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

Rather than mixing start and enabling recording of duplicates, how about:

  • (void)setRecordingDuplicates:(BOOL)recordDuplicates;
  • (BOOL)isRecordingDuplicates;

setRecordingDuplicates can be called before or after [VCR start] and have the same effect.

@dstnbrkr
Copy link
Owner

Fair enough, your feature is a good idea - let's make it happen. I added a bunch of comments inline. I added NSURLSession support between your original pull request and now, so unfortunately there's a pretty big rebase needed for this PR. The good news is that the updates include some better infrastructure for tests so you won't need to roll your own NSURLConnectionDelegate. There's now a reusable test delegate. Thanks for contributing this - let me know how I can help.

@dstnbrkr
Copy link
Owner

/cc @kreeger for any further feedback

@kreeger
Copy link
Contributor

kreeger commented Jan 18, 2014

This is cool. I'm gonna try it out now that you've got NSURLSession support, too!

@dstnbrkr
Copy link
Owner

closing this PR as inactive

@dstnbrkr dstnbrkr closed this Jan 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants