acc: Select subset of tests that cover all output#4800
Merged
Conversation
Adds a -subset flag that reduces each EnvMatrix variable to a single value using consistent hashing, cutting test runtime when full coverage is not needed. DATABRICKS_BUNDLE_ENGINE is biased 90% toward "direct" unless the script references the variable, in which case all variants are kept. The flag is automatically enabled during -update mode. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ghts Replace per-variable SubsetEnvMatrix with SubsetExpanded which operates on the already-expanded and exclusion-filtered combo list. This ensures EnvMatrixExclude rules are respected before selection. DATABRICKS_BUNDLE_ENGINE=direct gets weight 10; all other variants weight 1, giving ~10/11 probability of selecting a direct variant. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…NDLE_ENGINE When the script references \$DATABRICKS_BUNDLE_ENGINE (e.g. in output filenames), SubsetExpanded groups combos by engine value and picks one per group, ensuring both terraform and direct variants run. Otherwise it picks one combo total with direct weighted 10x over terraform. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…UNDLE_ENGINE When determining whether to keep all engine variants, walk parent directories checking script.prepare files in addition to the test's own script. Results are cached via sync.Map since parent prepare scripts are shared across many tests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…DLE_ENGINE _script files are bash helpers sourced by test scripts via 'source \$TESTDIR/../_script'. Check these (not script.prepare) when determining whether to keep all engine variants during subset selection. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Collaborator
|
Commit: 569a2a6
20 interesting tests: 9 SKIP, 7 KNOWN, 4 flaky
Top 20 slowest tests (at least 2 minutes):
|
denik
requested review from
andrewnester,
anton-107,
pietern,
shreyas-goenka and
simonfaltum
as code owners
Suggested reviewersBased on git history of the changed files, these people are best suited to review:
Confidence: low Eligible reviewersBased on CODEOWNERS, these people or teams could also review: @andrewnester, @anton-107, @pietern, @simonfaltum Suggestions based on git history of 3 changed files (2 scored). See CODEOWNERS for path-specific ownership rules. |
simonfaltum
reviewed
Mar 22, 2026
Member
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
Review Swarm Summary (2 independent reviewers + cross-review)
Verdict: APPROVE - Clean, well-thought-out optimization. No blocking issues.
The weighted consistent hashing approach is clever and the tests are solid. make test-update going from 3:32 to 2:05 is a meaningful win.
Minor suggestions below (none blocking).
| result := (err == nil && strings.Contains(string(content), "$DATABRICKS_BUNDLE_ENGINE")) || | ||
| anyHelperScriptUsesEngine(filepath.Dir(dir)) | ||
| helperScriptUsesEngineCache.Store(dir, result) | ||
| return result |
Member
There was a problem hiding this comment.
[SUGGESTION] anyHelperScriptUsesEngine() and the Subset wiring at line 395-400 are new harness control flow, but the unit tests only exercise SubsetExpanded() in isolation. A regression in the ancestor _script scan or the -update auto-enable path would not be caught. Consider adding a selftest acceptance case whose output varies by engine only through an ancestor _script, run under -subset/-update so the harness behavior is pinned end-to-end.
Contributor
Author
There was a problem hiding this comment.
Good idea to add self-test as it also serves as documentation. 22304e1
| h.Write([]byte(testDir)) | ||
| return weighted[h.Sum64()%uint64(len(weighted))] | ||
| } | ||
|
|
Member
There was a problem hiding this comment.
[SUGGESTION] weightedSelect will panic on empty input (division by zero). Currently unreachable since SubsetExpanded returns early for len(expanded) <= 1, but a precondition comment would help future maintainers:
// Precondition: envsets must be non-empty.
Contributor
Author
There was a problem hiding this comment.
panic on unsupported input is okay
acceptance/internal/config.go
Outdated
| } | ||
| var result [][]string | ||
| for _, group := range groups { | ||
| result = append(result, weightedSelect(group, testDir)) |
Member
There was a problem hiding this comment.
[NITPICK] The groups map iteration order is non-deterministic, so the order of the returned result slice varies across runs. Doesn't affect correctness but makes debugging harder. Sorting by engine name would make it deterministic.
Contributor
Author
There was a problem hiding this comment.
While sorting would be overkill, this give me an idea that we should build slice directly (and use side map to dedup): 569a2a6
simonfaltum
approved these changes
Mar 22, 2026
…map->slice conversion Replaces map[string][][]string (engine -> group) with a parallel pair of slices (result, groups) indexed by first-seen insertion order, using a keyToIdx map[string]int as the side structure for deduplication. This eliminates the non-deterministic map iteration that determined result order. Co-authored-by: Isaac
denik
added a commit
that referenced
this pull request
Mar 25, 2026
When -update is used with a -run filter that includes a specific EnvMatrix variant (detected by presence of '='), subset selection is not needed — Go's test framework already limits execution to the requested variant. Follow-up to #4800. Co-authored-by: Isaac
denik
added a commit
that referenced
this pull request
Mar 25, 2026
When -update is used with a -run filter that likely includes a specific EnvMatrix variant (detected by presence of '='), subset selection is not needed — Go's test framework already limits execution to the requested variant. Follow-up to #4800.
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
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.txtredirects. 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
Tests
Just like in #4795 remove all output files and check that "make test-update" regenerates them all.
This branch:
main: