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 neqoon 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.