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

systest: add QUIC support #5902

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from
Draft

systest: add QUIC support #5902

wants to merge 5 commits into from

Conversation

ivan4th
Copy link
Contributor

@ivan4th ivan4th commented Apr 29, 2024

Motivation

QUIC protocol is not verified in systests

Description

This adds a job that runs systests using QUIC instead of TCP

Test Plan

Make sure systests pass in both modes

TODO

  • Explain motivation or link existing issue(s)
  • Test changes and document test plan

@ivan4th
Copy link
Contributor Author

ivan4th commented Apr 29, 2024

bors try

spacemesh-bors bot added a commit that referenced this pull request Apr 29, 2024
Copy link

codecov bot commented Apr 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.6%. Comparing base (63dc6a8) to head (d8024a1).
Report is 5 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff           @@
##           develop   #5902   +/-   ##
=======================================
  Coverage     80.6%   80.6%           
=======================================
  Files          286     285    -1     
  Lines        29508   29547   +39     
=======================================
+ Hits         23801   23833   +32     
- Misses        4116    4130   +14     
+ Partials      1591    1584    -7     

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

@spacemesh-bors
Copy link

try

Build failed:

@ivan4th
Copy link
Contributor Author

ivan4th commented Apr 30, 2024

bors try

spacemesh-bors bot added a commit that referenced this pull request Apr 30, 2024
@spacemesh-bors
Copy link

try

Build failed:

This adds a job that runs systests using QUIC instead of TCP
@ivan4th
Copy link
Contributor Author

ivan4th commented Apr 30, 2024

bors try

spacemesh-bors bot added a commit that referenced this pull request Apr 30, 2024
@spacemesh-bors
Copy link

try

Build failed:

@ivan4th ivan4th marked this pull request as ready for review May 2, 2024 16:34
@dshulyak
Copy link
Contributor

dshulyak commented May 3, 2024

it additionally runs all of the tests but with the quic enabled?
it will use twice as much resources on gke, might be just x2 cost with autoscaling.

it also seems very wastesful, as the change to run quic rather minor.
some other ideas, setup half of the nodes with quic, other with tcp, run one any test with all nodes using quic, add one more additional test specifically with quic e.g TestSmeshingQuic

@fasmat
Copy link
Member

fasmat commented May 3, 2024

Regarding adding a TestSmeshingQuic test or adapting configuration of nodes for tests, I'd say the decision should be based on what we consider the "default" setup.

If both quic and tcp are considered equally supported by us we should just make half of all nodes in every test use quic and the other half tcp. If we consider quic experimental or a feature that only some nodes should enable I'd go with a TestSmeshingQuic test.

@fasmat
Copy link
Member

fasmat commented May 3, 2024

Also running all our system tests twice will more than doubles the number of tries / merges that fail due to a flaky test failing and we should look into stabilizing our system tests first before adding a bunch of additional tests.

@fasmat
Copy link
Member

fasmat commented Jun 4, 2024

What's the status on this?

@ivan4th
Copy link
Contributor Author

ivan4th commented Oct 7, 2024

Converting to draft as the code needs to be more careful about using extra resources for systests

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.

3 participants