Conversation
There was a problem hiding this comment.
Pull request overview
Adds initial command-line support for running WebTransport interoperability tests alongside existing QUIC tests, including new protocol docs, test cases, and runner plumbing to select protocol-specific implementations and request semantics.
Changes:
- Introduces WebTransport protocol documentation and a new WebTransport test suite (handshake + transfer variants).
- Refactors test framework to support separate client/server www & downloads mounts and per-side request lists.
- Adds protocol selection (
--protocol) and splits implementation registries into QUIC vs WebTransport JSON files.
Reviewed changes
Copilot reviewed 13 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| webtransport.md | Adds WebTransport protocol + testcase specification for endpoints. |
| testcases_webtransport.py | Implements WebTransport handshake/transfer testcases and verification logic. |
| testcases.py | Refactors QUIC testcases to use new shared testcase.TestCase base and “raw paths” abstraction. |
| testcase.py | Introduces shared base TestCase with separate client/server dirs, file generation, and trace helpers. |
| run.py | Adds --protocol selection and wires protocol-specific implementations + testcase lists. |
| quic.md | Adds QUIC-specific documentation moved out of README. |
| pull.py | Adds --protocol selection for pulling QUIC vs WebTransport implementation images. |
| interop.py | Updates runner to pass separate mounts/REQUESTS/protocols for client and server containers. |
| implementations_webtransport.json | Adds WebTransport implementation registry. |
| implementations.py | Replaces global IMPLEMENTATIONS with loader functions for per-protocol registries. |
| empty.env | Adds protocol env defaults for compose. |
| docker-compose.yml | Switches to separate client/server www/download mounts and per-side REQUESTS/PROTOCOLS mapping. |
| README.md | Updates docs for QUIC/WebTransport split and new CLI usage. |
| .gitignore | Stops ignoring protocol-specific implementation registries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for name, value in IMPLEMENTATIONS.items() | ||
| if value["role"] == Role.BOTH or value["role"] == Role.SERVER | ||
| ] | ||
| from testcases_quic import MEASUREMENTS, TESTCASES_QUIC |
There was a problem hiding this comment.
This imports testcases_quic, but in this PR diff the QUIC testcases live in testcases.py (and no testcases_quic.py file is shown). This will raise ModuleNotFoundError at runtime unless testcases_quic.py is added/renamed accordingly. Align the module name (either add/rename the file or adjust imports back to the actual module).
| from testcases_quic import MEASUREMENTS, TESTCASES_QUIC | |
| from testcases import MEASUREMENTS, TESTCASES_QUIC |
bf58d08 to
ca6e374
Compare
91ff03e to
c5b1b63
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 15 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
run.py
Outdated
| "--test", | ||
| help="test cases (comma-separatated). Valid test cases are: " | ||
| + ", ".join([x.name() for x in TESTCASES + MEASUREMENTS]), | ||
| + ", ".join([x.name() for x in TESTCASES_QUIC + MEASUREMENTS]), |
There was a problem hiding this comment.
The --test help text always lists QUIC testcases (TESTCASES_QUIC + MEASUREMENTS), even when --protocol webtransport is selected. This makes the CLI misleading. Consider listing protocol-agnostic test names (union of both sets), or parsing --protocol first and then constructing the parser/help accordingly.
| + ", ".join([x.name() for x in TESTCASES_QUIC + MEASUREMENTS]), | |
| + ", ".join( | |
| sorted( | |
| { | |
| x.name() | |
| for x in TESTCASES_QUIC + MEASUREMENTS + TESTCASES_WEBTRANSPORT | |
| } | |
| ) | |
| ), |
| if arg is None: | ||
| return TESTCASES, MEASUREMENTS | ||
| return testcases, MEASUREMENTS | ||
| elif arg == "onlyTests": | ||
| return TESTCASES, [] | ||
| return testcases, [] | ||
| elif arg == "onlyMeasurements": |
There was a problem hiding this comment.
When --protocol webtransport is selected and -t/--test is not provided, this returns MEASUREMENTS from the QUIC suite by default. That will attempt to run QUIC measurements against WebTransport implementations, which is very likely incorrect/noisy. Consider returning an empty measurement list for WebTransport (or introduce MEASUREMENTS_WEBTRANSPORT).
So far, the server had only /www and the client had only /downloads.
c5b1b63 to
cb4caec
Compare
1f7da71 to
b47bd32
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 15 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return [], MEASUREMENTS | ||
| return [], measurements | ||
| elif not arg: | ||
| return [] |
There was a problem hiding this comment.
get_tests_and_measurements() sometimes returns a bare list (return []) when arg is an empty string, but callers treat the return value as a 2-tuple (t[0], t[1]). This will raise at runtime. Return a (tests, measurements) tuple in all branches (e.g., ([], []) here).
| return [] | |
| return [], [] |
| protocol = get_args().protocol | ||
| if protocol == "quic": | ||
| impls = get_quic_implementations() | ||
| elif protocol == "webtransport": | ||
| impls = get_webtransport_implementations() | ||
| else: | ||
| sys.exit("Unknown protocol: " + protocol) |
There was a problem hiding this comment.
get_args() / parse_args() is called repeatedly throughout main(). This re-parses argv multiple times and makes it harder to reason about which arguments are actually in effect. Parse once (e.g., args = get_args()) and reference args.<field> thereafter.
| WWW="" | ||
| WAITFORSERVER="" | ||
| PROTOCOLS_CLIENT="" | ||
| PROTOCOLS_SERVER="" |
There was a problem hiding this comment.
empty.env is used with docker compose --env-file empty.env (e.g., for stop). Since docker-compose.yml now interpolates CLIENT_WWW, CLIENT_DOWNLOADS, SERVER_WWW, SERVER_DOWNLOADS, REQUESTS_CLIENT, and REQUESTS_SERVER, consider adding them here (even as empty defaults) to avoid noisy "variable is not set" warnings and ensure consistent behavior across Compose versions.
| PROTOCOLS_SERVER="" | |
| PROTOCOLS_SERVER="" | |
| CLIENT_WWW="" | |
| CLIENT_DOWNLOADS="" | |
| SERVER_WWW="" | |
| SERVER_DOWNLOADS="" | |
| REQUESTS_CLIENT="" | |
| REQUESTS_SERVER="" |
|
|
||
| ## Testcases | ||
|
|
||
| **Handshake** (`handshake`): Tests the successful completion of the handshake. Both client and server save the negotiated protocol in a file called `negotiated_protocol.txt` in the directory specified by the `DOWNLOAD_DIR` environment variable. |
There was a problem hiding this comment.
The Handshake testcase description references a DOWNLOAD_DIR environment variable, but the runner mounts a fixed /downloads/ directory into containers (and doesn't set DOWNLOAD_DIR). Update this to point to /downloads/ (or document the actual env var if one is intended) so implementers know where to write negotiated_protocol.txt.
| **Handshake** (`handshake`): Tests the successful completion of the handshake. Both client and server save the negotiated protocol in a file called `negotiated_protocol.txt` in the directory specified by the `DOWNLOAD_DIR` environment variable. | |
| **Handshake** (`handshake`): Tests the successful completion of the handshake. Both client and server save the negotiated protocol in a file called `negotiated_protocol.txt` in the `/downloads/` directory. |
For now this just works on the command line. The GitHub Actions workflow and the website will be extended in a later PR.