acc: Introduce phases; speed up "test-update" 2x. (#4795)

denik · claude · web-flow · commit 2acf955cf728 · 2026-03-20T12:28:07.000Z
## Changes
- Introduce Phase field in test.toml that allows specifying order in
which tests are run. All tests in Phase=0 are run before tests in
Phase=1
- Introduce 'inherit' tag on acceptance tests config fields that
disables default inheritance of test configs. This is needed as Phase=1
is frequently applied on parent test.
- Simplify acceptance test runner. Previously if there was a single
entry for a given EnvMatrix, it would be omitted from the test name,
e.g. TestAccept/bla/DATABRICKS_BUNDLE_ENGINE=direct would become
TestAccept/bla if "direct" was the only option. Now we keep the full
name (helps to see what variant is being run).

## 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:
```
% git grep -l 'Local = true'  '**/out.test.toml' | find_out_files.py | wc -l
    2420
% git grep -l 'Local = true'  '**/out.test.toml' | find_out_files.py | xargs rm
```

---------

Co-authored-by: Claude Sonnet 4.6 &lt;noreply@anthropic.com&gt;
diff --git a/Makefile b/Makefile
@@ -98,8 +98,6 @@ test-slow-acc:
 .PHONY: test-update
 test-update:
 	-go test ./acceptance -run '^TestAccept$$' -update -timeout=${LOCAL_TIMEOUT}
-	@# at the moment second pass is required because some tests show diff against output of another test for easier review
-	-go test ./acceptance -run '^TestAccept$$' -update -timeout=${LOCAL_TIMEOUT}
 
 # Updates acceptance test output for template tests only
 .PHONY: test-update-templates
diff --git a/acceptance/acceptance_test.go b/acceptance/acceptance_test.go
@@ -20,6 +20,7 @@ import (
 	"sort"
 	"strconv"
 	"strings"
+	"sync"
 	"testing"
 	"time"
 	"unicode/utf8"
@@ -293,14 +294,33 @@ func testAccept(t *testing.T, inprocessMode bool, singleTest string) int {
 
 	envFilters := getEnvFilters(t)
 
+	// Phases are only needed in update mode, where phase 0 tests regenerate
+	// output files that phase 1 tests read via $TESTDIR. In normal runs,
+	// those files are already committed and stable.
+	usePhases := testdiff.OverwriteMode
+	var phase0wg sync.WaitGroup
+	phase1Gate := make(chan struct{})
+	if !usePhases {
+		close(phase1Gate)
+	}
+
 	for _, dir := range testDirs {
 		totalDirs += 1
 
 		t.Run(dir, func(t *testing.T) {
 			selectedDirs += 1
 
 			config, configPath := internal.LoadConfig(t, dir)
-			skipReason := getSkipReason(&config, configPath)
+			err := validateTestPhase(config.Phase)
+			if err != nil {
+				t.Fatalf("Invalid config %s: %s", configPath, err)
+			}
+
+			// Apply default: CloudSlow implies Cloud. Do this before generating
+			// the materialized config so the implication is visible in out.test.toml.
+			if isTruePtr(config.CloudSlow) {
+				config.Cloud = config.CloudSlow
+			}
 
 			// Generate materialized config for this test.
 			// We do this before skipping the test, so the configs are generated for all tests.
@@ -313,21 +333,32 @@ func testAccept(t *testing.T, inprocessMode bool, singleTest string) int {
 				t.Skip("Skipping test execution (only regenerating out.test.toml)")
 			}
 
+			skipReason := getSkipReason(&config, configPath)
 			if skipReason != "" {
 				skippedDirs += 1
 				t.Skip(skipReason)
 			}
 
 			runParallel := !inprocessMode
-
 			if benchmarkMode && strings.Contains(dir, "benchmark") {
 				runParallel = false
 			}
 
+			// t.Run blocks until t.Parallel() is called, so Add must happen before t.Parallel().
+			// This ensures all phase0 adds are visible before the wait goroutine starts.
+			if usePhases && config.Phase == 0 {
+				phase0wg.Add(1)
+				t.Cleanup(phase0wg.Done)
+			}
+
 			if runParallel {
 				t.Parallel()
 			}
 
+			if config.Phase != 0 {
+				<-phase1Gate
+			}
+
 			// Build extra vars for exclusion matching (config state as env vars)
 			var extraVars []string
 			if cloudEnv != "" {
@@ -336,26 +367,25 @@ func testAccept(t *testing.T, inprocessMode bool, singleTest string) int {
 
 			expanded := internal.ExpandEnvMatrix(config.EnvMatrix, config.EnvMatrixExclude, extraVars)
 
-			if len(expanded) == 1 {
-				// env vars aren't part of the test case name, so log them for debugging
-				if len(expanded[0]) > 0 {
-					t.Logf("Running test with env %v", expanded[0])
-				}
-				runTest(t, dir, 0, coverDir, repls.Clone(), config, expanded[0], envFilters)
-			} else {
-				for ind, envset := range expanded {
-					envname := strings.Join(envset, "/")
-					t.Run(envname, func(t *testing.T) {
-						if runParallel {
-							t.Parallel()
-						}
-						runTest(t, dir, ind, coverDir, repls.Clone(), config, envset, envFilters)
-					})
-				}
+			for ind, envset := range expanded {
+				envname := strings.Join(envset, "/")
+				t.Run(envname, func(t *testing.T) {
+					if runParallel {
+						t.Parallel()
+					}
+					runTest(t, dir, ind, coverDir, repls.Clone(), config, envset, envFilters)
+				})
 			}
 		})
 	}
 
+	if usePhases {
+		go func() {
+			phase0wg.Wait()
+			close(phase1Gate)
+		}()
+	}
+
 	t.Logf("Summary (dirs): %d/%d/%d run/selected/total, %d skipped", selectedDirs-skippedDirs, selectedDirs, totalDirs, skippedDirs)
 
 	return selectedDirs - skippedDirs
@@ -405,13 +435,16 @@ func getTests(t *testing.T) []string {
 	return testDirs
 }
 
-// Return a reason to skip the test. Empty string means "don't skip".
-func getSkipReason(config *internal.TestConfig, configPath string) string {
-	// Apply default first, so that it's visible in out.test.toml
-	if isTruePtr(config.CloudSlow) {
-		config.Cloud = config.CloudSlow
+func validateTestPhase(phase int) error {
+	if phase == 0 || phase == 1 {
+		return nil
 	}
 
+	return fmt.Errorf("Phase must be 0 or 1, got %d", phase)
+}
+
+// Return a reason to skip the test. Empty string means "don't skip".
+func getSkipReason(config *internal.TestConfig, configPath string) string {
 	if os.Getenv("DATABRICKS_TEST_SKIPLOCAL") != "" && isTruePtr(config.Local) {
 		return "Disabled via DATABRICKS_TEST_SKIPLOCAL environment variable in " + configPath
 	}
diff --git a/acceptance/bundle/resources/permissions/out.test.toml b/acceptance/bundle/resources/permissions/out.test.toml
diff --git a/acceptance/bundle/resources/permissions/test.toml b/acceptance/bundle/resources/permissions/test.toml
@@ -1,2 +1,3 @@
+Phase = 1
 RecordRequests = true
 Ignore = ['.databricks']
diff --git a/acceptance/bundle/templates/default-python/classic/out.test.toml b/acceptance/bundle/templates/default-python/classic/out.test.toml
diff --git a/acceptance/bundle/templates/default-python/classic/test.toml b/acceptance/bundle/templates/default-python/classic/test.toml
@@ -1,2 +1,3 @@
+Phase = 1
 RecordRequests = true
 EnvMatrix.READPLAN = ["", "1"]
diff --git a/acceptance/bundle/templates/default-python/serverless-customcatalog/out.test.toml b/acceptance/bundle/templates/default-python/serverless-customcatalog/out.test.toml
diff --git a/acceptance/bundle/templates/default-python/serverless-customcatalog/test.toml b/acceptance/bundle/templates/default-python/serverless-customcatalog/test.toml
@@ -1,3 +1,5 @@
+Phase = 1
+
 [[Server]]
 Pattern = "GET /api/2.1/unity-catalog/current-metastore-assignment"
 Response.Body = '{"default_catalog_name": "customcatalog"}'
diff --git a/acceptance/bundle/user_agent/out.test.toml b/acceptance/bundle/user_agent/out.test.toml
diff --git a/acceptance/bundle/user_agent/test.toml b/acceptance/bundle/user_agent/test.toml
@@ -1,3 +1,4 @@
+Phase = 1
 RecordRequests = true
 Local = true
 IncludeRequestHeaders = ["User-Agent"]
diff --git a/acceptance/internal/config.go b/acceptance/internal/config.go
@@ -24,6 +24,12 @@ type TestConfig struct {
 	// Place to describe what's wrong with this test. Does not affect how the test is run.
 	Badness *string
 
+	// Execution phase for this test. Phase 1 tests run after all phase 0 tests complete,
+	// which is useful when a test depends on the output of another tests.
+	// Some tests run diff against sister test output to highlight the differences.
+	// Some tests summarize output of many child tests.
+	Phase int `inherit:"false"`
+
 	// Which OSes the test is enabled on. Each string is compared against runtime.GOOS.
 	// If absent, default to true.
 	GOOS map[string]bool
@@ -194,9 +200,19 @@ func LoadConfig(t *testing.T, dir string) (TestConfig, string) {
 	}
 
 	result := DoLoadConfig(t, configs[0])
+	leafConfigPath := filepath.Join(dir, configFilename)
+	leafConfig := TestConfig{}
+	hasLeafConfig := configs[0] == leafConfigPath
+	if hasLeafConfig {
+		leafConfig = result
+	}
 
 	for _, cfgName := range configs[1:] {
 		cfg := DoLoadConfig(t, cfgName)
+		if cfgName == leafConfigPath {
+			leafConfig = cfg
+			hasLeafConfig = true
+		}
 		err := mergo.Merge(
 			&result,
 			cfg,
@@ -210,14 +226,17 @@ func LoadConfig(t *testing.T, dir string) (TestConfig, string) {
 		}
 	}
 
+	restoreNonInheritable(&result, leafConfig, hasLeafConfig)
+
 	// Always ignore .cache directory (used by local cache)
 	result.Ignore = append(result.Ignore, ".cache")
 	result.CompiledIgnoreObject = ignore.CompileIgnoreLines(result.Ignore...)
 
 	// Validate incompatible configuration combinations
-	validateConfig(t, result, strings.Join(configs, ", "))
+	configDesc := filepath.ToSlash(strings.Join(configs, ", "))
+	validateConfig(t, result, configDesc)
 
-	return result, strings.Join(configs, ", ")
+	return result, configDesc
 }
 
 // validateConfig checks for incompatible configuration combinations.
@@ -256,6 +275,24 @@ func DoLoadConfig(t *testing.T, path string) TestConfig {
 	return config
 }
 
+// restoreNonInheritable resets fields tagged with `inherit:"false"` to their leaf config values.
+// If there is no leaf config, those fields are reset to their zero value.
+func restoreNonInheritable(result *TestConfig, leafConfig TestConfig, hasLeafConfig bool) {
+	typ := reflect.TypeFor[TestConfig]()
+	val := reflect.ValueOf(result).Elem()
+	leafVal := reflect.ValueOf(leafConfig)
+	for i := range typ.NumField() {
+		field := typ.Field(i)
+		if field.Tag.Get("inherit") == "false" {
+			if hasLeafConfig {
+				val.Field(i).Set(leafVal.Field(i))
+			} else {
+				val.Field(i).SetZero()
+			}
+		}
+	}
+}
+
 // mapTransformer is a mergo transformer that merges two maps
 // by overriding values in the destination map with values from the source map.
 //
diff --git a/acceptance/internal/config_test.go b/acceptance/internal/config_test.go
@@ -1,9 +1,12 @@
 package internal
 
 import (
+	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestExpandEnvMatrix(t *testing.T) {
@@ -197,3 +200,61 @@ func TestExpandEnvMatrix(t *testing.T) {
 		})
 	}
 }
+
+func TestLoadConfigPhaseIsNotInherited(t *testing.T) {
+	tests := []struct {
+		name       string
+		files      map[string]string
+		dir        string
+		wantPhase  int
+		wantConfig string
+	}{
+		{
+			name: "missing leaf config defaults to zero",
+			files: map[string]string{
+				"test.toml": "Phase = 3\n",
+			},
+			dir:        "suite/test",
+			wantPhase:  0,
+			wantConfig: "test.toml",
+		},
+		{
+			name: "leaf config without phase resets inherited value",
+			files: map[string]string{
+				"test.toml":            "Phase = 3\n",
+				"suite/test/test.toml": "Local = true\n",
+			},
+			dir:        "suite/test",
+			wantPhase:  0,
+			wantConfig: "test.toml, suite/test/test.toml",
+		},
+		{
+			name: "leaf phase wins",
+			files: map[string]string{
+				"test.toml":            "Phase = 3\n",
+				"suite/test/test.toml": "Local = true\nPhase = 1\n",
+			},
+			dir:        "suite/test",
+			wantPhase:  1,
+			wantConfig: "test.toml, suite/test/test.toml",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			root := t.TempDir()
+			t.Chdir(root)
+
+			for path, contents := range tt.files {
+				absPath := filepath.Join(root, filepath.FromSlash(path))
+				require.NoError(t, os.MkdirAll(filepath.Dir(absPath), 0o755))
+				require.NoError(t, os.WriteFile(absPath, []byte(contents), 0o644))
+			}
+
+			config, configPath := LoadConfig(t, tt.dir)
+
+			assert.Equal(t, tt.wantPhase, config.Phase)
+			assert.Equal(t, tt.wantConfig, configPath)
+		})
+	}
+}
diff --git a/acceptance/internal/materialized_config.go b/acceptance/internal/materialized_config.go
@@ -18,12 +18,18 @@ type MaterializedConfig struct {
 	RequiresCluster      *bool               `toml:"RequiresCluster,omitempty"`
 	RequiresWarehouse    *bool               `toml:"RequiresWarehouse,omitempty"`
 	RunsOnDbr            *bool               `toml:"RunsOnDbr,omitempty"`
+	Phase                *int                `toml:"Phase,omitempty"`
 	EnvMatrix            map[string][]string `toml:"EnvMatrix,omitempty"`
 }
 
 // GenerateMaterializedConfig creates a TOML representation of the configuration fields
 // that determine where and how a test is executed
 func GenerateMaterializedConfig(config TestConfig) (string, error) {
+	var phase *int
+	if config.Phase != 0 {
+		phase = &config.Phase
+	}
+
 	materialized := MaterializedConfig{
 		GOOS:                 config.GOOS,
 		CloudEnvs:            config.CloudEnvs,
@@ -34,6 +40,7 @@ func GenerateMaterializedConfig(config TestConfig) (string, error) {
 		RequiresCluster:      config.RequiresCluster,
 		RequiresWarehouse:    config.RequiresWarehouse,
 		RunsOnDbr:            config.RunsOnDbr,
+		Phase:                phase,
 		EnvMatrix:            config.EnvMatrix,
 	}
 

Original file line number	Diff line number	Diff line change
`@@ -1,2 +1,3 @@`
	`1`	`+Phase = 1`
`1`	`2`	`RecordRequests = true`
`2`	`3`	`Ignore = ['.databricks']`
Original file line number	Diff line number	Diff line change
`@@ -1,3 +1,5 @@`
	`1`	`+Phase = 1`
	`2`	`+`
`1`	`3`	`[[Server]]`
`2`	`4`	`Pattern = "GET /api/2.1/unity-catalog/current-metastore-assignment"`
`3`	`5`	`Response.Body = '{"default_catalog_name": "customcatalog"}'`
Original file line number	Diff line number	Diff line change
`@@ -1,3 +1,4 @@`
	`1`	`+Phase = 1`
`1`	`2`	`RecordRequests = true`
`2`	`3`	`Local = true`
`3`	`4`	`IncludeRequestHeaders = ["User-Agent"]`