-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
wip: set up very expensive tests to run in CI #12939
base: master
Are you sure you want to change the base?
Conversation
18a97fb
to
8efd0e7
Compare
8efd0e7
to
f204a6a
Compare
ef83f34
to
78a75b9
Compare
e0067e8
to
0057aca
Compare
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.
Sweet - thanks for doing this!
the very expensive tests workflow is separate to the test workflow. This is so that we don't have to rerun all the tests on a label change.
Makes sense - sounds good.
remove the label
Why do we remove need/very-expensive-tests
? I assume because this is the way to manually trigger the expensive test run for the PR.
I guess in a perfect world if the need/very-expensive-tests
label was present, the expensive test groups would become a required CI check and would automatically get run whenever other tests are run.
The information about that is currently stored in the ci cmd tool. It will be moved to the test files themselves in the future once we move more towards the self-identification setup.
Sounds good.
Question: should the workflow comment on a PR with the very expensive test results? I think it might be useful since if one modifies labels after the very expensive tests are run, then the link to the very expensive tests run would disappear from the PR.
I think PR's labeled with need/very-expensive-tests
should have the comment in the PR. (Ideally it would behave the same as every other test, except this PR just has extra tests being run as a result of the label.).
For a PR, I don't think we should be cutting issues if very expensive tests fail. I think we should only cut an issue if the very expensive cron job fails.
${{ steps.reports.outputs.path }}/${{ matrix.name }}.json | ||
continue-on-error: true | ||
name: Test | ||
uses: ./.github/workflows/reusable-test.yml |
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.
uses: ./.github/workflows/reusable-test.yml | |
uses: ./.github/workflows/reusable-test.yml | |
with: | |
run_very_expensive_tests: false |
I know that's the default, but it makes it explicit that this workflow isn't running with expensive tests.
Flags: []cli.Flag{ | ||
&cli.BoolFlag{ | ||
Name: "very-expensive-tests-run", | ||
Usage: "Whether the groups not containing any very expensive tests should be filtered out", |
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.
Usage: "Whether the groups not containing any very expensive tests should be filtered out", | |
Usage: "Whether to only include the groups with very expensive tests", |
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.
Does this mean that if the boolean flag is not set that our test run includes all tests (i.e., not-very-expensive-tests + very-expensive-tests)?
Related Issues
Closes #12136
Proposed Changes
In this PR I set up a new workflow responsible for running very expensive tests. I also enable running expensive tests in our regular test workflow.
Additional Info
This is an updated version of #12234
In this iteration, the very expensive tests workflow is separate to the test workflow. This is so that we don't have to rerun all the tests on a label change.
The way the new workflow works is that when one adds a
need/very-expensive-tests
label to a PR, the workflow will start, remove the label, and execute the tests.The new workflow will only execute the tests that have very expensive components to them. The information about that is currently stored in the
ci
cmd tool. It will be moved to the test files themselves in the future once we move more towards the self-identification setup.This is not ready to be merged because the only very expensive test we have at the moment is failing with
signal: killed
. Here's a link to the most recent failure: https://github.com/filecoin-project/lotus/actions/runs/13706584703Question: should the workflow comment on a PR with the very expensive test results? I think it might be useful since if one modifies labels after the very expensive tests are run, then the link to the very expensive tests run would disappear from the PR.
Checklist
Before you mark the PR ready for review, please make sure that: