Skip to content
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

Merged
merged 2 commits into from
Feb 29, 2024

Conversation

mxinden
Copy link
Contributor

@mxinden mxinden commented Feb 24, 2024

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)  |
|         | ?()  |   ?()   |  ?()   |
|         | ✕()  |   ✕()   |  ✕()   |
+---------+------+---------+--------+

Before I polish the actual implementation, @marten-seemann can you give this a high level review? Detailed comments are appreciated but not yet necessary.

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)  |
|         | ?()  |   ?()   |  ?()   |
|         | ✕()  |   ✕()   |  ✕()   |
+---------+------+---------+--------+
```
@marten-seemann
Copy link
Collaborator

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 --client and --server flag and things will work as expected? Have you tested this?

I'm not sure I'm a big fan of the naming of --must-include, but I have to admit I don't have a better proposal.

@mxinden
Copy link
Contributor Author

mxinden commented Feb 26, 2024

I assume I can use this flag in conjunction with the --client and --server flag and things will work as expected? Have you tested this?

Yes, as far as I as a newcomer can tell. Take an implementations.json with 4 implementations:

{
  "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"
  }
}
$ python3 run.py
[('quic-go', 'quic-go'), ('quic-go', 'ngtcp2'), ('quic-go', 'mvfst'), ('quic-go', 'quiche'), ('ngtcp2', 'quic-go'), ('ngtcp2', 'ngtcp2'), ('ngtcp2', 'mvfst'), ('ngtcp2', 'quiche'), ('mvfst', 'quic-go'), ('mvfst', 'ngtcp2'), ('mvfst', 'mvfst'), ('mvfst', 'quiche'), ('quiche', 'quic-go'), ('quiche', 'ngtcp2'), ('quiche', 'mvfst'), ('quiche', 'quiche')]
$ python3 run.py --must-include quic-go
[('quic-go', 'quic-go'), ('quic-go', 'ngtcp2'), ('quic-go', 'mvfst'), ('quic-go', 'quiche'), ('ngtcp2', 'quic-go'), ('mvfst', 'quic-go'), ('quiche', 'quic-go')]
$ python3 run.py --must-include quic-go --client ngtcp2
[('ngtcp2', 'quic-go')]
$ python3 run.py --must-include quic-go --client quic-go
[('quic-go', 'quic-go'), ('quic-go', 'ngtcp2'), ('quic-go', 'mvfst'), ('quic-go', 'quiche')]
$ python3 run.py --must-include quic-go --client quic-go --server ngtcp2
[('quic-go', 'ngtcp2')]

I'm not sure I'm a big fan of the naming of --must-include, but I have to admit I don't have a better proposal.

Would --must-include-implementation be more intuitive @marten-seemann? I am fine with any really.

If possible, I suggest testing the full pipeline up to https://interop.seemann.io/ before merging.

@marten-seemann
Copy link
Collaborator

Alternative: --cross-test (suggested by ChatGPT). The output kind of looks like a cross.

Not sure if that's a lot more intuitive though.

@mxinden
Copy link
Contributor Author

mxinden commented Feb 29, 2024

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.

@marten-seemann
Copy link
Collaborator

Fair point. Let's ship it!

@marten-seemann marten-seemann merged commit 7f922fa into quic-interop:master Feb 29, 2024
1 check passed
mxinden added a commit to mxinden/quic-interop-runner that referenced this pull request Mar 25, 2024
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
marten-seemann pushed a commit that referenced this pull request Mar 26, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants