diff --git a/acceptance/acceptance_test.go b/acceptance/acceptance_test.go index f7509623b81..29f1ccde3a3 100644 --- a/acceptance/acceptance_test.go +++ b/acceptance/acceptance_test.go @@ -1072,13 +1072,14 @@ func runTest(t *testing.T, pathFilter := preparePathFilter(config, customEnv) sortLines := compileSortLines(t, config) + suppressLines := compileSuppressLines(t, config) // output.txt lives in outputDir, not tmpDir, so the bundle sync never uploads it; // compare it from there. Every run produces it, so compare it explicitly rather // than relying on it turning up in the tmpDir scan below. out.requests.txt also // stays in outputDir and is never compared: tests assert on recorded requests // through print_requests.py, not by committing the raw recording. - doComparison(t, repls, sortLines, dir, outputDir, "output.txt", &printedRepls) + doComparison(t, repls, sortLines, suppressLines, dir, outputDir, "output.txt", &printedRepls) // Compare expected outputs for relPath := range outputs { @@ -1090,7 +1091,7 @@ func runTest(t *testing.T, continue } - doComparison(t, repls, sortLines, dir, tmpDir, relPath, &printedRepls) + doComparison(t, repls, sortLines, suppressLines, dir, tmpDir, relPath, &printedRepls) } // Make sure there are not unaccounted for new files @@ -1122,7 +1123,7 @@ func runTest(t *testing.T, if strings.HasPrefix(relPath, "out") { // We have a new file starting with "out" // Show the contents & support overwrite mode for it: - doComparison(t, repls, sortLines, dir, tmpDir, relPath, &printedRepls) + doComparison(t, repls, sortLines, suppressLines, dir, tmpDir, relPath, &printedRepls) } } @@ -1239,7 +1240,25 @@ func compileSortLines(t *testing.T, config internal.TestConfig) []*regexp.Regexp return result } -func doComparison(t *testing.T, repls testdiff.ReplacementsContext, sortLines []*regexp.Regexp, dirRef, dirNew, relPath string, printedRepls *bool) { +// compileSuppressLines compiles the non-empty SuppressLines patterns from the test config. +// Patterns are returned in name order so the result does not depend on map iteration. +func compileSuppressLines(t *testing.T, config internal.TestConfig) []*regexp.Regexp { + result := make([]*regexp.Regexp, 0, len(config.SuppressLines)) + for _, name := range slices.Sorted(maps.Keys(config.SuppressLines)) { + pattern := config.SuppressLines[name] + if pattern == "" { + continue + } + re, err := regexp.Compile(pattern) + if err != nil { + t.Fatalf("Invalid SuppressLines pattern %s = %#v: %s", name, pattern, err) + } + result = append(result, re) + } + return result +} + +func doComparison(t *testing.T, repls testdiff.ReplacementsContext, sortLines, suppressLines []*regexp.Regexp, dirRef, dirNew, relPath string, printedRepls *bool) { pathRef := filepath.Join(dirRef, relPath) pathNew := filepath.Join(dirNew, relPath) bufRef, okRef := tryReading(t, pathRef) @@ -1258,6 +1277,17 @@ func doComparison(t *testing.T, repls testdiff.ReplacementsContext, sortLines [] valueNew = repls.Replace(valueNew) } + // Move lines the test does not pin (e.g. retry warnings) from the compared output to the log. + // Applied to the reference too, so a golden that still contains them compares the same way. + for _, re := range suppressLines { + var removed []string + valueNew, removed = testdiff.RemoveLines(valueNew, re) + valueRef, _ = testdiff.RemoveLines(valueRef, re) + if len(removed) > 0 { + t.Logf("Suppressed from %s:\n%s", relPath, strings.Join(removed, "\n")) + } + } + // Canonicalize line runs whose order the command does not guarantee. Applied to // the reference too so a hand-edited golden compares the same way; sorting is // idempotent, so a stored reference is unaffected. diff --git a/acceptance/bundle/deploy/files/upload-504/test.toml b/acceptance/bundle/deploy/files/upload-504/test.toml index 7d496b2f139..711ede517b9 100644 --- a/acceptance/bundle/deploy/files/upload-504/test.toml +++ b/acceptance/bundle/deploy/files/upload-504/test.toml @@ -1,3 +1,3 @@ RecordRequests = true Env.DATABRICKS_BUNDLE_RETRY_INTERVAL_MS = "100" -Env.DATABRICKS_BUNDLE_RETRY_DISABLE_WARNING = "" +SuppressLines.warn_retry = "" diff --git a/acceptance/bundle/resources/permissions/pipelines/504/test.toml b/acceptance/bundle/resources/permissions/pipelines/504/test.toml index 99ccafaf8e8..b26f818ab24 100644 --- a/acceptance/bundle/resources/permissions/pipelines/504/test.toml +++ b/acceptance/bundle/resources/permissions/pipelines/504/test.toml @@ -2,4 +2,4 @@ Env.DATABRICKS_BUNDLE_ENGINE = "direct" EnvMatrix.READPLAN = [] RecordRequests = true Env.DATABRICKS_BUNDLE_RETRY_INTERVAL_MS = "100" -Env.DATABRICKS_BUNDLE_RETRY_DISABLE_WARNING = "" +SuppressLines.warn_retry = "" diff --git a/acceptance/bundle/sync/upload-504/test.toml b/acceptance/bundle/sync/upload-504/test.toml index 7d496b2f139..711ede517b9 100644 --- a/acceptance/bundle/sync/upload-504/test.toml +++ b/acceptance/bundle/sync/upload-504/test.toml @@ -1,3 +1,3 @@ RecordRequests = true Env.DATABRICKS_BUNDLE_RETRY_INTERVAL_MS = "100" -Env.DATABRICKS_BUNDLE_RETRY_DISABLE_WARNING = "" +SuppressLines.warn_retry = "" diff --git a/acceptance/bundle/upload/internal_server_error/test.toml b/acceptance/bundle/upload/internal_server_error/test.toml index 838bbee034f..89b52a06240 100644 --- a/acceptance/bundle/upload/internal_server_error/test.toml +++ b/acceptance/bundle/upload/internal_server_error/test.toml @@ -1,2 +1,2 @@ Env.DATABRICKS_BUNDLE_RETRY_INTERVAL_MS = "100" -Env.DATABRICKS_BUNDLE_RETRY_DISABLE_WARNING = "" +SuppressLines.warn_retry = "" diff --git a/acceptance/cmd/sync/get-status-504/test.toml b/acceptance/cmd/sync/get-status-504/test.toml index 838bbee034f..89b52a06240 100644 --- a/acceptance/cmd/sync/get-status-504/test.toml +++ b/acceptance/cmd/sync/get-status-504/test.toml @@ -1,2 +1,2 @@ Env.DATABRICKS_BUNDLE_RETRY_INTERVAL_MS = "100" -Env.DATABRICKS_BUNDLE_RETRY_DISABLE_WARNING = "" +SuppressLines.warn_retry = "" diff --git a/acceptance/cmd/sync/upload-504/test.toml b/acceptance/cmd/sync/upload-504/test.toml index 7d496b2f139..711ede517b9 100644 --- a/acceptance/cmd/sync/upload-504/test.toml +++ b/acceptance/cmd/sync/upload-504/test.toml @@ -1,3 +1,3 @@ RecordRequests = true Env.DATABRICKS_BUNDLE_RETRY_INTERVAL_MS = "100" -Env.DATABRICKS_BUNDLE_RETRY_DISABLE_WARNING = "" +SuppressLines.warn_retry = "" diff --git a/acceptance/internal/config.go b/acceptance/internal/config.go index a6541d062de..6ea79a9578a 100644 --- a/acceptance/internal/config.go +++ b/acceptance/internal/config.go @@ -96,6 +96,13 @@ type TestConfig struct { // to true; set it to false to drop an inherited pattern for this test. SortLinesOn map[string]bool + // Map of name -> regexp. Output lines matching an enabled regexp are removed from the + // compared output and written to the test log instead. Use for lines that depend on + // timing or fault injection, e.g. retry warnings. Entries are keyed by name so an inner + // test.toml can replace an inherited pattern by reusing its name, or disable it by + // setting it to "". + SuppressLines map[string]string + // List of gitignore patterns to ignore when checking output files Ignore []string diff --git a/acceptance/selftest/suppresslines/keep/out.test.toml b/acceptance/selftest/suppresslines/keep/out.test.toml new file mode 100644 index 00000000000..18b1a88417e --- /dev/null +++ b/acceptance/selftest/suppresslines/keep/out.test.toml @@ -0,0 +1 @@ +Cloud = false diff --git a/acceptance/selftest/suppresslines/keep/output.txt b/acceptance/selftest/suppresslines/keep/output.txt new file mode 100644 index 00000000000..f0f63e21e56 --- /dev/null +++ b/acceptance/selftest/suppresslines/keep/output.txt @@ -0,0 +1,4 @@ +Signal: before +Noise: 1 +Signal: after +Warn: deploying resources.jobs.foo: retrying after 500 Internal Server Error from GET /api diff --git a/acceptance/selftest/suppresslines/keep/script b/acceptance/selftest/suppresslines/keep/script new file mode 100644 index 00000000000..bcc5b790f43 --- /dev/null +++ b/acceptance/selftest/suppresslines/keep/script @@ -0,0 +1,4 @@ +echo "Signal: before" +echo "Noise: 1" +echo "Signal: after" +echo "Warn: deploying resources.jobs.foo: retrying after 500 Internal Server Error from GET /api" diff --git a/acceptance/selftest/suppresslines/keep/test.toml b/acceptance/selftest/suppresslines/keep/test.toml new file mode 100644 index 00000000000..a8d77b0c82c --- /dev/null +++ b/acceptance/selftest/suppresslines/keep/test.toml @@ -0,0 +1,3 @@ +# An empty pattern disables the inherited one, so these lines are compared again. +SuppressLines.noise = "" +SuppressLines.warn_retry = "" diff --git a/acceptance/selftest/suppresslines/out.test.toml b/acceptance/selftest/suppresslines/out.test.toml new file mode 100644 index 00000000000..18b1a88417e --- /dev/null +++ b/acceptance/selftest/suppresslines/out.test.toml @@ -0,0 +1 @@ +Cloud = false diff --git a/acceptance/selftest/suppresslines/output.txt b/acceptance/selftest/suppresslines/output.txt new file mode 100644 index 00000000000..0b56a035232 --- /dev/null +++ b/acceptance/selftest/suppresslines/output.txt @@ -0,0 +1,2 @@ +Signal: before +Signal: after diff --git a/acceptance/selftest/suppresslines/script b/acceptance/selftest/suppresslines/script new file mode 100644 index 00000000000..bcc5b790f43 --- /dev/null +++ b/acceptance/selftest/suppresslines/script @@ -0,0 +1,4 @@ +echo "Signal: before" +echo "Noise: 1" +echo "Signal: after" +echo "Warn: deploying resources.jobs.foo: retrying after 500 Internal Server Error from GET /api" diff --git a/acceptance/selftest/suppresslines/test.toml b/acceptance/selftest/suppresslines/test.toml new file mode 100644 index 00000000000..81de16753e3 --- /dev/null +++ b/acceptance/selftest/suppresslines/test.toml @@ -0,0 +1,4 @@ +# Runs once: suppression is independent of the deployment engine. + +# Lines matching the pattern are logged instead of being compared. +SuppressLines.noise = '^Noise: ' diff --git a/acceptance/test.toml b/acceptance/test.toml index 714b05e5242..6e18a40e78a 100644 --- a/acceptance/test.toml +++ b/acceptance/test.toml @@ -19,16 +19,17 @@ Env.PYTHONDONTWRITEBYTECODE = "1" Env.PYTHONUNBUFFERED = "1" Env.PYTHONUTF8 = "1" -# Retries are timing dependent, so their warnings would make output flaky. -# Tests that inject errors deterministically and assert the warning set this to "". -Env.DATABRICKS_BUNDLE_RETRY_DISABLE_WARNING = "1" - # The direct engine reports resources as soon as each one is applied, and resources are # applied in parallel, so the order of these lines is not deterministic. Sort each run of # them before comparing, so tests pin which resources were applied, not the order. # A test that needs the raw order sets SortLinesOn.deploy_actions = false. SortLines.deploy_actions = '^(Created|Updated|Deleted|Recreated|Resized) \S+$' +# Retries are timing dependent, so their warnings would make output flaky. They go to the +# test log instead. A test that injects errors deterministically and asserts the warning +# sets SuppressLines.warn_retry = "". +SuppressLines.warn_retry = '^Warn: .*retrying after ' + # >>> datetime.datetime.fromtimestamp(18000000000) # datetime.datetime(2027, 1, 15, 9, 0) # >>> datetime.datetime.fromtimestamp(1900000000) diff --git a/libs/retry/retry.go b/libs/retry/retry.go index 9a8a5e75db4..86dd9ec232c 100644 --- a/libs/retry/retry.go +++ b/libs/retry/retry.go @@ -23,10 +23,6 @@ const MaxRetries = 2 // IntervalMsVariable names the environment variable that overrides the pause between attempts. const IntervalMsVariable = "DATABRICKS_BUNDLE_RETRY_INTERVAL_MS" -// DisableWarningVariable names the environment variable that, when set to a non-empty value, -// suppresses the warning logged before each retry. -const DisableWarningVariable = "DATABRICKS_BUNDLE_RETRY_DISABLE_WARNING" - const defaultInterval = 15 * time.Second // interval returns the pause between attempts: the override in milliseconds if set and valid, 15s otherwise. @@ -89,9 +85,7 @@ func With[T any](ctx context.Context, check func(context.Context, error) bool, f if err == nil || attempt >= MaxRetries || !check(ctx, err) { return result, err } - if env.Get(ctx, DisableWarningVariable) == "" { - log.Warnf(ctx, "%s", retryMessage(err)) - } + log.Warnf(ctx, "%s", retryMessage(err)) select { case <-ctx.Done(): var zero T diff --git a/libs/testdiff/removelines.go b/libs/testdiff/removelines.go new file mode 100644 index 00000000000..f8d280a3eee --- /dev/null +++ b/libs/testdiff/removelines.go @@ -0,0 +1,21 @@ +package testdiff + +import ( + "regexp" + "strings" +) + +// RemoveLines drops every line matching pattern from input and returns the remaining text +// along with the dropped lines. Use it for output lines that depend on timing or fault +// injection, so that a test does not have to pin them. +func RemoveLines(input string, pattern *regexp.Regexp) (string, []string) { + var kept, removed []string + for line := range strings.SplitSeq(input, "\n") { + if pattern.MatchString(line) { + removed = append(removed, line) + } else { + kept = append(kept, line) + } + } + return strings.Join(kept, "\n"), removed +} diff --git a/libs/testdiff/removelines_test.go b/libs/testdiff/removelines_test.go new file mode 100644 index 00000000000..35d4bf03df2 --- /dev/null +++ b/libs/testdiff/removelines_test.go @@ -0,0 +1,20 @@ +package testdiff + +import ( + "regexp" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestRemoveLines(t *testing.T) { + kept, removed := RemoveLines("a\nNoise: 1\nb\nNoise: 2\n", regexp.MustCompile(`^Noise: `)) + assert.Equal(t, "a\nb\n", kept) + assert.Equal(t, []string{"Noise: 1", "Noise: 2"}, removed) +} + +func TestRemoveLinesNoMatch(t *testing.T) { + kept, removed := RemoveLines("a\nb", regexp.MustCompile(`^Noise: `)) + assert.Equal(t, "a\nb", kept) + assert.Empty(t, removed) +}