Uh oh!
There was an error while loading. Please reload this page.
acc: make changed-test detection reusable - #5880
Open
pietern wants to merge 6 commits into
Open
Conversation
Move the "which acceptance test dirs did this branch's diff affect?" logic out
of the skiplocal_test.go helper and into acceptance/internal, so both the
existing DATABRICKS_TEST_SKIPLOCAL=withchanged path and future callers can share
one detection function.
- internal.ChangedTests(baseRef, headRef, testDirs) runs the git diff and returns
a map of test dir -> ChangedTest{Added, VariantFilters}. An empty headRef diffs
against the working tree (preserving withchanged's local-dev behavior of
re-enabling uncommitted edits); a non-empty headRef diffs between two refs,
independent of the checkout, for CI and replaying historical PRs.
- internal.ParseChangedTests is split out as a pure function over git diff text,
unit-tested with literal diff strings (added dir, rename, nested dir, invariant
config INPUT_CONFIG= expansion, files outside acceptance/).
- selectChangedLocalTests keeps only the skiplocal policy (added-first ordering
and the maxChangedLocalTests cap) and delegates detection to ChangedTests.
No behavior change: the withchanged path produces the same selection as before.
Co-authored-by: IsaacContributor
Waiting for approvalBased on git history, these people are best suited to review:
Eligible reviewers: Suggestions based on git history. See OWNERS for ownership rules. |
A deleted invariant config (D status) previously added an INPUT_CONFIG= filter for a variant that no longer exists, so the invariant subdirs were re-enabled for a config that can never match. Skip deletions in the invariant-config branch. Also add deletion coverage to the parser tests: a deleted file in a still-present test dir re-enables that dir; a deleted whole test dir (script gone) is ignored; a deleted config is ignored; and a deleted config alongside a modified one keeps only the modified config's filter. Co-authored-by: Isaac
Collaborator
Integration test reportCommit: 658e3b7
8 interesting tests: 4 RECOVERED, 4 SKIP
Top 10 slowest tests (at least 2 minutes):
|
ParseChangedTests overwrote the whole ChangedTest for every changed file in a dir, so a later diff line (e.g. test.toml) reset Added to false after an added script had set it true. Since git diff lines for a dir arrive in arbitrary order, a brand-new dir could end up Added=false, breaking the added-first ordering that selectChangedLocalTests relies on under maxChangedLocalTests. Preserve the existing entry when a direct change re-enables all variants: clear VariantFilters but OR in Added. This restores parity with the pre-refactor algorithm's separate added map. Add tests covering both diff-line orders and the added-subdir-overrides-config-filter interaction. Co-authored-by: Isaac
The mapper only saw a changed file's own innermost test dir, so it missed changes to files that affect tests in other dirs: - A parent script.prepare/script.cleanup/test.toml is concatenated or inherited into every descendant test, so it now re-enables that whole subtree (works even when its own dir is also a test dir with nested test dirs under it). - A helper under acceptance/bin/ is on PATH for every test, so a change there re-enables the whole suite. - Any other changed file that no test dir owns (a sourced _script, a fixture read via $TESTDIR/..) re-enables every test dir in its subtree. This over-selects but errs toward running more tests, never skipping a real one. A stray file at the acceptance root maps to nothing and is ignored. Verified against real commits: a root script.prepare change (49ea8ae) now re-enables every test dir; earlier resource/config PRs are unchanged. Co-authored-by: Isaac
Two edge cases surfaced in review, both from the parser looking at only the new path of each diff record and treating deletes too coarsely: - A rename affects tests at both paths, but only the new path was classified. Moving an inherited test.toml/script.prepare out of a parent dir left its old descendants unmarked. Expand each rename record into a deletion of the old path plus a modification of the new, so one classifier covers deletes, renames, and edits uniformly (the new path stays "M", not "A", so a moved script does not mark its dir brand-new). - A deleted invariant config helper (job.yml.tmpl-init.sh) was ignored along with the config template itself, but the template still exists and its variant changed. Only skip a deletion when the deleted path is the .yml.tmpl template. Extract the per-path logic into classifyPath so the rename expansion reuses it. Co-authored-by: Isaac
Pure refactor, no behavior change: classifyPath was a five-branch precedence ladder mixing dispatch with dense per-case logic. Turn it into a switch that delegates the two knowledge-dense cases to named helpers (markInvariantConfig, markTestDir), so the dispatch reads at a glance and the details are opt-in. Co-authored-by: Isaac
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Extract the changed-test detection added in #5844 (
DATABRICKS_TEST_SKIPLOCAL=withchanged) intoacceptance/internalso it is generic for any functionality that wants to inspect which acceptance tests a diff touches, and harden it to catch cross-directory dependencies it previously missed.internal.ChangedTests(baseRef, headRef, testDirs)runs the git diff; an emptyheadRefdiffs against the working tree, a non-empty one diffs between two refs (checkout-independent).internal.ParseChangedTestsis a pure function overgit diff --name-statustext, unit-tested with literal diff strings.selectChangedLocalTestskeeps only the skiplocal policy (added-first ordering,maxChangedLocalTestscap) and delegates detection.INPUT_CONFIG=filter for a variant that is gone.ChangedTest.Addedsticky across diff lines so a newly-added dir stays added regardless of line order.script.prepare/script.cleanup/test.tomlre-enables its whole subtree; a change underacceptance/bin/re-enables the whole suite; any other unowned file (a sourced_script, a fixture read via$TESTDIR/..) re-enables its subtree.This pull request and its description were written by Isaac.