-
Notifications
You must be signed in to change notification settings - Fork 104
workflows: only run the modified tests #2745
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
base: master
Are you sure you want to change the base?
Conversation
Running all the tests on PR or push event of suricata-verify is irrelevant as the tests are supposed to be exclusive of one another. Make sure that unless there's a framework or workflows change, only the tests that are modified or added are run. This saves CI resources that would otherwise be unnecessarily spent. This does not affect the testing and coverage of the Suricata codebase as when there's an s-v PR, Suricata's workflow is to clone the entire repo and run all the tests in there.
636ea71 to
63bb027
Compare
| # Get names of all dirs changed in a PR | ||
| dirnames=$(git diff --name-only "${firstCommit}" "${lastCommit}" | xargs dirname | sort -u) | ||
| echo "Dirnames: $dirnames" | ||
| # check iff tests/ dir has changed | ||
| only_tests=1 | ||
| for dir in $dirnames; do | ||
| if [[ $dir != tests/* ]]; then | ||
| echo "$dir is not tests/" | ||
| only_tests=0 | ||
| break | ||
| fi | ||
| done | ||
| if [ $only_tests == 1 ]; then | ||
| # Get the names of the innermost test dir | ||
| testnames=$(echo "$dirnames" | xargs -I {} basename "{}" | tr '\n' ' ') | ||
| echo "PR diff shows a change was done in test dirs. Running $testnames" | ||
| final_tests=$(echo "--exact $testnames") | ||
| fi |
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 I could put all of this in a bash script btw and call it here to make the job even smaller to read.
| # check iff tests/ dir has changed | ||
| only_tests=1 | ||
| for dir in $dirnames; do |
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.
instead of loop, single grep -v ?
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.
TIL. Thank you! 🙇🏽♀️
| fi | ||
| ;; | ||
| esac | ||
| echo "args=$final_tests" >> "$GITHUB_OUTPUT" |
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.
Why do we go about final_tests here ? and test_names in other jobs ?
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.
Also, should not final_tests be defined also if [ $only_tests != 1 ]; ?
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.
Why do we go about final_tests here ? and test_names in other jobs ?
I guess I changed the varname at some point. Do you prefer test_names better?
Also, should not final_tests be defined also if [ $only_tests != 1 ]; ?
It is then set to nothing and we want no args to s-v in such a case so it runs the entire test suite. Is this not what you mean? Please lmk. Thank you!
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 guess I changed the varname at some point. Do you prefer test_names better?
No preference, but consistency ;-)
It is then set to nothing
I do not see where...
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.
oh. I thought bash variables are declared as and when they're used and set to null unless overridden. Is this an incorrect assumption?
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.
See bash set -u
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.
thank you! 🙇🏽♀️
Previous PR: #2743
Changes since v4:
tests/-- only the respective changed tests must be runTicket: None. Should there be one?
Known TODOs: