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

Support multiple requests for the same url #37

Merged
merged 2 commits into from
Jun 30, 2017

Conversation

haitaoli
Copy link
Contributor

@haitaoli haitaoli commented Jun 24, 2017

Summary

This PR addresses issue #33.

  • Recordings are now saved in an array for each url+method key.
  • After a recording is replayed, it's removed from array. The last recording in the array is not removed, to maintain compatibility with current behavior.

If VCR is in both recording and replaying mode, the replayed recording is added back to the queue. This behavior doesn't make a difference when there is only one recording per url. But when there are multiple responses, this creates a "loop". For example, responses A and B are in the array, the replay sequence becomes A, B, A, B etc. If VCR is only in replay mode, then the replayed sequence is A, B, B, B...

@dstnbrkr
Copy link
Owner

Thanks @haitaoli, this looks interesting! I will review and get back to you.

@dstnbrkr
Copy link
Owner

The replaying:(BOOL) parameter makes the logic a little complex. What if instead there was no replaying parameter? Looks like that would be possible if recordingForRequestKey: used an index instead of mutating the recordings array. For instance, recordingForRequestKey could just increment an index and use index++ % [recordings count] to cycle through each recording for each request. Might make the test cases simpler too since you won't need to account for different behaviors depending on replaying/recording.

__block VCRRecording *recording = [self.responseDictionary objectForKey:key];
- (VCRRecording *)recordingForRequestKey:(VCRRequestKey *)key replaying:(BOOL)replaying {
__block VCRRecording *recording;
__block NSMutableArray *recordings = [self.responseDictionary objectForKey:key];
Copy link
Owner

Choose a reason for hiding this comment

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

I don't see recordings getting mutated in a block, so this won't need the __block declaration.

@haitaoli
Copy link
Contributor Author

haitaoli commented Jun 29, 2017

recordingForRequest is called at two places, one to check if there is recording available (canInit), the other to get recording, so it cannot always increment the index. I thought about giving them different and more meaningful names such as hasRecording and replay, but that would require a lot of changes to rename it everywhere. If you don't mind a much bigger PR I can do that.

Cycling through all recordings might not be the ideal behavior. If replay runs out of recordings then something is wrong. In that case the latest response probably makes more sense.

@dstnbrkr
Copy link
Owner

Got it - given that it's used for both recording and replaying, what you have makes sense! So then the only outstanding item is the extra __block declaration and this is good to go. Thanks again for this contribution!

@haitaoli
Copy link
Contributor Author

Removed uncessary __block.

@dstnbrkr dstnbrkr merged commit 0cc83fa into dstnbrkr:master Jun 30, 2017
@haitaoli
Copy link
Contributor Author

haitaoli commented Jul 5, 2017

@dstnbrkr thanks for merging it. Do you have plan to release a new version?

@dstnbrkr
Copy link
Owner

dstnbrkr commented Jul 7, 2017

@haitaoli all set! (just shipped 0.2.3)

@haitaoli
Copy link
Contributor Author

haitaoli commented Jul 7, 2017

Great! Thank you.

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.

2 participants