aiohttp: fix multiple requests being replayed per request and add support for request_info on mocked responses
#495
Conversation
|
What do you think @neozenith? How can I help make this easy for you and the other maintainers to get this over the finish line? Anyone else I should tag? :) |
Codecov Report
@@ Coverage Diff @@
## master #495 +/- ##
========================================
Coverage ? 89.3%
========================================
Files ? 24
Lines ? 1562
Branches ? 217
========================================
Hits ? 1395
Misses ? 137
Partials ? 30
Continue to review full report at Codecov.
|
|
Hey @nickdirienzo thank you so much for diving into this issue! It'd make me very happy to get this over the line. If it is a breaking change though, we should bump it to v3.x.x That said we are due to cull python2 support with PR #443 as well. I might add this to the I'll have to circle back to reviewing this later this week as I have limited bandwidth. |
|
)You're welcome! Bumping to 3.x.x sounds right. I'm mostly worried about users that may have been following redirects in their tests so they would have to re-record those tests. However, I'm not entirely sure those tests are correct with the current implementation, so maybe I'm over-worrying about this. What's the timeline for the v3 release? (A bit of an aside: I can try to also find time to hack on #463. However, I'm not sure if that's a must-have for the release or not.) And no worries. Take your time on reviewing this one. Appreciate any and all feedback! |
|
I'll aim to get on top of the review in the next week. We've been holding off the cull of python2 but that clock is ticking ... As soon as a PR is ready and good to go I'd rather release it than sit on our hands to collect a group of PRs together since there are so few PRs it makes no sense to wait. Also means we will increment a few major changes pretty quickly but it all goes into the changelog. Speaking of which, you should add a line to |
|
Thanks for the context! Yep, 2020 is around the corner 😅 That sounds good to me. I'll update the changelog. |
|
Hey @nickdirienzo, sorry... I will get to this early this week. Got busy. |
|
Hey Josh! All good. I totally understand. Not in a rush here. I wasn't able
to push up that change until this weekend anyway.
…On Sat, Dec 7, 2019 at 3:59 PM Josh Peak ***@***.***> wrote:
Hey @nickdirienzo <https://github.com/nickdirienzo>, sorry... I will get
to this early this week. Got busy.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#495?email_source=notifications&email_token=AAFQU324ZXAKWKN7PYCFVCDQXQ2GHA5CNFSM4JTOC7YKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGGSFUI#issuecomment-562897617>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFQU36GTD5SIU3ADRONILDQXQ2GHANCNFSM4JTOC7YA>
.
|
|
Oh sweet — thanks @neozenith! Let me know when the 3.x release is and I can retest this before we release, if that'd be helpful. |
|
I'm preparing a release now. |
This PR resolves #481 and #475, while also improving #456.
It was discovered that, for
aiohttpspecifically, when there were multiple requests recorded that looked the same, they would all be played. Looking into this further, it was because the URL we recording to was the original request's URL instead of the implicit request's that followed the redirects.This led to the change you see here where we're recording the individual request-response chains (similar to that of other HTTP clients). We also change the explicit
while cassette.can_play_response_for(vcr_request)to one that checks the response status codes and only performs a similar check if the code is a 3xx one.While implementing that, I wanted to be extra sure that we had similar looking responses, so I started checking the
request_infoobject. Originally, each of the mock responses do not have those objects available (I'm not sure if this is intended or not), so this PR also adds support for that.test_redirectwas amended to include a specific check that we're returning the samerequest_infobetween the real one and the recorded one.Speaking of tests, thanks to @vEpiphyte for writing
test_double_requests, which made this issue repeatable and give me confidence that this works.There are two concerns I have around this PR:
aiohttpclients where redirects are performed because how we're using the cassette is fundamentally different. It feels weird to have a major bump for this, but if we're following SemVer, that may be right --- how do you all do versioning? Happy to include the version bump in this PR if that's preferred.cassette.find_requests_with_most_matches(https://github.com/kevin1024/vcrpy/compare/master...nickdirienzo:issue-481?expand=1#diff-4636a4db24aa9f9206350b6a8e171ae1R104). While the request should exist, it may not and then the assumed look up will fail hard. We do need to get the recorded request in this block, otherwise we can't have the correctrequest_info. Do y'all have ideas on how to do this more safely?Some references that I needed along the way:
aiohttpfollows redirects by default: https://docs.aiohttp.org/en/stable/client_advanced.html#redirection-historyRequestInfo: https://docs.aiohttp.org/en/stable/client_reference.html#aiohttp.RequestInfo