Allow testing HTTP/1.1 and HTTP/2 in the same test #3310
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pquentin
added
the
Skip Changelog
illia-v
previously approved these changes
Jan 24, 2024
test/with_dummyserver/test_http2.py
Outdated
There was a problem hiding this comment.
Should we rename this file to test_hypercorn_server.py now since the test uses both HTTP/1.1 and HTTP/2?
There was a problem hiding this comment.
Or maybe it's OK to drop that file altogether now?
There was a problem hiding this comment.
Good point, I'm fine with dropping it since other tests start using HTTP/2
illia-v
approved these changes
Jan 24, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #3194
We're using a parametrized fixture that when used, will run a given test twice, with HTTP/1.1 and HTTP/2. When used with HTTP/2, it injects HTTP/2 support (which currently forces the usage of HTTP/2). The value of the fixture is "h11" or "h2", which helps to check the header return by Hypercorn.
Things I can do differently:
pytest.mark.parametrizeas done by Hip. It's much more explicit (case in point, I had no idea thatloopback_hostwas a parametrized fixture) but also a lot of work - AnyIO needs a lot of pytest code to make that work transparently.http_version, I could use a different name to clarify that this tests HTTP/1.1 and HTTP/2.2, buthttp_versionis natural to use inside tests.This pull request was sponsored by Elastic, my employer.