acc: Avoid selecting entire invariant suites when adding exclusions

acceptance/acceptance_test.go

@@ -641,25 +641,28 @@ func testAccept(t *testing.T, inprocessMode bool, selectedTests []string, skipTo
 				}
 				runTest(t, dir, 0, coverDir, repls.Clone(), config, nil, envFilters, sandboxProxyURL)
 			} else {
+				matched := false
 				for ind, envset := range expanded {
+					// Under selection.EnvVar, an invariant dir re-enabled by a
+					// specific config change runs only its matching variants.
+					if selectChanged && !selection.MatchesFilters(envset, changedTests[dir]) {
+						continue
+					}
+					matched = true
 					envname := strings.Join(envset, "/")
 					t.Run(envname, func(t *testing.T) {
 						if runParallel {
 							t.Parallel()
 						}
-						// Under selection.EnvVar, an invariant dir re-enabled by a
-						// specific config change runs only its matching variants.
-						if selectChanged {
-							if variantFilters := changedTests[dir]; variantFilters != nil {
-								checkEnvFilters(t, envset, variantFilters)
-							}
-						}
 						if reason := subset.skipReason(dir, envset); reason != "" {
 							t.Skip(reason)
 						}
 						runTest(t, dir, ind, coverDir, repls.Clone(), config, envset, envFilters, sandboxProxyURL)
 					})
 				}
+				if selectChanged && !matched {
+					t.Skip("No environment matrix variants match changed-test selection")
+				}
 			}
 		})
 	}

acceptance/internal/selection/selection.go

@@ -12,6 +12,7 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"reflect"
 	"slices"
 	"strconv"
 	"strings"
@@ -225,7 +226,119 @@ func FromGit(root string, testDirs map[string]bool, limit int) (Result, error) {
 		return Result{}, fmt.Errorf("git diff --merge-base origin/main failed: %w", err)
 	}
 
-	return FromDiff(root, strings.TrimSpace(string(out)), testDirs, limit), nil
+	diff, err := withoutAddedExclusions(root, strings.TrimSpace(string(out)))
+	if err != nil {
+		return Result{}, err
+	}
+	return FromDiff(root, diff, testDirs, limit), nil
+}
+
+// withoutAddedExclusions ignores invariant configs that only disable more variants:
+// none of the remaining variants changed, so running the whole directory adds no coverage.
+func withoutAddedExclusions(root, diff string) (string, error) {
+	lines := strings.Split(diff, "\n")
+	var base string
+	for i, line := range lines {
+		fields := strings.Split(line, "\t")
+		if len(fields) != 2 || fields[0] != "M" {
+			continue
+		}
+		path := fields[1]
+		if !strings.HasPrefix(path, acceptanceDirPrefix+invariantDirPrefix) || filepath.Base(path) != internal.ConfigFilename {
+			continue
+		}
+		if base == "" {
+			out, err := exec.Command("git", "merge-base", "HEAD", "origin/main").CombinedOutput()
+			if err != nil {
+				return "", fmt.Errorf("finding selection merge base: %w: %s", err, strings.TrimSpace(string(out)))
+			}
+			base = strings.TrimSpace(string(out))
+		}
+		before, err := exec.Command("git", "show", base+":"+path).CombinedOutput()
+		if err != nil {
+			return "", fmt.Errorf("reading %s at %s: %w: %s", path, base, err, strings.TrimSpace(string(before)))
+		}
+		after, err := os.ReadFile(filepath.Join(root, strings.TrimPrefix(path, acceptanceDirPrefix)))
+		if err != nil {
+			return "", fmt.Errorf("reading %s: %w", path, err)
+		}
+		added := addedExclusions(before, after)
+		if len(added) == 0 {
+			continue
+		}
+		inherited, err := inheritedExclusions(base, path)
+		if err != nil {
+			return "", err
+		}
+		// A new leaf rule can override an inherited rule and re-enable its old variants.
+		shadowsInherited := false
+		for name := range added {
+			if inherited[name] {
+				shadowsInherited = true
+				break
+			}
+		}
+		if !shadowsInherited {
+			lines[i] = ""
+		}
+	}
+	return strings.Join(lines, "\n"), nil
+}
+
+// addedExclusions returns the new rules if adding exclusions is the only config change.
+func addedExclusions(before, after []byte) map[string][]string {
+	var oldConfig, newConfig internal.TestConfig
+	if metadata, err := toml.Decode(string(before), &oldConfig); err != nil || len(metadata.Undecoded()) > 0 {
+		return nil
+	}
+	if metadata, err := toml.Decode(string(after), &newConfig); err != nil || len(metadata.Undecoded()) > 0 {
+		return nil
+	}
+	oldRules, newRules := oldConfig.EnvMatrixExclude, newConfig.EnvMatrixExclude
+	if len(newRules) <= len(oldRules) {
+		return nil
+	}
+	for name, rule := range oldRules {
+		if !reflect.DeepEqual(rule, newRules[name]) {
+			return nil
+		}
+	}
+	oldConfig.EnvMatrixExclude, newConfig.EnvMatrixExclude = nil, nil
+	if !reflect.DeepEqual(oldConfig, newConfig) {
+		return nil
+	}
+	for name := range oldRules {
+		delete(newRules, name)
+	}
+	return newRules
+}
+
+// inheritedExclusions returns the ancestor exclusion rule names at base.
+func inheritedExclusions(base, path string) (map[string]bool, error) {
+	args := []string{"ls-tree", "-r", "--full-tree", "--name-only", base, "--"}
+	for dir := filepath.Dir(filepath.Dir(path)); filepath.ToSlash(dir)+"/" != acceptanceDirPrefix; dir = filepath.Dir(dir) {
+		args = append(args, filepath.ToSlash(filepath.Join(dir, internal.ConfigFilename)))
+	}
+	args = append(args, acceptanceDirPrefix+internal.ConfigFilename)
+	out, err := exec.Command("git", args...).CombinedOutput()
+	if err != nil {
+		return nil, fmt.Errorf("listing inherited configs at %s: %w: %s", base, err, strings.TrimSpace(string(out)))
+	}
+	rules := map[string]bool{}
+	for path := range strings.FieldsSeq(string(out)) {
+		contents, err := exec.Command("git", "show", base+":"+path).CombinedOutput()
+		if err != nil {
+			return nil, fmt.Errorf("reading %s at %s: %w: %s", path, base, err, strings.TrimSpace(string(contents)))
+		}
+		var config internal.TestConfig
+		if _, err := toml.Decode(string(contents), &config); err != nil {
+			return nil, fmt.Errorf("parsing %s at %s: %w", path, base, err)
+		}
+		for name := range config.EnvMatrixExclude {
+			rules[name] = true
+		}
+	}
+	return rules, nil
 }
 
 // testDirForFile maps a repo-relative changed file (e.g. acceptance/bundle/foo/script)

acceptance/internal/selection/selection_test.go

@@ -2,6 +2,7 @@ package selection_test
 
 import (
 	"os"
+	"os/exec"
 	"path/filepath"
 	"strings"
 	"testing"
@@ -345,3 +346,81 @@ func TestFromDiffTiesAreAlphabetical(t *testing.T) {
 		assert.Equal(t, expected, names)
 	}
 }
+
+func TestFromGitExclusionChanges(t *testing.T) {
+	const before = "Cloud = false\nEnvMatrixExclude.old = [\"INPUT_CONFIG=old.yml.tmpl\"]\n"
+	const added = before + "EnvMatrixExclude.new = [\"INPUT_CONFIG=new.yml.tmpl\"]\n"
+	for _, tc := range []struct {
+		name    string
+		before  string
+		after   string
+		parent  string
+		script  bool
+		stage   bool
+		commit  bool
+		wantDir bool
+	}{
+		{name: "added exclusion", before: before, after: added},
+		{name: "first exclusion", before: "Cloud = false\n", after: "Cloud = false\nEnvMatrixExclude.new = [\"INPUT_CONFIG=new.yml.tmpl\"]\n"},
+		{name: "empty config", before: "", after: "EnvMatrixExclude.new = [\"INPUT_CONFIG=new.yml.tmpl\"]\n"},
+		{name: "staged exclusion", before: before, after: added, stage: true},
+		{name: "committed exclusion", before: before, after: added, stage: true, commit: true},
+		{name: "removed exclusion", before: before, after: "Cloud = false\n", wantDir: true},
+		{name: "replaced exclusion", before: before, after: "Cloud = false\nEnvMatrixExclude.old = [\"INPUT_CONFIG=new.yml.tmpl\"]\n", wantDir: true},
+		{name: "other setting changed", before: before, after: strings.Replace(added, "false", "true", 1), wantDir: true},
+		{name: "script changed too", before: before, after: added, script: true, wantDir: true},
+		{name: "invalid config", before: before, after: added + "invalid = [\n", wantDir: true},
+		{name: "invalid rule type", before: before, after: before + "EnvMatrixExclude.new = 1\n", wantDir: true},
+		{name: "unknown setting", before: before, after: added + "Unknown = true\n", wantDir: true},
+		{name: "overrides inherited exclusion", before: before, after: added, parent: "EnvMatrixExclude.new = [\"INPUT_CONFIG=other.yml.tmpl\"]\n", wantDir: true},
+		{name: "distinct inherited exclusion", before: before, after: added, parent: "EnvMatrixExclude.other = [\"INPUT_CONFIG=other.yml.tmpl\"]\n"},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			repo := t.TempDir()
+			t.Chdir(repo)
+			runSelectionGit(t, "init", "--quiet", "--initial-branch=main")
+			root := filepath.Join(repo, "acceptance")
+			dir := "bundle/invariant/jobs"
+			configPath := filepath.Join(root, dir, "test.toml")
+			scriptPath := filepath.Join(root, dir, "script")
+			require.NoError(t, os.MkdirAll(filepath.Dir(configPath), 0o755))
+			require.NoError(t, os.WriteFile(configPath, []byte(tc.before), 0o644))
+			require.NoError(t, os.WriteFile(scriptPath, []byte("echo before\n"), 0o644))
+			if tc.parent != "" {
+				require.NoError(t, os.WriteFile(filepath.Join(root, "bundle/invariant/test.toml"), []byte(tc.parent), 0o644))
+			}
+			runSelectionGit(t, "add", ".")
+			runSelectionGit(t, "commit", "--quiet", "-m", "baseline")
+			runSelectionGit(t, "update-ref", "refs/remotes/origin/main", "HEAD")
+			require.NoError(t, os.WriteFile(configPath, []byte(tc.after), 0o644))
+			if tc.script {
+				require.NoError(t, os.WriteFile(scriptPath, []byte("echo after\n"), 0o644))
+			}
+			if tc.stage {
+				runSelectionGit(t, "add", ".")
+			}
+			if tc.commit {
+				runSelectionGit(t, "commit", "--quiet", "-m", "add exclusion")
+			}
+
+			// Both the inspection command (repo root) and harness (acceptance/) use FromGit.
+			for _, cwd := range []string{repo, root} {
+				t.Chdir(cwd)
+				result, err := selection.FromGit(root, map[string]bool{dir: true}, 10)
+				require.NoError(t, err)
+				want := map[string][]string{}
+				if tc.wantDir {
+					want[dir] = nil
+				}
+				assert.Equal(t, want, result.Tests())
+			}
+		})
+	}
+}
+
+func runSelectionGit(t *testing.T, args ...string) {
+	t.Helper()
+	args = append([]string{"-c", "user.name=Test", "-c", "user.email=test@example.test", "-c", "commit.gpgsign=false"}, args...)
+	out, err := exec.Command("git", args...).CombinedOutput()
+	require.NoError(t, err, "git %v: %s", args, out)
+}

acceptance/selftest/selection_exclusions/fixtures/base.toml

@@ -0,0 +1 @@
+Cloud = false

acceptance/selftest/selection_exclusions/fixtures/changed_script.txt

@@ -0,0 +1 @@
+echo changed test

acceptance/selftest/selection_exclusions/fixtures/excluded.toml

@@ -0,0 +1,2 @@
+Cloud = false
+EnvMatrixExclude.new = ["INPUT_CONFIG=new.yml.tmpl"]

acceptance/selftest/selection_exclusions/fixtures/new.yml.tmpl

@@ -0,0 +1 @@
+# New invariant template.

acceptance/selftest/selection_exclusions/fixtures/new_matrix.toml

@@ -0,0 +1 @@
+EnvMatrix.INPUT_CONFIG = ["old.yml.tmpl", "new.yml.tmpl"]

acceptance/selftest/selection_exclusions/fixtures/old.yml.tmpl

@@ -0,0 +1 @@
+# Existing invariant template.

acceptance/selftest/selection_exclusions/fixtures/old_matrix.toml

@@ -0,0 +1 @@
+EnvMatrix.INPUT_CONFIG = ["old.yml.tmpl"]

acceptance/selftest/selection_exclusions/fixtures/script.txt

@@ -0,0 +1 @@
+echo original test

acceptance/selftest/selection_exclusions/out.test.toml

@@ -0,0 +1 @@
+Cloud = false

acceptance/selftest/selection_exclusions/output.txt

@@ -0,0 +1,26 @@
+
+=== A new template excluded from migration selects only its no_drift variant
+
+>>> cat acceptance/bundle/invariant/migrate/test.toml
+Cloud = false
+EnvMatrixExclude.new = ["INPUT_CONFIG=new.yml.tmpl"]
+
+>>> selection
+Selected 1 changed tests (limit=50, 0 not selected)
+   10  ^bundle$/^invariant$/^no_drift$/^INPUT_CONFIG=new\.yml\.tmpl$
+
+=== Changing the migration script also selects its existing variants
+
+>>> selection
+Selected 2 changed tests (limit=50, 0 not selected)
+   10  ^bundle$/^invariant$/^no_drift$/^INPUT_CONFIG=new\.yml\.tmpl$
+    5  ^bundle$/^invariant$/^migrate$
+
+=== Removing the exclusion selects migration, including the re-enabled variant
+
+>>> cat acceptance/bundle/invariant/migrate/test.toml
+Cloud = false
+
+>>> selection
+Selected 1 changed tests (limit=50, 0 not selected)
+    5  ^bundle$/^invariant$/^migrate$

acceptance/selftest/selection_exclusions/script

@@ -0,0 +1,43 @@
+# Use a small Git repo so selection sees the contents of changed test.toml files.
+# File arguments to $SELECTION only exercise path-based selection.
+mkdir -p repo/acceptance/bundle/invariant/{configs,migrate,no_drift}
+for test in migrate no_drift; do
+    cp fixtures/script.txt "repo/acceptance/bundle/invariant/$test/script"
+    cp fixtures/old_matrix.toml "repo/acceptance/bundle/invariant/$test/out.test.toml"
+done
+cp fixtures/base.toml repo/acceptance/bundle/invariant/migrate/test.toml
+cp fixtures/old.yml.tmpl repo/acceptance/bundle/invariant/configs/
+cd repo
+git init -qb main
+git config user.name Tester
+git config user.email tester@example.test
+git add .
+git commit -qm baseline
+git update-ref refs/remotes/origin/main HEAD
+
+selection() {
+    "$SELECTION" -root acceptance
+}
+
+title "A new template excluded from migration selects only its no_drift variant\n"
+cp ../fixtures/new.yml.tmpl acceptance/bundle/invariant/configs/
+cp ../fixtures/excluded.toml acceptance/bundle/invariant/migrate/test.toml
+cp ../fixtures/new_matrix.toml acceptance/bundle/invariant/no_drift/out.test.toml
+git add .
+trace cat acceptance/bundle/invariant/migrate/test.toml
+trace selection
+
+title "Changing the migration script also selects its existing variants\n"
+cp ../fixtures/changed_script.txt acceptance/bundle/invariant/migrate/script
+trace selection
+
+# Make the added template and exclusion part of the baseline for the next change.
+cp ../fixtures/script.txt acceptance/bundle/invariant/migrate/script
+git commit -qm 'Add template and exclusion'
+git update-ref refs/remotes/origin/main HEAD
+
+title "Removing the exclusion selects migration, including the re-enabled variant\n"
+cp ../fixtures/base.toml acceptance/bundle/invariant/migrate/test.toml
+cp ../fixtures/new_matrix.toml acceptance/bundle/invariant/migrate/out.test.toml
+trace cat acceptance/bundle/invariant/migrate/test.toml
+trace selection

acceptance/selftest/selection_exclusions/test.toml

@@ -0,0 +1 @@
+Ignore = ["repo"]