Conversation
|
💚 CLA has been signed |
|
I have now signed the CA! |
margaretjgu
left a comment
margaretjgu
left a comment
There was a problem hiding this comment.
Thanks for putting this together @sgraaf ! This is a great addition!
Just noticed a few small things that should be easy to fix :)
On test coverage, it'd be great to also add HttpxHttpNode to the parametrized lists in test_logging.py and test_httpbin.py (the sync @pytest.mark.parametrize("node_class", ["urllib3", "requests"]) tests) and the test_unknown_parameter case in test_base.py. That way it gets the same integration coverage as the other sync nodes.
Thanks again for the contribution!
| ssl_context.load_cert_chain(config.client_cert) | ||
|
|
||
| self.client = httpx.Client( | ||
| base_url=f"{config.scheme}://{config.host}:{config.port}", |
There was a problem hiding this comment.
The async node includes config.path_prefix in the base url:
This means
path_prefix won't work on the sync node. There's also no test_path_prefix for the sync node (there is one for async in TestHttpxAsyncNodeCreation), so this bug goes undetected by the test suite.
| body=body, | ||
| exception=err, | ||
| ) | ||
| raise err from None |
There was a problem hiding this comment.
Should be similar to async implementation raise err from e for easier debugging. We should match that within sync
|
|
||
| meta = ApiResponseMeta( | ||
| resp.status_code, | ||
| resp.http_version, |
There was a problem hiding this comment.
The async node strips the HTTP prefix:
Similar to the above, maybe nitpick but I think we should avoid divergence in behaviour between sync/async
sgraaf
force-pushed
the
add-sync-httpx-node
branch
from
9171fff to
797cc15
Compare
|
Thank you for the review & feedback! I've just updated the PR based on your feedback and all should be well now! :) Please let me know if there are any remaining issues. |
3 participants
This PR adds a synchronous node class that uses a
httpx'Client. This would enable developers of downstream packages and applications to use either / both sync and async node classes using onlyhttpx, instead of multiple Python HTTP packages.The implementation is almost an exact copy of
HttpxAsyncHttpNode, stripped of allasync/await/a*().