diff --git a/Makefile b/Makefile index e138b08bce..5714483a36 100644 --- 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 index 695027f0a2..e293388dc6 100644 --- a/acceptance/acceptance_test.go +++ b/acceptance/acceptance_test.go @@ -20,6 +20,7 @@ import ( "sort" "strconv" "strings" + "sync" "testing" "time" "unicode/utf8" @@ -293,6 +294,16 @@ 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 @@ -300,7 +311,16 @@ func testAccept(t *testing.T, inprocessMode bool, singleTest string) int { 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 index d560f1de04..4cfe03e9f9 100644 --- a/acceptance/bundle/resources/permissions/out.test.toml +++ b/acceptance/bundle/resources/permissions/out.test.toml @@ -1,5 +1,6 @@ Local = true Cloud = false +Phase = 1 [EnvMatrix] DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"] diff --git a/acceptance/bundle/resources/permissions/test.toml b/acceptance/bundle/resources/permissions/test.toml index 4a1a3bb8f5..7cb7c1def5 100644 --- 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 index c820fbee96..3c8bb15e9f 100644 --- a/acceptance/bundle/templates/default-python/classic/out.test.toml +++ b/acceptance/bundle/templates/default-python/classic/out.test.toml @@ -1,5 +1,6 @@ Local = true Cloud = false +Phase = 1 [EnvMatrix] DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"] diff --git a/acceptance/bundle/templates/default-python/classic/test.toml b/acceptance/bundle/templates/default-python/classic/test.toml index 0b1b996a83..68f298aca3 100644 --- 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 index d560f1de04..4cfe03e9f9 100644 --- a/acceptance/bundle/templates/default-python/serverless-customcatalog/out.test.toml +++ b/acceptance/bundle/templates/default-python/serverless-customcatalog/out.test.toml @@ -1,5 +1,6 @@ Local = true Cloud = false +Phase = 1 [EnvMatrix] DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"] diff --git a/acceptance/bundle/templates/default-python/serverless-customcatalog/test.toml b/acceptance/bundle/templates/default-python/serverless-customcatalog/test.toml index 4029057bea..a9b8e0859e 100644 --- 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 index d560f1de04..4cfe03e9f9 100644 --- a/acceptance/bundle/user_agent/out.test.toml +++ b/acceptance/bundle/user_agent/out.test.toml @@ -1,5 +1,6 @@ Local = true Cloud = false +Phase = 1 [EnvMatrix] DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"] diff --git a/acceptance/bundle/user_agent/test.toml b/acceptance/bundle/user_agent/test.toml index 9295fedc55..a9f876b6b7 100644 --- 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 index 376047c9ca..23981e7093 100644 --- 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 index c2701337f5..d21c4bcb6c 100644 --- 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 index 9ab0dc18cb..a1f30c841c 100644 --- 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, }