diff --git a/.agent/rules/testing.md b/.agent/rules/testing.md index f0bdff1366..9af7bc9a36 100644 --- a/.agent/rules/testing.md +++ b/.agent/rules/testing.md @@ -208,6 +208,22 @@ trace $CLI postgres get-branch "$name" # full payload straight into output.txt For a field that appears on some responses but not others (e.g. a provenance field absent on a default resource), append `| with_entries(select(.value != null))` to the projected object so the optional field is dropped rather than emitted as `null`. +### Soft-failing volatile goldens + +Some goldens drift for reasons outside our control: a live backend rewords a message, a response grows a field we don't assert, or a remotely-hosted template is updated upstream. None is a regression, yet each turns cloud PRs and nightlies red. `SoftFailFiles` / `SoftFail` in `test.toml` downgrade a *content diff* from a hard failure to a non-blocking `SOFTFAIL` marker. See `acceptance/README.md` ("Soft-failing volatile goldens") for the full mechanism and oncall refresh cadence. + +**RULE: Soft-failing is the last resort. Reach for the cheaper tools first, in order.** A `Repls` regex mask for *describable* volatility (UUIDs, timestamps, versions); then field projection (`_fields()`, above) for "the backend added a field I don't assert" — projecting to the fields you assert ignores new fields with zero drift and should never reach soft-fail. Only *undescribable* drift you can't regex ahead of time (a reworded message whose new text is unknown, a wholesale remote-artifact dump) justifies `SoftFailFiles`. + +**RULE: Shield per file, and route the volatile output to its own `out.*` file so the assertable part stays strict.** Split along a *volatility* seam, not a *convenience* seam: keep our own logic (e.g. the plan's action/reason) in a strict golden and isolate only the backend-mirrored output into the soft-failed `out.*` file. + +**RULE: `output.txt` can never be soft-failed** (it's a hard config error), and every `SoftFailFiles` entry must start with `out`. `output.txt` carries the CLI behavior a local regression would corrupt, so our own logic still turns the test red. + +**RULE: Only a content diff is downgraded.** A panic, an unexpected exit code, a missing golden, or an unexpected new file stays a hard failure even under `SoftFail`. + +**RULE: The shield takes effect only on cloud runs.** `SoftFailFiles` and `SoftFail` are ignored when `CLOUD_ENV` is unset, because a pure local testserver test is deterministic and cannot drift — any diff on a local run stays a hard failure. A test that is both `Local` and `Cloud` therefore keeps its local golden strict while shielding cloud drift. + +**RULE: Reserve the whole-test shield `SoftFail = true` for e2e tests whose behavior depends on a runtime-fetched artifact, and only when the same behavior is also covered by a hermetic local test.** It shields *every* golden including `output.txt`, so it can only be safe when a real regression would still turn the local test red. Prefer `SoftFailFiles` whenever a volatility seam can be isolated. + ### Test server Acceptance tests run against an in-process fake of the Databricks API in `libs/testserver/` (`FakeWorkspace` and the per-service handler files). The fake keeps real in-memory state and returns the same errors the backend does: 404 on a missing parent, 409 on a duplicate create, 400 on a missing required field, and so on. `test.toml` can also stub a single route with a canned response: diff --git a/.github/workflows/push.yml b/.github/workflows/push.yml index eb0ea35e05..86b2f6be61 100644 --- a/.github/workflows/push.yml +++ b/.github/workflows/push.yml @@ -152,6 +152,15 @@ jobs: if-no-files-found: warn retention-days: 7 + - name: Report soft-failed acceptance goldens + # Non-blocking: surfaces goldens that drifted but did not fail the build, + # so oncall can refresh them on a cadence. Runs even when tests failed. + if: ${{ always() }} + run: | + if [ -f test-output.json ]; then + python3 tools/softfail_report.py test-output.json >> "$GITHUB_STEP_SUMMARY" + fi + - name: Check no files changed or appeared after running tests run: | # Register untracked files with intent-to-add so `git diff` reports them diff --git a/acceptance/README.md b/acceptance/README.md index 7277481327..006a462e5a 100644 --- a/acceptance/README.md +++ b/acceptance/README.md @@ -26,6 +26,77 @@ Any file starting with "LOG" will be logged to test log (visible with go test -v See [selftest](./selftest) for more examples. +## Soft-failing volatile goldens + +Some goldens drift for reasons outside our control: a live backend rewords a message, a +response grows a field we don't assert, or a remotely-hosted template is updated upstream. +None of these are regressions, yet each turns PRs and nightlies red. `SoftFailFiles` +downgrades a *content diff* in a named golden from a hard failure to a recorded, +non-blocking `SOFTFAIL` marker. It is the last resort — reach for these tools in order: + +1. **`Repls` regex mask** — for *describable* volatility (UUIDs, timestamps, versions). Keeps the rest of the file strict. +2. **Field projection** (`_fields()`, see `.agent/rules/testing.md`) — for "the backend added a field I don't assert". Projecting a response to the fields you assert ignores new fields with zero drift. This fully handles the extra-field case; it should never reach soft-fail. +3. **`SoftFailFiles`** — only for *undescribable* drift you can't regex ahead of time: a reworded message whose new text is unknown, or a wholesale remote-artifact dump we don't control. + +### How it works + +Add the golden's name to `SoftFailFiles` in `test.toml` (inherited from a parent like any +other config, and surfaced in `out.test.toml` so reviewers see the shield on every PR). +Add a comment explaining why the file drifts: + +```toml +# out.template.txt is a dump of the remotely-fetched mlops-stacks template +SoftFailFiles = ["out.template.txt"] +``` + +- **`output.txt` can never be shielded** (a hard config error), and entries must start with `out`. `output.txt` carries the CLI behavior a local regression would corrupt, so a regression in our own logic still turns the test red. +- Only a *content diff* is downgraded. A panic, an unexpected exit code, a missing golden, or an unexpected new file stays a hard failure. + +Because the shield is per-file, **route volatile output to its own `out.*` file** and keep +the assertable part strict. Split along a *volatility* seam, not a *convenience* seam: + +```bash +# strict — our logic, stays red on any drift +trace $CLI bundle plan | contains.py "Plan: 1 to add" + +# volatile — reworded backend output, isolated into its own soft-failed file +trace $CLI some-command-with-drifty-output &> out.drifty.txt +``` + +A pure local testserver test is deterministic and cannot drift, so the shield is +**ignored on local runs**: `SoftFailFiles` and `SoftFail` take effect only when +`CLOUD_ENV` is set, and any diff on a local run stays a hard failure. A test that is both +`Local` and `Cloud` therefore keeps its local golden strict while shielding cloud drift. + +### Whole-test shield (`SoftFail`) + +When a test's behavior *end-to-end* depends on a runtime-fetched artifact outside our +control — so an upstream change breaks not just captured output but the run itself (e.g. a +remotely-hosted template whose new resources fail to deploy) — no volatility seam exists to +split, because `output.txt` itself is the drift. `SoftFail = true` shields **every** golden +for that test, including `output.txt`: + +```toml +# mlops-stacks template is fetched remotely; upstream changes break this e2e test +SoftFail = true +``` + +This is a blunt instrument. Only use it when the CLI behavior under test is **also covered +by a hermetic local test**, so shielding the whole e2e test can't hide a real regression in +that behavior (it would still turn the local test red). Prefer `SoftFailFiles` whenever a +seam can be isolated. Structural failures (panics, unexpected/missing files) stay hard even +under `SoftFail`. + +### Refresh cadence (oncall) + +Soft-failed goldens are reported to the GitHub step summary on every run (via +`tools/softfail_report.py`, which greps the gotestsum JSON for `SOFTFAIL` markers) so drift +is visible even though the build is green. On a cadence, oncall: + +1. runs `./task test-update` (cloud tests under the cloud runner), +2. eyeballs `git diff` — mundane drift → commit; anything resembling a real behavior change → investigate, +3. commits the refreshed goldens. + ## Benchmarks Benchmarks are regular acceptance test that log measurements in certain format. The output can be fed to `tools/bench_parse.py` to print a summary table. diff --git a/acceptance/acceptance_test.go b/acceptance/acceptance_test.go index e5781f88f4..cb18738a80 100644 --- a/acceptance/acceptance_test.go +++ b/acceptance/acceptance_test.go @@ -972,13 +972,23 @@ func runTest(t *testing.T, pathFilter := preparePathFilter(config, customEnv) + // Soft-fail shields drift we don't control (cloud API output, remote artifacts), + // which only occurs on cloud runs. A local testserver run is deterministic, so any + // diff there is a real regression and must stay red regardless of the shield config. + softFailFiles := config.SoftFailFiles + softFailAll := isTruePtr(config.SoftFail) + if !isRunningOnCloud { + softFailFiles = nil + softFailAll = false + } + // Compare expected outputs for relPath := range outputs { if shouldSkip(pathFilter, relPath) { continue } - doComparison(t, repls, dir, tmpDir, relPath, &printedRepls) + doComparison(t, repls, dir, tmpDir, relPath, softFailFiles, softFailAll, &printedRepls) } // Make sure there are not unaccounted for new files @@ -1012,7 +1022,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, dir, tmpDir, relPath, &printedRepls) + doComparison(t, repls, dir, tmpDir, relPath, softFailFiles, softFailAll, &printedRepls) } } @@ -1085,7 +1095,7 @@ func addEnvVar(t *testing.T, env []string, repls *testdiff.ReplacementsContext, return append(env, key+"="+newValue) } -func doComparison(t *testing.T, repls testdiff.ReplacementsContext, dirRef, dirNew, relPath string, printedRepls *bool) { +func doComparison(t testutil.TestingT, repls testdiff.ReplacementsContext, dirRef, dirNew, relPath string, softFailFiles []string, softFailAll bool, printedRepls *bool) { pathRef := filepath.Join(dirRef, relPath) pathNew := filepath.Join(dirNew, relPath) bufRef, okRef := tryReading(t, pathRef) @@ -1145,6 +1155,17 @@ func doComparison(t *testing.T, repls testdiff.ReplacementsContext, dirRef, dirN return } + // Soft-fail: a content diff in a shielded golden is downgraded to a recorded, + // non-blocking marker instead of failing the test. softFailAll shields every + // file including output.txt; softFailFiles shields named files only. This must + // run before AssertEqualTexts, which calls testify's assert.Equal and marks the + // test failed as a side effect that its bool return cannot undo. + if valueRef != valueNew && (softFailAll || slices.Contains(softFailFiles, relPath)) { + diff := testdiff.UnifiedDiff(pathRef, pathNew, valueRef, valueNew) + t.Logf("SOFTFAIL %s\n%s", relPath, diff) + return + } + // Compare the reference and new values. equal := testdiff.AssertEqualTexts(t, pathRef, pathNew, valueRef, valueNew) @@ -1441,7 +1462,7 @@ func formatOutput(w io.Writer, err error) { } } -func tryReading(t *testing.T, path string) (string, bool) { +func tryReading(t testutil.TestingT, path string) (string, bool) { info, err := os.Stat(path) if err != nil { if !errors.Is(err, os.ErrNotExist) { diff --git a/acceptance/bundle/deploy/mlops-stacks/out.test.toml b/acceptance/bundle/deploy/mlops-stacks/out.test.toml index 2d812727e3..9cad1bffd3 100644 --- a/acceptance/bundle/deploy/mlops-stacks/out.test.toml +++ b/acceptance/bundle/deploy/mlops-stacks/out.test.toml @@ -1,4 +1,5 @@ Local = false Cloud = true RequiresUnityCatalog = true +SoftFail = true EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"] diff --git a/acceptance/bundle/deploy/mlops-stacks/test.toml b/acceptance/bundle/deploy/mlops-stacks/test.toml index 3b87ff1bf1..38df5031b0 100644 --- a/acceptance/bundle/deploy/mlops-stacks/test.toml +++ b/acceptance/bundle/deploy/mlops-stacks/test.toml @@ -9,6 +9,13 @@ RequiresUnityCatalog = true Badness = "the newly initialized bundle from the 'mlops-stacks' template contains two validation warnings in the configuration" +# This test runs the mlops-stacks template end-to-end. The template is fetched from +# github.com/databricks/mlops-stacks at runtime and updated out of band, so upstream +# changes break not just the captured output but the deploy itself. bundle init and +# bundle deploy are covered by hermetic local tests, so shielding this e2e test cannot +# hide a real CLI regression. Refresh the goldens with `./task test-update` on cloud. +SoftFail = true + Ignore = [ "config.json" ] diff --git a/acceptance/bundle/resources/dashboards/detect-change/out.plan.direct.json b/acceptance/bundle/resources/dashboards/detect-change/out.plan.direct.json index b00e44d4fe..ee62d3427c 100644 --- a/acceptance/bundle/resources/dashboards/detect-change/out.plan.direct.json +++ b/acceptance/bundle/resources/dashboards/detect-change/out.plan.direct.json @@ -16,57 +16,36 @@ "warehouse_id": "[TEST_DEFAULT_WAREHOUSE_ID]" } }, - "remote_state": { - "create_time": "[TIMESTAMP]", - "dashboard_id": "[DASHBOARD_ID]", - "display_name": "test-dashboard-[UNIQUE_NAME]", - "embed_credentials": false, - "etag": "[ETAG_2]", - "lifecycle_state": "ACTIVE", - "parent_path": "/Workspace/Users/[USERNAME]/.bundle/[UNIQUE_NAME]/resources", - "path": "/Users/[USERNAME]/.bundle/[UNIQUE_NAME]/resources/test-dashboard-[UNIQUE_NAME].lvdash.json", - "published": true, - "serialized_dashboard": "{}\n", - "update_time": "[TIMESTAMP]", - "warehouse_id": "[TEST_DEFAULT_WAREHOUSE_ID]" - }, "changes": { "create_time": { "action": "skip", - "reason": "spec:output_only", - "remote": "[TIMESTAMP]" + "reason": "spec:output_only" }, "dashboard_id": { "action": "skip", - "reason": "spec:output_only", - "remote": "[DASHBOARD_ID]" + "reason": "spec:output_only" }, "etag": { "action": "update", - "old": "[ETAG_1]", - "remote": "[ETAG_2]" + "old": "[ETAG_1]" }, "lifecycle_state": { "action": "skip", - "reason": "spec:output_only", - "remote": "ACTIVE" + "reason": "spec:output_only" }, "path": { "action": "skip", - "reason": "spec:output_only", - "remote": "/Users/[USERNAME]/.bundle/[UNIQUE_NAME]/resources/test-dashboard-[UNIQUE_NAME].lvdash.json" + "reason": "spec:output_only" }, "serialized_dashboard": { "action": "skip", "reason": "etag_based", "old": "{\n \"pages\": [\n {\n \"displayName\": \"New Page\",\n \"layout\": [\n {\n \"position\": {\n \"height\": 2,\n \"width\": 6,\n \"x\": 0,\n \"y\": 0\n },\n \"widget\": {\n \"name\": \"82eb9107\",\n \"textbox_spec\": \"# I'm a title\"\n }\n },\n {\n \"position\": {\n \"height\": 2,\n \"width\": 6,\n \"x\": 0,\n \"y\": 2\n },\n \"widget\": {\n \"name\": \"ffa6de4f\",\n \"textbox_spec\": \"Text\"\n }\n }\n ],\n \"name\": \"fdd21a3c\"\n }\n ]\n}\n", - "new": "{\n \"pages\": [\n {\n \"displayName\": \"New Page\",\n \"layout\": [\n {\n \"position\": {\n \"height\": 2,\n \"width\": 6,\n \"x\": 0,\n \"y\": 0\n },\n \"widget\": {\n \"name\": \"82eb9107\",\n \"textbox_spec\": \"# I'm a title\"\n }\n },\n {\n \"position\": {\n \"height\": 2,\n \"width\": 6,\n \"x\": 0,\n \"y\": 2\n },\n \"widget\": {\n \"name\": \"ffa6de4f\",\n \"textbox_spec\": \"Text\"\n }\n }\n ],\n \"name\": \"fdd21a3c\"\n }\n ]\n}\n", - "remote": "{}\n" + "new": "{\n \"pages\": [\n {\n \"displayName\": \"New Page\",\n \"layout\": [\n {\n \"position\": {\n \"height\": 2,\n \"width\": 6,\n \"x\": 0,\n \"y\": 0\n },\n \"widget\": {\n \"name\": \"82eb9107\",\n \"textbox_spec\": \"# I'm a title\"\n }\n },\n {\n \"position\": {\n \"height\": 2,\n \"width\": 6,\n \"x\": 0,\n \"y\": 2\n },\n \"widget\": {\n \"name\": \"ffa6de4f\",\n \"textbox_spec\": \"Text\"\n }\n }\n ],\n \"name\": \"fdd21a3c\"\n }\n ]\n}\n" }, "update_time": { "action": "skip", - "reason": "spec:output_only", - "remote": "[TIMESTAMP]" + "reason": "spec:output_only" } } } diff --git a/acceptance/bundle/resources/dashboards/detect-change/out.plan.remote.direct.json b/acceptance/bundle/resources/dashboards/detect-change/out.plan.remote.direct.json new file mode 100644 index 0000000000..3311218de1 --- /dev/null +++ b/acceptance/bundle/resources/dashboards/detect-change/out.plan.remote.direct.json @@ -0,0 +1,27 @@ +{ + "resources.dashboards.file_reference": { + "remote_state": { + "create_time": "[TIMESTAMP]", + "dashboard_id": "[DASHBOARD_ID]", + "display_name": "test-dashboard-[UNIQUE_NAME]", + "embed_credentials": false, + "etag": "[ETAG_2]", + "lifecycle_state": "ACTIVE", + "parent_path": "/Workspace/Users/[USERNAME]/.bundle/[UNIQUE_NAME]/resources", + "path": "/Users/[USERNAME]/.bundle/[UNIQUE_NAME]/resources/test-dashboard-[UNIQUE_NAME].lvdash.json", + "published": true, + "serialized_dashboard": "{}\n", + "update_time": "[TIMESTAMP]", + "warehouse_id": "[TEST_DEFAULT_WAREHOUSE_ID]" + }, + "changes": { + "create_time": "[TIMESTAMP]", + "dashboard_id": "[DASHBOARD_ID]", + "etag": "[ETAG_2]", + "lifecycle_state": "ACTIVE", + "path": "/Users/[USERNAME]/.bundle/[UNIQUE_NAME]/resources/test-dashboard-[UNIQUE_NAME].lvdash.json", + "serialized_dashboard": "{}\n", + "update_time": "[TIMESTAMP]" + } + } +} diff --git a/acceptance/bundle/resources/dashboards/detect-change/out.plan.remote.terraform.json b/acceptance/bundle/resources/dashboards/detect-change/out.plan.remote.terraform.json new file mode 100644 index 0000000000..656f3a3e1f --- /dev/null +++ b/acceptance/bundle/resources/dashboards/detect-change/out.plan.remote.terraform.json @@ -0,0 +1,6 @@ +{ + "resources.dashboards.file_reference": { + "remote_state": null, + "changes": {} + } +} diff --git a/acceptance/bundle/resources/dashboards/detect-change/out.test.toml b/acceptance/bundle/resources/dashboards/detect-change/out.test.toml index f489904c6f..15941ff9b3 100644 --- a/acceptance/bundle/resources/dashboards/detect-change/out.test.toml +++ b/acceptance/bundle/resources/dashboards/detect-change/out.test.toml @@ -4,3 +4,4 @@ RequiresWarehouse = true GOOSOnPR.darwin = false GOOSOnPR.windows = false EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["terraform", "direct"] +SoftFailFiles = ["out.plan.remote.direct.json"] diff --git a/acceptance/bundle/resources/dashboards/detect-change/output.txt b/acceptance/bundle/resources/dashboards/detect-change/output.txt index fe3f8962ec..d82af2bc78 100644 --- a/acceptance/bundle/resources/dashboards/detect-change/output.txt +++ b/acceptance/bundle/resources/dashboards/detect-change/output.txt @@ -82,6 +82,10 @@ To overwrite the remote changes with your local version, use --force. The remote modifications will be lost. +>>> jq del(.plan[].remote_state?) | del(.plan[].changes[]?.remote) plan.json + +>>> jq .plan | map_values({remote_state, changes: (.changes // {} | map_values(.remote))}) plan.json + >>> errcode [CLI] bundle deploy Error: dashboard "file_reference" has been modified remotely at resources.dashboards.file_reference diff --git a/acceptance/bundle/resources/dashboards/detect-change/script b/acceptance/bundle/resources/dashboards/detect-change/script index 98ab4cc93b..dbddd015a6 100644 --- a/acceptance/bundle/resources/dashboards/detect-change/script +++ b/acceptance/bundle/resources/dashboards/detect-change/script @@ -38,7 +38,15 @@ echo "$(echo "$UPDATE_RESP" | jq -r '.etag'):ETAG_2" >> ACC_REPLS title "Try to redeploy the bundle and confirm that the out of band modification is detected:" trace $CLI bundle plan -trace $CLI bundle plan -o json > out.plan.$DATABRICKS_BUNDLE_ENGINE.json +# Split the plan JSON along a volatility seam. The planning decisions +# (action/reason/old/new) are our logic and stay strict. The remote view -- +# remote_state (the full backend read) and each change's `remote` value -- mirrors +# the dashboards API response, whose output-only fields grow and change out of our +# control on cloud, so it is routed to its own soft-failed file (see test.toml). +trace $CLI bundle plan -o json > plan.json +trace jq 'del(.plan[].remote_state?) | del(.plan[].changes[]?.remote)' plan.json > out.plan.$DATABRICKS_BUNDLE_ENGINE.json +trace jq '.plan | map_values({remote_state, changes: (.changes // {} | map_values(.remote))})' plan.json > out.plan.remote.$DATABRICKS_BUNDLE_ENGINE.json +rm plan.json trace errcode $CLI bundle deploy title "Redeploy the bundle with the --force flag and confirm that the out of band modification is ignored:" diff --git a/acceptance/bundle/resources/dashboards/detect-change/test.toml b/acceptance/bundle/resources/dashboards/detect-change/test.toml index fbdfbe42ea..4c04aa4998 100644 --- a/acceptance/bundle/resources/dashboards/detect-change/test.toml +++ b/acceptance/bundle/resources/dashboards/detect-change/test.toml @@ -1,6 +1,18 @@ Local = true Cloud = true +# The plan's remote view (remote_state + each change's `remote` value) mirrors the +# dashboards API response. Its output-only fields (create_time, update_time, etag, +# lifecycle_state, path, ...) are populated by the backend and grow/change out of our +# control on cloud, so the remote-only file is soft-failed. The shield only takes effect +# on cloud runs, so the deterministic local run keeps this file strict too. The planning +# decisions in out.plan.$engine.json stay strict on both. Only the direct engine exposes +# a remote view; the terraform remote file is structurally empty and stays strict (a +# regression that suddenly populated it should turn the test red). +SoftFailFiles = [ + "out.plan.remote.direct.json", +] + Ignore = [ "databricks.yml", ] diff --git a/acceptance/internal/config.go b/acceptance/internal/config.go index b2d1a9f578..fe33356b49 100644 --- a/acceptance/internal/config.go +++ b/acceptance/internal/config.go @@ -2,6 +2,7 @@ package internal import ( "errors" + "fmt" "hash/fnv" "io/fs" "maps" @@ -95,6 +96,23 @@ type TestConfig struct { // List of gitignore patterns to ignore when checking output files Ignore []string + // Golden files whose content diffs are downgraded from a hard failure to a + // recorded, non-blocking SOFTFAIL. Use ONLY for files that capture volatility + // outside our control that a Repls mask or field-projection can't express + // (reworded backend messages, remotely-updated template dumps). "output.txt" + // is rejected: it carries the CLI behavior a local regression would corrupt. + SoftFailFiles []string + + // If true, ALL golden files for this test (including output.txt) are soft-failed: + // any content diff is downgraded to a non-blocking SOFTFAIL marker. This is a + // blunt, whole-test shield reserved for tests whose behavior end-to-end depends + // on an artifact fetched at runtime outside our control (e.g. a remotely-hosted + // template), where an upstream change breaks not just captured output but the + // run itself. Only use it when the CLI behavior under test is also covered by a + // hermetic local test. Prefer the per-file SoftFailFiles whenever a volatility + // seam can be isolated. + SoftFail *bool + CompiledIgnoreObject *ignore.GitIgnore // Environment variables @@ -257,6 +275,26 @@ func validateConfig(t *testing.T, config TestConfig, configPath string) { "Output files must not be ignored.", configPath, pattern) } } + + if err := validateSoftFailFiles(config); err != nil { + t.Fatalf("Invalid config %s: %s", configPath, err) + } +} + +// validateSoftFailFiles enforces the per-file soft-fail structural guards: an +// entry may not target output.txt (it carries the CLI behavior a local regression +// would corrupt) and must be a generated file (out*). The whole-test SoftFail +// shield deliberately has no such file restriction. +func validateSoftFailFiles(config TestConfig) error { + for _, name := range config.SoftFailFiles { + if name == "output.txt" { + return fmt.Errorf("SoftFailFiles must not contain %q; it carries CLI behavior a local regression would corrupt", name) + } + if !strings.HasPrefix(name, "out") { + return fmt.Errorf("SoftFailFiles entry %q must start with \"out\"; only generated output files can be soft-failed", name) + } + } + return nil } func DoLoadConfig(t *testing.T, path string) TestConfig { diff --git a/acceptance/internal/config_test.go b/acceptance/internal/config_test.go index a81345ed57..262b087794 100644 --- a/acceptance/internal/config_test.go +++ b/acceptance/internal/config_test.go @@ -242,6 +242,50 @@ func TestSubsetExpanded_ScriptUsesEngine(t *testing.T) { assert.True(t, engines["DATABRICKS_BUNDLE_ENGINE=direct"]) } +func TestValidateSoftFailFiles(t *testing.T) { + yes := true + tests := []struct { + name string + config TestConfig + wantErr string + }{ + { + name: "empty is allowed", + config: TestConfig{}, + }, + { + name: "valid out file", + config: TestConfig{SoftFailFiles: []string{"out.drifty.txt"}}, + }, + { + name: "output.txt is rejected", + config: TestConfig{SoftFailFiles: []string{"output.txt"}}, + wantErr: "must not contain", + }, + { + name: "non-out file is rejected", + config: TestConfig{SoftFailFiles: []string{"databricks.yml"}}, + wantErr: "must start with", + }, + { + name: "whole-test SoftFail", + config: TestConfig{SoftFail: &yes}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateSoftFailFiles(tt.config) + if tt.wantErr == "" { + assert.NoError(t, err) + } else { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + } + }) + } +} + func TestLoadConfigPhaseIsNotInherited(t *testing.T) { tests := []struct { name string diff --git a/acceptance/internal/materialized_config.go b/acceptance/internal/materialized_config.go index cd6d34739d..4ac5e8bcad 100644 --- a/acceptance/internal/materialized_config.go +++ b/acceptance/internal/materialized_config.go @@ -47,6 +47,7 @@ func GenerateMaterializedConfig(config *TestConfig) string { writeBool(&buf, "RequiresCluster", config.RequiresCluster) writeBool(&buf, "RequiresWarehouse", config.RequiresWarehouse) writeBool(&buf, "RunsOnDbr", config.RunsOnDbr) + writeBool(&buf, "SoftFail", config.SoftFail) if config.Phase != 0 { fmt.Fprintf(&buf, "Phase = %d\n", config.Phase) } @@ -65,6 +66,10 @@ func GenerateMaterializedConfig(config *TestConfig) string { writeTomlStringArray(&buf, "EnvMatrix."+k, envMatrix[k]) } + if len(config.SoftFailFiles) > 0 { + writeTomlStringArray(&buf, "SoftFailFiles", config.SoftFailFiles) + } + return buf.String() } diff --git a/acceptance/softfail_test.go b/acceptance/softfail_test.go new file mode 100644 index 0000000000..73fa3928aa --- /dev/null +++ b/acceptance/softfail_test.go @@ -0,0 +1,104 @@ +package acceptance_test + +import ( + "context" + "fmt" + "path/filepath" + "strings" + "testing" + + "github.com/databricks/cli/internal/testutil" + "github.com/databricks/cli/libs/testdiff" + "github.com/stretchr/testify/assert" +) + +// recordingT records log output and whether a failure was reported, so we can +// drive doComparison without failing the enclosing test. +type recordingT struct { + testutil.TestingT + ctx context.Context + logs strings.Builder + failed bool +} + +func (r *recordingT) Helper() {} + +func (r *recordingT) Log(args ...any) { fmt.Fprintln(&r.logs, args...) } + +func (r *recordingT) Logf(format string, args ...any) { + fmt.Fprintf(&r.logs, format, args...) +} + +func (r *recordingT) Error(args ...any) { r.failed = true } + +func (r *recordingT) Errorf(format string, args ...any) { r.failed = true } + +func (r *recordingT) Context() context.Context { return r.ctx } + +// TestSoftFailComparison exercises the soft-fail branch in doComparison directly. +// A committed golden mismatch can't drive an acceptance selftest for this: -update +// would heal it, so the SOFTFAIL path would stop firing. Driving the comparison +// with a recording T lets us assert both the green-on-listed-file path and that an +// unlisted file (including output.txt) still fails. +func TestSoftFailComparison(t *testing.T) { + tests := []struct { + name string + relPath string + softFailFiles []string + softFailAll bool + wantFailed bool + wantSoftfail bool + }{ + { + name: "listed file drifts green with marker", + relPath: "out.drifty.txt", + softFailFiles: []string{"out.drifty.txt"}, + wantFailed: false, + wantSoftfail: true, + }, + { + name: "unlisted file drift still fails", + relPath: "out.other.txt", + softFailFiles: []string{"out.drifty.txt"}, + wantFailed: true, + wantSoftfail: false, + }, + { + name: "output.txt drift still fails", + relPath: "output.txt", + softFailFiles: []string{"out.drifty.txt"}, + wantFailed: true, + wantSoftfail: false, + }, + { + name: "whole-test shield drifts green with marker", + relPath: "out.other.txt", + softFailAll: true, + wantFailed: false, + wantSoftfail: true, + }, + { + name: "whole-test shield covers output.txt", + relPath: "output.txt", + softFailAll: true, + wantFailed: false, + wantSoftfail: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dirRef := t.TempDir() + dirNew := t.TempDir() + testutil.WriteFile(t, filepath.Join(dirRef, tt.relPath), "reference\n") + testutil.WriteFile(t, filepath.Join(dirNew, tt.relPath), "drifted\n") + + rt := &recordingT{ctx: t.Context()} + doComparison(rt, testdiff.ReplacementsContext{}, dirRef, dirNew, tt.relPath, tt.softFailFiles, tt.softFailAll, nil) + + assert.Equal(t, tt.wantFailed, rt.failed) + gotSoftfail := strings.Contains(rt.logs.String(), "SOFTFAIL "+tt.relPath) + assert.Equal(t, tt.wantSoftfail, gotSoftfail) + }) + } +} diff --git a/tools/softfail_report.py b/tools/softfail_report.py new file mode 100644 index 0000000000..4d7b4ebd68 --- /dev/null +++ b/tools/softfail_report.py @@ -0,0 +1,117 @@ +#!/usr/bin/env python3 +""" +Scan a gotestsum/`go test -json` file for SOFTFAIL markers and print a markdown +report. Acceptance tests emit `SOFTFAIL ` followed by a unified diff via +t.Logf when a soft-failed golden drifts (see doComparison in acceptance_test.go). + +Those tests stay green, so the drift is invisible unless we surface it. This report +is written to the GitHub step summary on every run so oncall can refresh the golden +on a cadence without the build ever turning red. + +Usage: softfail_report.py test-output.json [test-output.json ...] +Exits 0 whether or not markers are found (soft-fail is non-blocking by design). +""" + +import json +import sys +from collections import defaultdict + +MARKER = "SOFTFAIL " + +# Prefixes of go test status/control lines that the framework emits between a +# test's own output and its terminating action (e.g. " --- PASS: TestAccept/x"). +# The unified diff's own "--- " / "+++ " headers lack the colon, so +# they are not mistaken for these. +CONTROL_PREFIXES = ("--- PASS:", "--- FAIL:", "--- SKIP:", "--- BENCH:", "=== ") + + +def collect_softfails(events): + """Group SOFTFAIL diff blocks by test from `go test -json` output events. + + Each SOFTFAIL marker line opens a block that captures the unified diff that + follows it, closing on the next non-output action or go test status line so the + "--- PASS:" the framework emits for a passing soft-failed test stays out of the diff. + + >>> evs = [ + ... {"Action": "output", "Test": "TestAccept/x", "Output": " foo.go:1: SOFTFAIL out.drifty.txt\\n"}, + ... {"Action": "output", "Test": "TestAccept/x", "Output": " --- a\\n"}, + ... {"Action": "output", "Test": "TestAccept/x", "Output": " +++ b\\n"}, + ... {"Action": "output", "Test": "TestAccept/x", "Output": " --- PASS: TestAccept/x (0.50s)\\n"}, + ... {"Action": "pass", "Test": "TestAccept/x"}, + ... ] + >>> result = collect_softfails(evs) + >>> result["TestAccept/x"][0][0] + 'out.drifty.txt' + >>> "PASS" in "".join(result["TestAccept/x"][0][1]) + False + """ + blocks = defaultdict(list) + open_block = {} + for ev in events: + if ev.get("Action") != "output": + open_block.pop(ev.get("Test"), None) + continue + test = ev.get("Test") + out = ev.get("Output", "") + idx = out.find(MARKER) + if idx != -1: + relpath = out[idx + len(MARKER) :].strip() + block = [relpath, []] + blocks[test].append(block) + open_block[test] = block + elif test in open_block: + if out.lstrip().startswith(CONTROL_PREFIXES): + open_block.pop(test) + continue + open_block[test][1].append(out) + return blocks + + +def render(blocks): + """Render collected SOFTFAIL blocks as a markdown report.""" + if not blocks: + return "No soft-failed acceptance goldens drifted in this run.\n" + + total = sum(len(v) for v in blocks.values()) + lines = [ + f"## Soft-failed acceptance goldens ({total})", + "", + "These goldens drifted but did not fail the build. Refresh with `./task test-update`.", + "", + ] + for test in sorted(blocks): + for relpath, diff in blocks[test]: + lines.append(f"
{relpath} — {test}") + lines.append("") + lines.append("```diff") + lines.append("".join(diff).rstrip("\n")) + lines.append("```") + lines.append("
") + lines.append("") + return "\n".join(lines) + + +def read_events(paths): + for path in paths: + with open(path) as f: + for line in f: + line = line.strip() + if not line: + continue + try: + yield json.loads(line) + except json.JSONDecodeError: + continue + + +def main(argv): + if len(argv) < 2: + sys.stderr.write(f"usage: {argv[0]} test-output.json [...]\n") + return 2 + blocks = collect_softfails(read_events(argv[1:])) + sys.stdout.write(render(blocks)) + return 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv))