Introduce Hypercorn as a test HTTP/2 server#412
Introduce Hypercorn as a test HTTP/2 server#412pgjones wants to merge 2 commits intoencode:masterfrom
Conversation
|
I think it is at a good stage for feedback. |
sethmlarson
left a comment
sethmlarson
left a comment
There was a problem hiding this comment.
Thanks for opening this @pgjones! Here's some preliminary comments.
tests/dispatch/test_http2.py
Outdated
|
|
||
| async with AsyncClient(backend=backend) as client: | ||
| response = await client.get("http://example.org") | ||
| @pytest.mark.skipif( |
There was a problem hiding this comment.
Can the h2_server raise the skip within the fixture?
|
From a POV of managing encode as a whole this one is a mixed bag. I'd personally much rather look at adding HTTP/2 support to uvicorn, than switch to hypercorn as the test server. It's also problematic for us to switch to something that has a different set of Python requirements, (3.7+) rather than being able to keep roughly in line across a suite of projects. I'm way more invested in promoting ecosystem-as-a-whole rather than individual projects, but just wrt. logisitics and management overhead, this change probably comes out as a net negative to me. |
Hi @tomchristie this adds Hypercorn as a test server rather than replacing uvicorn as the test server, as per #290. In its current form it only uses Hypercorn for the HTTP/2 tests. |
florimondmanca
changed the title
florimondmanca
left a comment
florimondmanca
left a comment
There was a problem hiding this comment.
I think this is a great addition.
There's definitely some refactoring required to help make the hypercorn-based HTTP/2 server as self-contained and isolated as possible. For example, move it to conftest.py with a custom H2Server class that exposes the required interface to be able to pass it to serve_in_thread(). I may draft a PR against your fork to demonstrate the idea. :)
|
@florimondmanca thanks. I'm happy to tidy this up - I just got it into a workable state to get initial thoughts. |
This allows the HTTP/2 tests to be directed at an actual HTTP/2 server rather than a mock server. This is beneficial as it is closer to the actual usage by users.
|
Thanks @florimondmanca I've pulled in your changes and rebased against master. |
|
While I appreciate a lot the effort put into getting something (almost!) working for this one, this PR has gone a bit stale because opinions are mixed about eventually integrating it into master. I'll close it, and maybe we can take inspiration from this for a future implementation (maybe when Uvicorn gets HTTP/2?). Thanks @pgjones! ❤️ |
5 participants
This allows the HTTP/2 tests to be directed at an actual HTTP/2 server
rather than a mock server. This is beneficial as it is closer to the
actual usage by users.