-
-
Notifications
You must be signed in to change notification settings - Fork 86
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
feat: add --must-include flag #355
Conversation
Introduce new command line flag `--must-include`. When running the QUIC Interop Runner in CI of a single implementation, one is only interested in test pairs including that particular implementation. As an example, take a world with 3 QUIC implementations, quic-go, ngtcp2 and neqo. On the neqo CI one is not interested in the client-server pair quic-go->ngtcp2. With this commit one can specify `--must-include neqo` on neqo's CI. ``` $ python3 run.py --test handshake --must-include neqo Saving logs to logs_2024-02-24T17:39:08. Server: neqo. Client: quic-go. Running test case: handshake Server: neqo. Client: ngtcp2. Running test case: handshake Server: quic-go. Client: neqo. Running test case: handshake Server: ngtcp2. Client: neqo. Running test case: handshake Server: neqo. Client: neqo. Running test case: handshake Run took 0:01:50.070133 +---------+------+---------+--------+ | | neqo | quic-go | ngtcp2 | +---------+------+---------+--------+ | quic-go | ✓(H) | | | | | ?() | | | | | ✕() | | | +---------+------+---------+--------+ | ngtcp2 | ✓(H) | | | | | ?() | | | | | ✕() | | | +---------+------+---------+--------+ | neqo | ✓(H) | ✓(H) | ✓(H) | | | ?() | ?() | ?() | | | ✕() | ✕() | ✕() | +---------+------+---------+--------+ ```
This looks good to me. I haven't reviewed the pair generation code in detail. I assume I can use this flag in conjunction with the I'm not sure I'm a big fan of the naming of |
Yes, as far as I as a newcomer can tell. Take an {
"quic-go": {
"image": "martenseemann/quic-go-interop:latest",
"url": "https://github.com/lucas-clemente/quic-go",
"role": "both"
},
"ngtcp2": {
"image": "ghcr.io/ngtcp2/ngtcp2-interop:latest",
"url": "https://github.com/ngtcp2/ngtcp2",
"role": "both"
},
"mvfst": {
"image": "lnicco/mvfst-qns:latest",
"url": "https://github.com/facebookincubator/mvfst",
"role": "both"
},
"quiche": {
"image": "cloudflare/quiche-qns:latest",
"url": "https://github.com/cloudflare/quiche",
"role": "both"
}
}
Would If possible, I suggest testing the full pipeline up to https://interop.seemann.io/ before merging. |
Alternative: Not sure if that's a lot more intuitive though. |
I assume most folks will not use the flag directly, but instead use it through the upcoming GitHub Action (#356). The action requires specifying the name of the implementation to test already, thus users won't even need to entertain the concept in the first place. Long story short, I don't think the name matters much. In addition, given that it is not user-facing in most cases, we can change it anytime. |
Fair point. Let's ship it! |
Previously when calculating each cell of the _measurements_ result table, instead of always setting a cell, a cell would be appended in case one already existed. Appending a cell to another doesn't make sense in this context. In addition this code also errors, given that on the first cell `row[server] +=` results in a `KeyError`. ``` Traceback (most recent call last): File "run.py", line 168, in <module> sys.exit(main()) File "run.py", line 150, in main return InteropRunner( File "/home/runner/work/quic-go/quic-go/quic-interop-runner/interop.py", line 529, in run self._print_results() File "/home/runner/work/quic-go/quic-go/quic-interop-runner/interop.py", line 243, in _print_results row[server] += "\n".join(results) KeyError: 'quic-go-latest' ``` See e.g. https://github.com/quic-go/quic-go/actions/runs/8159188951/job/23046885072. This commit fixes the above simply by setting the cell, not appending it for the case where one already exists. Bug introduced in quic-interop#355
Previously when calculating each cell of the _measurements_ result table, instead of always setting a cell, a cell would be appended in case one already existed. Appending a cell to another doesn't make sense in this context. In addition this code also errors, given that on the first cell `row[server] +=` results in a `KeyError`. ``` Traceback (most recent call last): File "run.py", line 168, in <module> sys.exit(main()) File "run.py", line 150, in main return InteropRunner( File "/home/runner/work/quic-go/quic-go/quic-interop-runner/interop.py", line 529, in run self._print_results() File "/home/runner/work/quic-go/quic-go/quic-interop-runner/interop.py", line 243, in _print_results row[server] += "\n".join(results) KeyError: 'quic-go-latest' ``` See e.g. https://github.com/quic-go/quic-go/actions/runs/8159188951/job/23046885072. This commit fixes the above simply by setting the cell, not appending it for the case where one already exists. Bug introduced in #355
Introduce new command line flag
--must-include
.When running the QUIC Interop Runner in CI of a single implementation, one is only interested in test pairs including that particular implementation. As an example, take a world with 3 QUIC implementations, quic-go, ngtcp2 and neqo. On the neqo CI one is not interested in the client-server pair quic-go->ngtcp2.
With this commit one can specify
--must-include neqo
on neqo's CI.Before I polish the actual implementation, @marten-seemann can you give this a high level review? Detailed comments are appreciated but not yet necessary.