-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Introduce test infrastructure for QuicTransport #22844
Conversation
@Hexcles, what do you think about this? @ricea @aboba @vasilvv: Can you take a look at the webtransport/quic part? I tested this with manually running the server,
...and giving additional command line flags to Chrome.
|
@vasilvv: I'd appreciate your review on quic_transport_server.py too. |
async def notify(connection): | ||
_, writer = await connection.create_stream(is_unidirectional=True) | ||
writer.write(b'PASS') | ||
writer.write_eof() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is connection
ever closed? Do we need to close it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not closed, and I don't think it needs to be closed in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both PRs have landed! @yutakahirano could you do a |
Co-Authored-By: Robert Ma <robertma@chromium.org>
Co-Authored-By: Robert Ma <robertma@chromium.org>
25e8cf1
to
25182ba
Compare
Sorry for the late response. This now pass checks. For the certificate issue I filed aiortc/aioquic#89. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with exception of one minor comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great!
@@ -0,0 +1,12 @@ | |||
To generate cert.key and cert.pem: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, once aiortc/aioquic#89 is fixed, we can use web-platform.test.{key,pem}
directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do that.
An implication is that the QUIC server will be relatively self contained, without dependencies on other modules in wpt that are currently Python 2-only.
https://github.com/web-platform-tests/rfcs/blob/master/rfcs/quic.md
does not apply to certificate files, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's OK to use the cert in tools/cert
. We could perhaps keep the certs in tools/quic/certs
as well but only as a fallback.
) | ||
try: | ||
loop.run_forever() | ||
except KeyboardInterrupt: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: is there any cleanup that's required? Like releasing the port?
If so, we'd also need to catch SIGTERM
and SIGINT
(which is different from KeyboardInterrupt
, which I think is only raised after interactive Ctrl-C), and exit gracefully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yutakahirano I took a look and I don't think any cleanup is needed here. There's no subprocess, and the UDP port gets released almost immediately if the process is killed (unlike TCP ports).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we still haven't got a new aioquic
release yet. Nonetheless, I'm going to merge this PR for now so that I can build some final pieces of intergration.
Sorry for the late response, and thank you for merging this. I'll address the remaining comments in followup PRs. |
Address comments provided in #22844.
* [QuicTransport] Address comments in pull/22844 Address comments provided in #22844.
…/22844, a=testonly Automatic update from web-platform-tests [QuicTransport] Address comments in pull/22844 (#23705) * [QuicTransport] Address comments in pull/22844 Address comments provided in web-platform-tests/wpt#22844. -- wpt-commits: 6e5e590ef6d34a2cdbc04bbc493d34e482db1d77 wpt-pr: 23705
…/22844, a=testonly Automatic update from web-platform-tests [QuicTransport] Address comments in pull/22844 (#23705) * [QuicTransport] Address comments in pull/22844 Address comments provided in web-platform-tests/wpt#22844. -- wpt-commits: 6e5e590ef6d34a2cdbc04bbc493d34e482db1d77 wpt-pr: 23705
This change introduces test infrastructure for
QuicTransport.
See also: https://github.com/web-platform-tests/rfcs/blob/master/rfcs/quic.md
tools/quic contains the test server and files needed by the server such as certificate files.
tools/quic/quic_transport_server.py is based on
https://github.com/aiortc/aioquic/blob/master/examples/http3_server.py.
webtransport/quic contains a test example. It also contains a sample custom handler.
This change doesn't contain a means to run the QuicTransport server automatically.
Tracking issue: #19114