-
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
Improve runtime of tests #1854
Improve runtime of tests #1854
Conversation
86739ba
to
ed19b80
Compare
this way we can compare
Get more information on how much time we spend on each task to better plan CI runtime optimizations.
This reverts commit 6c09212.
Only run that workflow on push to main (instead of PR)
This will be replaced by `nextest.yml` in before merge.
ed19b80
to
c1cd5f5
Compare
We think this will not be required on PR since it is a new workflow.
run: | | ||
export RUSTFLAGS="$RUSTFLAGS --cfg hotshot_example" | ||
export PATH="$PWD/target/release:$PATH" | ||
cargo nextest run --locked --release --workspace --all-features --retries 2 --verbose -E '!test(slow_)' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nice to add just recipies to mirror the new test commands locally since it's also nice to run the tests quickly locally. And sometimes we want to re-run the same tests as in a CI job locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, some suggestions
@@ -0,0 +1,64 @@ | |||
name: SlowEst |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name: SlowEst | |
name: Slow Tests |
RUST_LOG: info,libp2p=off,node=error | ||
|
||
jobs: | ||
slowest: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or is "slowest" chosen on purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I was it was a play on nextest
plus these are in fact the slowest tests.
Closes #1848
This PR improves CI runtime by working around slow tests. The last commit on main, tests took 36min. Tests are currently completing in this PR in 12m.
--retries
(retries and marks tests that succeed upon retry as FLAKYslow
dev-node tests to separate workflow, only run that workflow on push to 'main'Note that slow tests would now only be run on main, but the only tests currently considerd slow are
dev-node
tests. If this is merged more or less as is, I will create an issue to look into why these tests are slow and explore possible solutions.