acc: Introduce phases; speed up "test-update" 2x.#4795
Merged
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: a60dcfb
20 interesting tests: 9 SKIP, 7 KNOWN, 4 flaky
Top 22 slowest tests (at least 2 minutes):
|
denik
force-pushed
the
denik/test-phases
branch
from
2c01e5d to
7e3e8e8
Compare
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.
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>
…ashes on Windows Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
denik
force-pushed
the
denik/test-phases
branch
from
cee9c0d to
3f9f164
Compare
Phase 1 tests read output files from phase 0 tests via $TESTDIR, which points to the source directory. In normal runs those files are already committed and stable, so the phase gate just adds unnecessary serialization. Only activate the WaitGroup/channel gate when -update is set. Co-authored-by: Isaac
Collaborator
|
Commit: 2acf955
55 interesting tests: 25 flaky, 17 RECOVERED, 9 KNOWN, 3 FAIL, 1 SKIP
Top 50 slowest tests (at least 2 minutes):
|
Collaborator
|
Commit: 2acf955 |
Collaborator
|
Commit: 2acf955 |
github-merge-queue bot
pushed a commit
that referenced
this pull request
Mar 23, 2026
## Changes If -subset (or -update) option is passed, only a few tests selected from EnvMatrix-generated test cases: by default a single test is selected from the whole matrix. The configurations with DATABRICKS_BUNDLE_ENGINE=direct setting get 10x weight, so they are preferred over terraform (but we still run a few tests on terraform). We can do this, because all subtests are supposed to generate the same output. The exception is tests that do `> out.$DATABRICKS_BUNDLE_ENGINE.txt` redirects. Those need both terraform and direct variants to run. For such tests, we select two variants, one with "terraform" and another with "direct" engine. The exact rule for triggering this: either script in current directory or _script in any parent directories contains "$DATABRICKS_BUNDLE_ENGINE" substring. ## Why - Speed up "make test-update". - Potentially more correct update, since parallel runs do not write the same files at the same time. ## Tests Just like in #4795 remove all output files and check that "make test-update" regenerates them all. This branch: ``` % git grep -l 'Local = true' '**/out.test.toml' | find_out_files.py | xargs rm % time make test-update # This branch … make test-update 561.48s user 266.50s system 659% cpu 2:05.49 total ``` main: ``` % git grep -l 'Local = true' '**/out.test.toml' | find_out_files.py | xargs rm … % time make test-update # main make test-update 1063.22s user 442.75s system 709% cpu 3:32.27 total ``` --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
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: