acc: Introduce phases; speed up "test-update" 2x.#4795
Open
Conversation
denik
requested review from
andrewnester,
anton-107,
pietern,
shreyas-goenka and
simonfaltum
as code owners
denik
changed the title
Suggested reviewersBased on git history of the changed files, these people are best suited to review:
Confidence: high Eligible reviewersBased on CODEOWNERS, these people or teams could also review: @andrewnester, @anton-107, @pietern, @simonfaltum Suggestions based on git history of 14 changed files (5 scored). See CODEOWNERS for path-specific ownership rules. |
Collaborator
|
Commit: cee9c0d
20 interesting tests: 9 SKIP, 7 KNOWN, 4 flaky
Top 20 slowest tests (at least 2 minutes):
|
Add a non-inheritable Phase setting to test.toml so acceptance tests can wait for earlier phases before running, and mark the template and permissions tests that must run after phase 0.
Collect runnable acceptance tests up front so phase 0 runs immediately and phase 1 only starts after all phase 0 tests complete.
…aphore - Remove runnableTest struct and phaseSemaphore in favor of sync.WaitGroup and a plain channel (phase1Gate) - Keep all test logic inside t.Run callbacks (original structure) - Phase=0 tests call phase0wg.Add(1) before t.Parallel() and t.Cleanup(phase0wg.Done) after all subtests complete; phase=1 tests block on <-phase1Gate - Use t.Cleanup (not defer) so the gate opens only after all EnvMatrix subtests finish writing their output files - Move CloudSlow→Cloud mutation out of getSkipReason to call site, before GenerateMaterializedConfig, so out.test.toml captures the implied value Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
denik
force-pushed
the
denik/test-phases
branch
from
2c01e5d to
7e3e8e8
Compare
…ashes on Windows Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
pietern
approved these changes
Mar 19, 2026
|
|
||
| // Execution phase for this test. Tests run in ascending phase order. | ||
| // Phase is not inherited from parent test.toml files. Default is 0. | ||
| Phase int `inherit:"false"` |
Contributor
There was a problem hiding this comment.
Can you copy the comment from the Makefile here?
Something along the lines of:
at the moment second pass is required because some tests show diff against output of another test for easier review
Makes it easier to understand why this is needed. An example would be nice too.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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
Why
We have tests that depend on output of other tests. Because of that "make test-update" has to "go test -update" twice. This is no longer needed, one update is enough.
Tests
Manually, by removing all output for local tests and running full update: