diff --git a/acceptance/internal/changed_tests.go b/acceptance/internal/changed_tests.go new file mode 100644 index 00000000000..097b30a0f3a --- /dev/null +++ b/acceptance/internal/changed_tests.go @@ -0,0 +1,213 @@ +package internal + +import ( + "fmt" + "os/exec" + "path/filepath" + "strings" +) + +const ( + invariantConfigsPrefix = "acceptance/bundle/invariant/configs/" + invariantDirPrefix = "bundle/invariant/" + + acceptancePrefix = "acceptance/" + // binPrefix holds helper scripts placed on PATH for every test (print_requests.py, + // contains.py, ...), so a change to any of them can affect any test. + binPrefix = "acceptance/bin/" +) + +// ChangedTest describes how one acceptance test dir was affected by a branch diff. +type ChangedTest struct { + // Added is true when the dir is brand new (its "script" file was added on the branch). + Added bool + + // VariantFilters restricts which env-matrix variants of the dir are relevant. + // nil means all variants; a non-nil slice (e.g. "INPUT_CONFIG=job.yml.tmpl") + // restricts to matching variants. Only invariant-config changes set this. + VariantFilters []string +} + +// ChangedTests returns the acceptance test dirs affected by the diff against the +// merge base with baseRef, mapped to how each was affected. testDirs is the set +// of known test dirs (relative to acceptance/). +// +// When headRef is empty, the diff is against the working tree: this covers +// committed, staged, and unstaged changes alike, which enables the local dev +// workflow of "touch a config, run the test, see it re-enabled" without +// committing (the same reason lintdiff.py uses --merge-base). Untracked files +// (not yet git-added) are not visible to git diff and will not be re-enabled +// until staged or committed. +// +// When headRef is non-empty, the diff is between the two refs (baseRef and +// headRef), independent of what is checked out. This is what CI and tests that +// replay a historical PR want: only committed changes on headRef, no dependence +// on the working tree. The three-dot form baseRef...headRef would only cover +// committed changes too, but --merge-base keeps the semantics identical to the +// working-tree case. +func ChangedTests(baseRef, headRef string, testDirs map[string]bool) (map[string]ChangedTest, error) { + args := []string{"diff", "--name-status", "--merge-base", "-M", baseRef} + if headRef != "" { + args = append(args, headRef) + } + out, err := exec.Command("git", args...).Output() + if err != nil { + return nil, fmt.Errorf("git diff against %s failed: %w", baseRef, err) + } + return ParseChangedTests(string(out), testDirs), nil +} + +// ParseChangedTests turns the output of `git diff --name-status -M` into the set +// of affected acceptance test dirs. It is split from ChangedTests so the mapping +// logic can be unit-tested with literal diff strings, without a git repo. +// +// Each diff line is "\t" (or "\t\t" for renames); +// the last field is the current path. A changed invariant config +// (acceptance/bundle/invariant/configs/*.yml.tmpl) maps to all invariant subdirs +// with an INPUT_CONFIG= filter, so touching job.yml.tmpl re-enables all subdirs +// but only for their job.yml.tmpl variants. A direct change to a subdir takes +// precedence and re-enables all of its variants. +func ParseChangedTests(diffOutput string, testDirs map[string]bool) map[string]ChangedTest { + result := map[string]ChangedTest{} + + for line := range strings.SplitSeq(strings.TrimSpace(diffOutput), "\n") { + fields := strings.Split(line, "\t") + if len(fields) < 2 { + continue + } + status := fields[0] + path := fields[len(fields)-1] + + // A changed invariant config re-enables all invariant subdirs with an + // INPUT_CONFIG filter, unless a subdir was already unlocked by a non-config change. + if strings.HasPrefix(path, invariantConfigsPrefix) { + // A deleted config has no variant left to run, so ignore it. Without + // this, a deletion would add an INPUT_CONFIG= filter matching nothing. + if status == "D" { + continue + } + configName := path[len(invariantConfigsPrefix):] + // Strip -init.sh / -cleanup.sh suffixes to get the base config name. + if i := strings.Index(configName, "-"); i > 0 && strings.HasSuffix(configName, ".sh") { + configName = configName[:i] + } + if strings.HasSuffix(configName, ".yml.tmpl") { + for dir := range testDirs { + if strings.HasPrefix(dir, invariantDirPrefix) { + existing, ok := result[dir] + if !ok || existing.VariantFilters != nil { + existing.VariantFilters = append(existing.VariantFilters, "INPUT_CONFIG="+configName) + result[dir] = existing + } + } + } + } + continue + } + + // bin/ holds helper scripts placed on PATH for every test, so a change to + // any of them can affect any test: re-enable the whole suite. + if strings.HasPrefix(path, binPrefix) { + markSubtree("", testDirs, result) + continue + } + + // A shared file (script.prepare/script.cleanup/test.toml) is concatenated + // or inherited into every test at or below its dir, so it re-enables that + // whole subtree. This runs before the single-dir mapping because such a + // file can live inside a test dir that also has nested test dirs, which + // the single-dir mapping would miss. + if sharedTestFile(path) { + markSubtree(containingDir(path), testDirs, result) + continue + } + + dir := testDirForFile(path, testDirs) + if dir == "" { + // The file is not owned by any test dir. It is a shared input in a + // non-test dir (a sourced _script, a fixture read via $TESTDIR/..), + // so conservatively re-enable every test dir in its subtree. A stray + // file at the acceptance root (no subtree dir) affects nothing + // identifiable and is ignored. + if base := containingDir(path); base != "" { + markSubtree(base, testDirs, result) + } + continue + } + // A direct change re-enables all variants, so clear any config-scoped + // filter (nil VariantFilters = all variants). Added is sticky: once any + // line marks the dir new it stays new, since diff lines for the same dir + // arrive in arbitrary order. A script file with status A means the test + // dir is brand new; renames (R) land here as the destination path but are + // not "added". + ct := result[dir] + ct.VariantFilters = nil + if status == "A" && strings.HasSuffix(path, "/script") { + ct.Added = true + } + result[dir] = ct + } + + return result +} + +// sharedTestFilenames are files that, when they live in an ancestor dir, affect +// every test below them: script.prepare / script.cleanup are concatenated into +// each descendant's script (see readMergedScriptContents in acceptance_test.go) +// and test.toml config is inherited by descendants. +var sharedTestFilenames = map[string]bool{ + "script.prepare": true, + "script.cleanup": true, + "test.toml": true, +} + +// sharedTestFile reports whether path is a shared file whose change propagates to +// every test in its subtree. +func sharedTestFile(path string) bool { + return strings.HasPrefix(path, acceptancePrefix) && sharedTestFilenames[filepath.Base(path)] +} + +// containingDir returns the dir of an acceptance/ path relative to acceptance/ +// ("" for a file directly under acceptance/), or "" if path is outside acceptance/. +func containingDir(path string) string { + if !strings.HasPrefix(path, acceptancePrefix) { + return "" + } + dir := filepath.ToSlash(filepath.Dir(path[len(acceptancePrefix):])) + if dir == "." { + return "" + } + return dir +} + +// markSubtree re-enables (with all variants) every test dir equal to or nested +// under base. An empty base matches every test dir. It preserves any existing +// Added flag and only clears VariantFilters, since a subtree-wide change affects +// all variants. +func markSubtree(base string, testDirs map[string]bool, result map[string]ChangedTest) { + for dir := range testDirs { + if base == "" || dir == base || strings.HasPrefix(dir, base+"/") { + ct := result[dir] + ct.VariantFilters = nil + result[dir] = ct + } + } +} + +// testDirForFile maps a repo-relative changed file (e.g. acceptance/bundle/foo/script) +// to its owning test dir relative to acceptance/ (e.g. bundle/foo), or "" if the file +// is outside acceptance/ or not under any known test dir. +func testDirForFile(repoRelPath string, testDirs map[string]bool) string { + parts := strings.Split(filepath.ToSlash(repoRelPath), "/") + if len(parts) < 2 || parts[0] != "acceptance" { + return "" + } + // Longest ancestor first so nested tests map to the innermost test dir. + for depth := len(parts); depth > 1; depth-- { + candidate := strings.Join(parts[1:depth], "/") + if testDirs[candidate] { + return candidate + } + } + return "" +} diff --git a/acceptance/internal/changed_tests_test.go b/acceptance/internal/changed_tests_test.go new file mode 100644 index 00000000000..930806865e4 --- /dev/null +++ b/acceptance/internal/changed_tests_test.go @@ -0,0 +1,243 @@ +package internal + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// testDirsFixture is the set of known acceptance test dirs used across the +// parser tests. Values mirror real acceptance/ layout: some plain bundle tests +// and several bundle/invariant subdirs that share the configs/ directory. +func testDirsFixture() map[string]bool { + return map[string]bool{ + "bundle/resources": true, + "bundle/resources/jobs": true, + "bundle/invariant/no_drift": true, + "bundle/invariant/migrate": true, + "bundle/invariant/continue_293": true, + "cmd/version": true, + } +} + +func TestParseChangedTests(t *testing.T) { + tests := []struct { + name string + diff string + want map[string]ChangedTest + }{ + { + name: "empty diff", + diff: "", + want: map[string]ChangedTest{}, + }, + { + name: "modified file maps to owning test dir", + diff: "M\tacceptance/bundle/resources/script", + want: map[string]ChangedTest{ + "bundle/resources": {Added: false, VariantFilters: nil}, + }, + }, + { + name: "added script marks the dir as added", + diff: "A\tacceptance/bundle/resources/jobs/script", + want: map[string]ChangedTest{ + "bundle/resources/jobs": {Added: true, VariantFilters: nil}, + }, + }, + { + name: "added dir stays added when another file in it is also changed", + diff: "A\tacceptance/bundle/resources/jobs/script\nM\tacceptance/bundle/resources/jobs/test.toml", + want: map[string]ChangedTest{ + "bundle/resources/jobs": {Added: true, VariantFilters: nil}, + }, + }, + { + name: "added dir stays added regardless of diff line order", + diff: "M\tacceptance/bundle/resources/jobs/test.toml\nA\tacceptance/bundle/resources/jobs/script", + want: map[string]ChangedTest{ + "bundle/resources/jobs": {Added: true, VariantFilters: nil}, + }, + }, + { + name: "nested file maps to innermost test dir", + diff: "M\tacceptance/bundle/resources/jobs/output.txt", + want: map[string]ChangedTest{ + "bundle/resources/jobs": {Added: false, VariantFilters: nil}, + }, + }, + { + name: "file outside acceptance is ignored", + diff: "M\tlibs/dyn/value.go", + want: map[string]ChangedTest{}, + }, + { + name: "file under acceptance but not in any test dir is ignored", + diff: "M\tacceptance/README.md", + want: map[string]ChangedTest{}, + }, + { + name: "rename destination is used and is not treated as added", + diff: "R092\tacceptance/bundle/resources/old\tacceptance/bundle/resources/script", + want: map[string]ChangedTest{ + "bundle/resources": {Added: false, VariantFilters: nil}, + }, + }, + { + name: "deleted file in a still-existing test dir re-enables that dir", + diff: "D\tacceptance/bundle/resources/output.txt", + want: map[string]ChangedTest{ + "bundle/resources": {Added: false, VariantFilters: nil}, + }, + }, + { + name: "parent script.prepare re-enables all descendant test dirs", + diff: "M\tacceptance/bundle/resources/script.prepare", + want: map[string]ChangedTest{ + "bundle/resources": {Added: false, VariantFilters: nil}, + "bundle/resources/jobs": {Added: false, VariantFilters: nil}, + }, + }, + { + name: "parent script.cleanup re-enables all descendant test dirs", + diff: "M\tacceptance/bundle/resources/script.cleanup", + want: map[string]ChangedTest{ + "bundle/resources": {Added: false, VariantFilters: nil}, + "bundle/resources/jobs": {Added: false, VariantFilters: nil}, + }, + }, + { + name: "parent test.toml re-enables all descendant test dirs", + diff: "M\tacceptance/bundle/invariant/test.toml", + want: map[string]ChangedTest{ + "bundle/invariant/no_drift": {Added: false, VariantFilters: nil}, + "bundle/invariant/migrate": {Added: false, VariantFilters: nil}, + "bundle/invariant/continue_293": {Added: false, VariantFilters: nil}, + }, + }, + { + name: "root script.prepare re-enables every test dir", + diff: "M\tacceptance/script.prepare", + want: map[string]ChangedTest{ + "bundle/resources": {Added: false, VariantFilters: nil}, + "bundle/resources/jobs": {Added: false, VariantFilters: nil}, + "bundle/invariant/no_drift": {Added: false, VariantFilters: nil}, + "bundle/invariant/migrate": {Added: false, VariantFilters: nil}, + "bundle/invariant/continue_293": {Added: false, VariantFilters: nil}, + "cmd/version": {Added: false, VariantFilters: nil}, + }, + }, + { + name: "test.toml directly in a test dir re-enables only that dir", + diff: "M\tacceptance/bundle/resources/jobs/test.toml", + want: map[string]ChangedTest{ + "bundle/resources/jobs": {Added: false, VariantFilters: nil}, + }, + }, + { + name: "parent shared file preserves an added descendant", + diff: "A\tacceptance/bundle/resources/jobs/script\nM\tacceptance/bundle/resources/test.toml", + want: map[string]ChangedTest{ + "bundle/resources": {Added: false, VariantFilters: nil}, + "bundle/resources/jobs": {Added: true, VariantFilters: nil}, + }, + }, + { + name: "a bin helper re-enables every test dir", + diff: "M\tacceptance/bin/print_requests.py", + want: map[string]ChangedTest{ + "bundle/resources": {Added: false, VariantFilters: nil}, + "bundle/resources/jobs": {Added: false, VariantFilters: nil}, + "bundle/invariant/no_drift": {Added: false, VariantFilters: nil}, + "bundle/invariant/migrate": {Added: false, VariantFilters: nil}, + "bundle/invariant/continue_293": {Added: false, VariantFilters: nil}, + "cmd/version": {Added: false, VariantFilters: nil}, + }, + }, + { + name: "a shared fixture in a non-test dir re-enables its subtree", + diff: "M\tacceptance/bundle/invariant/_script", + want: map[string]ChangedTest{ + "bundle/invariant/no_drift": {Added: false, VariantFilters: nil}, + "bundle/invariant/migrate": {Added: false, VariantFilters: nil}, + "bundle/invariant/continue_293": {Added: false, VariantFilters: nil}, + }, + }, + { + name: "a stray file at the acceptance root is ignored", + diff: "M\tacceptance/README.md", + want: map[string]ChangedTest{}, + }, + { + name: "deleted script of a removed test dir is ignored (dir no longer in testDirs)", + diff: "D\tacceptance/bundle/removed_test/script", + want: map[string]ChangedTest{}, + }, + { + name: "deleted invariant config is ignored", + diff: "D\tacceptance/bundle/invariant/configs/job.yml.tmpl", + want: map[string]ChangedTest{}, + }, + { + name: "deleted config alongside a modified config keeps only the modified filter", + diff: "M\tacceptance/bundle/invariant/configs/job.yml.tmpl\nD\tacceptance/bundle/invariant/configs/job_with_permissions.yml.tmpl", + want: map[string]ChangedTest{ + "bundle/invariant/no_drift": {Added: false, VariantFilters: []string{"INPUT_CONFIG=job.yml.tmpl"}}, + "bundle/invariant/migrate": {Added: false, VariantFilters: []string{"INPUT_CONFIG=job.yml.tmpl"}}, + "bundle/invariant/continue_293": {Added: false, VariantFilters: []string{"INPUT_CONFIG=job.yml.tmpl"}}, + }, + }, + { + name: "changed invariant config re-enables all invariant subdirs with INPUT_CONFIG filter", + diff: "M\tacceptance/bundle/invariant/configs/job.yml.tmpl", + want: map[string]ChangedTest{ + "bundle/invariant/no_drift": {Added: false, VariantFilters: []string{"INPUT_CONFIG=job.yml.tmpl"}}, + "bundle/invariant/migrate": {Added: false, VariantFilters: []string{"INPUT_CONFIG=job.yml.tmpl"}}, + "bundle/invariant/continue_293": {Added: false, VariantFilters: []string{"INPUT_CONFIG=job.yml.tmpl"}}, + }, + }, + { + name: "invariant config init.sh strips suffix to base config name", + diff: "M\tacceptance/bundle/invariant/configs/job.yml.tmpl-init.sh", + want: map[string]ChangedTest{ + "bundle/invariant/no_drift": {Added: false, VariantFilters: []string{"INPUT_CONFIG=job.yml.tmpl"}}, + "bundle/invariant/migrate": {Added: false, VariantFilters: []string{"INPUT_CONFIG=job.yml.tmpl"}}, + "bundle/invariant/continue_293": {Added: false, VariantFilters: []string{"INPUT_CONFIG=job.yml.tmpl"}}, + }, + }, + { + name: "direct change to an invariant subdir overrides config-scoped filter with all-variants", + diff: "M\tacceptance/bundle/invariant/configs/job.yml.tmpl\nM\tacceptance/bundle/invariant/no_drift/script", + want: map[string]ChangedTest{ + "bundle/invariant/no_drift": {Added: false, VariantFilters: nil}, + "bundle/invariant/migrate": {Added: false, VariantFilters: []string{"INPUT_CONFIG=job.yml.tmpl"}}, + "bundle/invariant/continue_293": {Added: false, VariantFilters: []string{"INPUT_CONFIG=job.yml.tmpl"}}, + }, + }, + { + name: "added invariant subdir overrides config-scoped filter and stays added", + diff: "M\tacceptance/bundle/invariant/configs/job.yml.tmpl\nA\tacceptance/bundle/invariant/no_drift/script", + want: map[string]ChangedTest{ + "bundle/invariant/no_drift": {Added: true, VariantFilters: nil}, + "bundle/invariant/migrate": {Added: false, VariantFilters: []string{"INPUT_CONFIG=job.yml.tmpl"}}, + "bundle/invariant/continue_293": {Added: false, VariantFilters: []string{"INPUT_CONFIG=job.yml.tmpl"}}, + }, + }, + { + name: "two changed invariant configs accumulate filters", + diff: "M\tacceptance/bundle/invariant/configs/job.yml.tmpl\nM\tacceptance/bundle/invariant/configs/pipeline.yml.tmpl", + want: map[string]ChangedTest{ + "bundle/invariant/no_drift": {Added: false, VariantFilters: []string{"INPUT_CONFIG=job.yml.tmpl", "INPUT_CONFIG=pipeline.yml.tmpl"}}, + "bundle/invariant/migrate": {Added: false, VariantFilters: []string{"INPUT_CONFIG=job.yml.tmpl", "INPUT_CONFIG=pipeline.yml.tmpl"}}, + "bundle/invariant/continue_293": {Added: false, VariantFilters: []string{"INPUT_CONFIG=job.yml.tmpl", "INPUT_CONFIG=pipeline.yml.tmpl"}}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := ParseChangedTests(tt.diff, testDirsFixture()) + assert.Equal(t, tt.want, got) + }) + } +} diff --git a/acceptance/skiplocal_test.go b/acceptance/skiplocal_test.go index c72f7c39a3f..90bfbe8decd 100644 --- a/acceptance/skiplocal_test.go +++ b/acceptance/skiplocal_test.go @@ -1,10 +1,9 @@ package acceptance_test import ( - "os/exec" - "path/filepath" "slices" - "strings" + + "github.com/databricks/cli/acceptance/internal" ) // DATABRICKS_TEST_SKIPLOCAL controls skipping of Local acceptance tests. @@ -21,98 +20,29 @@ const ( // maxChangedLocalTests caps how many changed tests SkipLocalWithChanged re-enables, // keeping the cloud run bounded. Added tests are preferred over modified ones. maxChangedLocalTests = 50 - - invariantConfigsPrefix = "acceptance/bundle/invariant/configs/" - invariantDirPrefix = "bundle/invariant/" ) -// testDirForFile maps a repo-relative changed file (e.g. acceptance/bundle/foo/script) -// to its owning test dir relative to acceptance/ (e.g. bundle/foo), or "" if the file -// is outside acceptance/ or not under any known test dir. -func testDirForFile(repoRelPath string, testDirs map[string]bool) string { - parts := strings.Split(filepath.ToSlash(repoRelPath), "/") - if len(parts) < 2 || parts[0] != "acceptance" { - return "" - } - // Longest ancestor first so nested tests map to the innermost test dir. - for depth := len(parts); depth > 1; depth-- { - candidate := strings.Join(parts[1:depth], "/") - if testDirs[candidate] { - return candidate - } - } - return "" -} - // selectChangedLocalTests returns a map of test dir → extra env filters for // re-enabling under SkipLocalWithChanged. A nil filter slice means all variants // of that dir run; a non-nil slice restricts to variants matching those filters // (applied by the caller via checkEnvFilters in the variant loop). // Added dirs come before modified ones; the total is capped at maxChangedLocalTests. // -// A changed invariant config (acceptance/bundle/invariant/configs/*.yml.tmpl) -// maps to all invariant subdirs with an INPUT_CONFIG= filter, so touching -// job.yml.tmpl re-enables all subdirs but only for their job.yml.tmpl variants. -// -// --merge-base diffs the working tree against the merge base of HEAD and -// origin/main. This covers committed, staged, and unstaged changes alike — -// the working tree reflects all three. Untracked files (not yet git-added) -// are not visible to git diff and will not be re-enabled until staged or -// committed. The three-dot form origin/main...HEAD only covers committed -// changes and misses unstaged edits, which breaks the "touch a config, run -// the test" local dev workflow (same reason lintdiff.py uses --merge-base). +// Detection (which dirs the branch's diff against origin/main affects, and how) +// lives in internal.ChangedTests; this wrapper applies the skiplocal-specific +// ordering and cap. On error (e.g. origin/main unreachable) it returns nil, so a +// local run without origin/main degrades to skipping all Local tests. func selectChangedLocalTests(testDirs map[string]bool) map[string][]string { - out, _ := exec.Command("git", "diff", "--name-status", "--merge-base", "-M", "origin/main").Output() - diff := strings.TrimSpace(string(out)) - - // result accumulates dirs with their filters; added tracks brand-new dirs. - // nil filter slice = all variants run; non-nil = restricted to those filters. - result := map[string][]string{} - added := map[string]bool{} - - for line := range strings.SplitSeq(diff, "\n") { - fields := strings.Split(line, "\t") - if len(fields) < 2 { - continue - } - status := fields[0] - path := fields[len(fields)-1] - - // A changed invariant config re-enables all invariant subdirs with an - // INPUT_CONFIG filter, unless a subdir was already unlocked by a non-config change. - if strings.HasPrefix(path, invariantConfigsPrefix) { - configName := path[len(invariantConfigsPrefix):] - // Strip -init.sh / -cleanup.sh suffixes to get the base config name. - if i := strings.Index(configName, "-"); i > 0 && strings.HasSuffix(configName, ".sh") { - configName = configName[:i] - } - if strings.HasSuffix(configName, ".yml.tmpl") { - for dir := range testDirs { - if strings.HasPrefix(dir, invariantDirPrefix) { - if existing, ok := result[dir]; !ok || existing != nil { - result[dir] = append(result[dir], "INPUT_CONFIG="+configName) - } - } - } - } - continue - } - - dir := testDirForFile(path, testDirs) - if dir == "" { - continue - } - result[dir] = nil // nil = all variants; overrides any prior config-scoped filter - // A script file with status A means the test dir is brand new. - // Renames (R) land here as the destination path but are not "added". - if status == "A" && strings.HasSuffix(path, "/script") { - added[dir] = true - } + // Empty headRef: diff against the working tree so uncommitted local edits + // re-enable their tests without needing a commit. + changed, err := internal.ChangedTests("origin/main", "", testDirs) + if err != nil { + return nil } var addedDirs, modifiedDirs []string - for dir := range result { - if added[dir] { + for dir, ct := range changed { + if ct.Added { addedDirs = append(addedDirs, dir) } else { modifiedDirs = append(modifiedDirs, dir) @@ -126,9 +56,9 @@ func selectChangedLocalTests(testDirs map[string]bool) map[string][]string { selected = selected[:maxChangedLocalTests] } - out2 := make(map[string][]string, len(selected)) + out := make(map[string][]string, len(selected)) for _, dir := range selected { - out2[dir] = result[dir] + out[dir] = changed[dir].VariantFilters } - return out2 + return out }