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(.github): run QUIC Interop Runner #4339

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

mxinden
Copy link
Collaborator

@mxinden mxinden commented Mar 1, 2024

On a pull request, run the QUIC Interop Runner testcases.

Runs the pull request as a client and server against all other available implementations.

Depends on quic-interop/quic-interop-runner#356.

On a pull request, run the QUIC Interop Runner testcases.

Runs the pull request as a client and server against all other available implementations.
@mxinden
Copy link
Collaborator Author

mxinden commented Mar 1, 2024

This workflow requires approval from a maintainer. Learn more about approving workflows.

Mind approving the workflow @marten-seemann? I expect I will have to iterate on this pull request a bit. Any way we can get around this restriction?

@marten-seemann
Copy link
Member

Mind approving the workflow @marten-seemann? I expect I will have to iterate on this pull request a bit. Any way we can get around this restriction?

Sure! I don't think GitHub allows me to white-list you, since you're a first-time contributor. The only way to get around that would be to get another PR merged. Maybe you can find a typo in a comment or in the README somewhere? 🤡

Copy link

codecov bot commented Mar 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.80%. Comparing base (a70419b) to head (20b2136).

❗ Current head 20b2136 differs from pull request most recent head 61c8c62. Consider uploading reports for the commit 61c8c62 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4339      +/-   ##
==========================================
- Coverage   84.82%   84.80%   -0.02%     
==========================================
  Files         150      150              
  Lines       14287    14287              
==========================================
- Hits        12118    12115       -3     
- Misses       1669     1672       +3     
  Partials      500      500              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@marten-seemann
Copy link
Member

Better solution: I can just invite you as a collaborator on the repo.

build-args: |
GITREF=${{ steps.tag.outputs.gitref }}
tags: martenseemann/quic-go-interop:${{ steps.tag.outputs.tag }}
- name: Run QUIC Interop tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to make this a separate step:

  1. Building the image is cheap (this is Go after all ;))
  2. The run triggers are orthogonal anyway, so there's no condition where we'd build twice anyway.

Copy link
Collaborator Author

@mxinden mxinden Mar 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to make this a separate step:

Note that it is already a separate step. Are you suggesting to make it a separate job instead? Or maybe even a separate workflow? Happy to move it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, separate job is what I meant, sorry for mixing up the terminology here. I think I'd even prefer a separate workflow, since it's (almost) orthogonal to this one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. See 2b5e635.

@mxinden
Copy link
Collaborator Author

mxinden commented Mar 4, 2024

Making progress. Latest run:

+----------------+----------------------------------------------+----------------------------------------------+--------------------------------------------+
|                |                quic-go-latest                |                   quic-go                    |                   msquic                   |
+----------------+----------------------------------------------+----------------------------------------------+--------------------------------------------+
| quic-go-latest | ✓(H,DC,LR,C20,M,S,R,Z,3,B,U,A,L1,L2,C1,C2,6) | ✓(H,DC,LR,C20,M,S,R,Z,3,B,U,A,L1,L2,C1,C2,6) | ✓(H,DC,LR,C20,M,S,R,Z,B,U,A,L1,L2,C1,C2,6) |
|                |                   ?(E,V2)                    |                   ?(E,V2)                    |                 ?(3,E,V2)                  |
|                |                     ✕()                      |                     ✕()                      |                    ✕()                     |
+----------------+----------------------------------------------+----------------------------------------------+--------------------------------------------+
|    quic-go     | ✓(H,DC,LR,C20,M,S,R,Z,3,B,U,A,L1,L2,C1,C2,6) |                                              |                                            |
|                |                   ?(E,V2)                    |                                              |                                            |
|                |                     ✕()                      |                                              |                                            |
+----------------+----------------------------------------------+----------------------------------------------+--------------------------------------------+
|      neqo      |   ✓(H,DC,LR,C20,M,S,R,3,B,U,A,L2,C1,C2,6)    |                                              |                                            |
|                |                   ?(E,V2)                    |                                              |                                            |
|                |                   ✕(Z,L1)                    |                                              |                                            |
+----------------+----------------------------------------------+----------------------------------------------+

https://github.com/quic-go/quic-go/actions/runs/8143949752/job/22256953717?pr=4339

(Note that the workflow that is posting a comment will only run once this pull request is merged, i.e. once the workflow definition is in master.)

@marten-seemann
Copy link
Member

Very impressive!

I see that the runtime of this step is now 1.5h. Have you considered parallelizing it, similar to what the interop runner does? I realize that we then won't need the --must-include flag anymore (it's a useful addition nevertheless). Then we could run the entire test in just 10-15 minutes (maybe even faster?), which would make it more useful on PRs.

@mxinden
Copy link
Collaborator Author

mxinden commented May 3, 2024

mozilla/neqo#1847 adds parallelization to the Neqo Interop action, as Marten proposed above. I hope to get to port this to quic-interop/quic-interop-runner#356 at some point and would then update this pull request.

Sorry for the delay.

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.

None yet

2 participants