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

Fix HTTPX support #784

Merged
merged 5 commits into from
Dec 8, 2023
Merged

Fix HTTPX support #784

merged 5 commits into from
Dec 8, 2023

Conversation

parkerhancock
Copy link
Contributor

Guys, VCRpy's support for httpx is hella broken. Like for serious. So I fixed it. Summary of the changes:

Changed patch target from .send to ._send_single_request

This change moves the patch target to client._send_single_request, which is the lowest, and most atomic request for data over the wire. By moving the patch lower down the stack, it fixes #684 (support for httpx.Auth), #619 (errors handling redirects), #600 (stubs trying to access an invalid property), #599 (encoding types), #597 (async httpx streaming doesn't work).

Removed attempt to decode raw body content

For some reason, in the record function, the serializer was trying to decode binary body content (i.e. content.decode("utf-8", "ignore"). This one line meant that any raw binary data that happened not to resolve to a valid Unicode code point was deleted, which meant subsequent requests for binary content would be corrupted. This fixes #656 (Binary upload data unsupported), and #597 (async httpx streaming).

Simplified Stubs & Test Suite

The stubs and tests have been dramatically pared down and simplified, because we've moved the patch target so much lower. We no longer need to test for redirects or auth flow. The only thing we need to hold httpx's hand on is cookies, which is still tested. Also added a test case for streaming data (#597).

Replaced mockbin with httpbin

Mockbin went commercial, which meant the whole test suite was broken. I moved all the tests to now target httpbin.org.

Other Benefits

I've also:

  • Added a separate fixture for testing streaming, since httpx uses client.stream rather than client.request(. . . , stream=True)
  • Made corresponding changes so that both httpx.AsyncClient and httpx.Client both work.

PLEASE PLEASE PLEASE MERGE THIS REQUEST AND SAVE US POOR HTTPX USERS!

@jairhenrique
Copy link
Collaborator

@parkerhancock can you help us moving mockbin tests to httpbin?

@parkerhancock
Copy link
Contributor Author

@parkerhancock can you help us moving mockbin tests to httpbin?

Happy to! Maybe that will be a good next PR once this one is complete.

@parkerhancock
Copy link
Contributor Author

@parkerhancock can you help us moving mockbin tests to httpbin?

Test suite was failing on this PR, so I fixed it for you. All tests on all backends now pass on py311, supported by the pytest-httpbin plugin (local server, no calls to external servers).

mockbin has been summarily executed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These changes were necessary because trying to spawn a new process lead to an exception caused by trying to pickle a thread lock. So the same proxy process now runs in a separate thread, rather than a separate process.

@codecov-commenter
Copy link

codecov-commenter commented Dec 8, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (defad28) 89.77% compared to head (5e6c20c) 90.10%.

Files Patch % Lines
vcr/serializers/compat.py 0.00% 0 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #784      +/-   ##
==========================================
+ Coverage   89.77%   90.10%   +0.33%     
==========================================
  Files          27       27              
  Lines        1819     1809      -10     
  Branches      284      335      +51     
==========================================
- Hits         1633     1630       -3     
+ Misses        141      134       -7     
  Partials       45       45              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jairhenrique
Copy link
Collaborator

@parkerhancock LGTM

@parkerhancock
Copy link
Contributor Author

@parkerhancock LGTM

🙏 YOU ARE MY HERO! LET'S GO 🚀 🚀 🚀

Any idea when we can get this pushed out to PyPI in a release? It is a blocker to having a decent release over at patent_client

(And thanks again for your prompt review!)

@ionelmc
Copy link

ionelmc commented Dec 10, 2023

I've tried this, it works. Just a small note, I found out that I needed to recreate some of my cassettes for tests that dealt with binary content as the previous implementation incorrectly saved responses as text instead of bytes.

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.

httpx auth flow is skipped by replay? [BUG] HTTPX Binary upload data unsuported
4 participants