Add testmask tool for conditional CI test execution #4017
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pietern
requested review from
andrewnester,
anton-107,
denik and
shreyas-goenka
as code owners
pietern
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
pietern
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
pietern
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
denik
reviewed
Nov 27, 2025
tools/testmask/main_test.go
Outdated
| expectedAcceptance: []string{"bundle"}, | ||
| }, | ||
| { | ||
| name: "skip testdata", |
Contributor
There was a problem hiding this comment.
why is this called "skip testdata"?
also, should have a test where testdata alone is modified?
denik
reviewed
Nov 27, 2025
Collaborator
|
Commit: 459f899
9 failing tests:
Top 30 slowest tests (at least 2 minutes):
|
pietern
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
pietern
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
pietern
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
pietern
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
pietern
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
pietern
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
pietern
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
pietern
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
denik
reviewed
Dec 1, 2025
Makefile
Outdated
|
|
||
| test-slow: | ||
| ${GOTESTSUM_CMD} ${PACKAGES} -- -timeout=${LOCAL_TIMEOUT} | ||
| make test SHORT_FLAG="-short" |
Contributor
There was a problem hiding this comment.
Should be the opposite? SHORT_FLAG="" for test-slow and SHORT_FLAG="-short" for test.
The "reuse" by calling make recursively IMO is not worth it I'd just copy paste the two lines directly with appropriate flags. What if I'm running /usr/local/bin/make or make.exe for whatever reason?
Makefile
Outdated
| TEST_PACKAGES = ./acceptance/internal ./libs/... ./internal/... ./cmd/... ./bundle/... ./experimental/aitools/... ./experimental/ssh/... . | ||
|
|
||
| # Default acceptance test filter (all) | ||
| ACCEPTANCE_TEST_FILTER = TestAccept |
Contributor
There was a problem hiding this comment.
This default means if I don't touch experimental stuff I still run tests for it? I thought we wanted to skip them?
Makefile
Outdated
| test: | ||
| ${GOTESTSUM_CMD} ${PACKAGES} -- -timeout=${LOCAL_TIMEOUT} -short | ||
| ${GOTESTSUM_CMD} --packages "${TEST_PACKAGES}" -- -timeout=${LOCAL_TIMEOUT} ${SHORT_FLAG} | ||
| ${GOTESTSUM_CMD} --packages ./acceptance/... -- -timeout=${LOCAL_TIMEOUT} ${SHORT_FLAG} -run ${ACCEPTANCE_TEST_FILTER} |
Contributor
There was a problem hiding this comment.
These are now sequenced.
Both have a decent degree of parallelism so the impact on runtime should be negligible.
They don't have parallelism at the end of the run, when slow hanging test is taking forever to finish.
We can make them parallel at make level, by putting them into separate targets and adding dependency on both (which is a better structure anyway, regardless of performance).
Makefile
Outdated
| TEST_PACKAGES = ./acceptance/internal ./libs/... ./internal/... ./cmd/... ./bundle/... ./experimental/aitools/... ./experimental/ssh/... . | ||
|
|
||
| # Default acceptance test filter (all) | ||
| ACCEPTANCE_TEST_FILTER = TestAccept |
Contributor
There was a problem hiding this comment.
Also, why is it TestAccept and not empty string? We also have TestInprocessMode in acceptance_test.go
Makefile
Outdated
| TEST_PACKAGES = ./acceptance/internal ./libs/... ./internal/... ./cmd/... ./bundle/... ./experimental/aitools/... ./experimental/ssh/... . | ||
|
|
||
| # Default acceptance test filter (all) | ||
| ACCEPTANCE_TEST_FILTER = TestAccept |
Contributor
There was a problem hiding this comment.
One possible approach on how exclude those targets in 'test': add Slow = true to acceptance/experimental/test.toml then configure all this experimental targets to include slow tests.
pietern
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
denik
approved these changes
Dec 2, 2025
denik
reviewed
Dec 2, 2025
| go run . ${{ github.event.merge_group.head.sha }} ${{ github.event.merge_group.base.sha }} | tee output.json | ||
| echo "targets=$(jq -c '.' output.json)" >> $GITHUB_OUTPUT | ||
| - name: Run testmask (other events) |
Contributor
There was a problem hiding this comment.
- name: Run testmask (merge group)
if: ${{ github.event_name == 'merge_group' }}
In the above step name matches the condition, but this one does not? (the pull_request is excluded in both).
- name: Run testmask (other events)
- if: ${{ github.event_name != 'pull_request' && github.event_name != 'merge_group' }}
Contributor
Author
There was a problem hiding this comment.
There is another step above it for pull requests.
pietern
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Use head_sha and base_sha instead of head.sha and base.sha for merge group events. The merge_group event structure has flat properties, unlike pull_request events. See: https://docs.github.com/en/webhooks/webhook-events-and-payloads#merge_group
pietern
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Merged
pietern
added a commit
that referenced
this pull request
Dec 2, 2025
## Changes The `TEST_PACKAGES` variable no longer includes acceptance tests (#4017). This made the step that merges acceptance test coverage fail. ## Tests Locally: ``` make cover TEST_PACKAGES=./libs/env ACCEPTANCE_TEST_FILTER=TestAccept/selftest ```
Collaborator
|
Commit: 9202af4
64 failing tests:
Top 50 slowest tests (at least 2 minutes):
|
pietern
added a commit
that referenced
this pull request
Dec 3, 2025
Consolidate integration workflow files into a single job in push.yml that depends on the testmask job introduced in #4017. Integration tests are now only triggered for pull requests when the "test" target is included in the testmask output. When skipped, a success status is posted to satisfy the required check without running tests. This reduces CI resource usage for changes that don't affect code covered by integration tests (e.g., documentation-only changes).
github-actions bot
pushed a commit
that referenced
this pull request
Dec 3, 2025
## Changes Consolidate integration workflow files into a single job in `push.yml` that depends on the testmask job introduced in #4017. Integration tests are now only triggered for pull requests when the "test" target is included in the testmask output. When skipped, a success status is posted to satisfy the required check without running tests. ## Why This reduces CI resource usage for changes that don't affect code covered by integration tests (e.g., changes to ./experimental), and reduces the time-to-signal and time-to-merge for those changes. ## Tests * Integration tests were triggered in this PR * Integration tests were skipped in #4070
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
A new tool
testmaskundertools/that determines the set of Makefile targets to run for testing.The test jobs all depend on a job that runs this tool and use its output to determine if they should run or skip. If a change only touches files under
experimental/ssh, for example, it won't trigger the main test target. We can extend this to add more selective targets, only if we are sure that there is no dependency between the files matched for the target and other files in the repository. This, for now, relies on convention rather than strict enforcement.How
Every file in the diff between the PR and the PR base is attributed to one of the targets. If all files map to the same target, only that target is run. If files map to multiple targets, multiple targets are run. If there are files that cannot be mapped to a specific target, then the main
testtarget is run.Mapping to GitHub Actions
This approach uses a GitHub Actions test job for every target. This results in a status check mark for each of them, making it easy to eyeball which tests ran on a PR, which tests can on main, and when they broke. Executing the same locally is straightforward because they are all named after Makefile targets.
It applies to pull requests and merge queue checks. Scheduled runs and main branch pushes always run the full test suite.
Why
Not all tests need to run for every change. Being more selective reduces build times and removes noise in PRs.
Tests
The tests for this PR show it works.
A new test in
tools/testmaskconfirms that the targets it maps to are present in the Makefile.PRs for each of the targets: