acc: soft-fail acceptance goldens that drift outside our control#5931
Open
janniklasrose wants to merge 10 commits into
Open
acc: soft-fail acceptance goldens that drift outside our control#5931janniklasrose wants to merge 10 commits into
janniklasrose wants to merge 10 commits into
Conversation
Some acceptance 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 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: - Per-file and opt-in, surfaced in out.test.toml so reviewers see the shield on every PR. output.txt can never be shielded (a hard config error), so a regression in our own logic still turns the test red. - Requires Badness, documenting why each file is shielded. - Only a content diff is downgraded; panics, unexpected exit codes, and missing/extra files stay hard failures. - The doComparison branch runs before AssertEqualTexts, whose testify side effect would otherwise mark the test failed. Drift is reported to the GitHub step summary via tools/softfail_report.py (greps the gotestsum JSON for SOFTFAIL markers) so oncall can refresh the golden with ./task test-update on a cadence without the build turning red. Co-authored-by: Isaac
Some tests depend end-to-end on a runtime-fetched artifact outside our control, so an upstream change breaks not just captured output but the run itself. For those there is no volatility seam to split into a per-file SoftFailFiles shield: output.txt is the drift. Add a whole-test SoftFail flag that downgrades a content diff in ANY of the test's goldens (including output.txt) to a non-blocking SOFTFAIL marker. It is a blunt instrument, gated on Badness and intended only for tests whose CLI behavior is also covered by a hermetic local test, so shielding the e2e test can't hide a real regression. Structural failures (panics, unexpected/missing files) stay hard. Co-authored-by: Isaac
This test initializes and deploys the mlops-stacks template fetched from github at runtime, which is updated out of band. An upstream change breaks not just the captured output but the deploy itself, and there is no volatility seam to isolate: output.txt is the drift. bundle init and bundle deploy are covered by hermetic local tests, so shielding this e2e test cannot hide a real CLI regression. Co-authored-by: Isaac
…mote view The detect-change test dumps the full `bundle plan -o json` into one golden. That plan mixes our planning decisions (action/reason/old/new) with the remote view: remote_state (the full backend read) and each change's `remote` value. On cloud the remote view mirrors the dashboards API response, whose output-only fields (create_time, update_time, etag, lifecycle_state, path, ...) are populated by the backend and change out of our control, so the whole golden drifts red for reasons unrelated to the CLI. Split along that volatility seam: the planning decisions stay strict in out.plan.$engine.json, and the remote view is projected into its own out.plan.remote.$engine.json. Only the direct-engine remote file is listed in SoftFailFiles (terraform exposes no remote view here; its file is structurally empty and stays strict, so a regression that populated it would turn the test red). Co-authored-by: Isaac
Requiring Badness whenever SoftFailFiles/SoftFail is set overloaded an unrelated documentary field and coupled two independent concepts. The shield is already self-documenting: a comment in test.toml explains why the file drifts, and the flag is surfaced in out.test.toml for reviewers. Remove the requirement (no SoftFailReason replacement — it isn't needed). The structural guards stay: output.txt can never be shielded and per-file entries must start with "out". Restore the mlops-stacks Badness to its original text (it documents the template's validation warnings, unrelated to soft-fail). Co-authored-by: Isaac
janniklasrose
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
janniklasrose
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
janniklasrose
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
janniklasrose
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
…ff report Two review fixes: - Ignore SoftFailFiles/SoftFail on local runs. A local testserver run is deterministic and cannot drift, so a diff there is a real regression. The shield now takes effect only when CLOUD_ENV is set, so a Local+Cloud test (e.g. dashboards/detect-change) keeps its local golden strict while still shielding cloud API drift. - Close a SOFTFAIL diff block on go test status lines (--- PASS:, etc.) in softfail_report.py. go test -json emits the "--- PASS:" status line as an output event before the pass action, so it was landing inside the reported diff block. The unified diff's own "--- <path>" headers lack the colon and are not affected. Co-authored-by: Isaac
janniklasrose
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
janniklasrose
requested review from
andrewnester,
denik,
pietern and
shreyas-goenka
Contributor
Approval status: pending
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Some acceptance 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 (mlops-stacks) is updated upstream. None are regressions, yet each turns PRs and nightlies red. This adds a way to downgrade such drift to a recorded, non-blocking
SOFTFAILmarker while keeping real regressions red.What
Four commits, smallest blast radius first:
Per-file
SoftFailFiles— opt-in shield for named goldens.output.txtcan never be shielded (a hard config error), entries must start without, andBadnessis required. ThedoComparisonhook emitsSOFTFAIL <file>+ unified diff viat.Logfand returns green, beforeAssertEqualTexts(whose testify side effect would otherwise mark the test failed). Content-diff only: panics, unexpected exit codes, and missing/extra files stay hard. Drift is reported to the GitHub step summary viatools/softfail_report.py(greps the gotestsum JSON), so it's visible even though the build is green.Whole-test
SoftFail— a blunt shield covering every golden includingoutput.txt, for tests whose behavior end-to-end depends on a runtime-fetched artifact (so an upstream change breaks the run itself, not just captured output). Only appropriate when the CLI behavior under test is also covered by a hermetic local test. Also gated onBadness.mlops-stacks opt-out — applies whole-test
SoftFailtobundle/deploy/mlops-stacks(fetches + deploys the mlops-stacks template from github at runtime).bundle init/bundle deployare covered by local tests.dashboards/detect-change split — demonstrates the preferred per-file approach: split
bundle plan -o jsonalong a volatility seam. Planning decisions (action/reason/old/new) stay strict inout.plan.$engine.json; the backend-mirroring remote view (remote_state+ each change'sremote) is projected into a soft-failedout.plan.remote.direct.json.Toolbox ordering
Soft-fail is the last resort. Reach for
Replsmasks and field projection first (both keep the golden strict); useSoftFailFilesonly for undescribable drift, and whole-testSoftFailonly when no seam can be isolated. Seeacceptance/README.md.Testing
TestValidateSoftFailFiles(Badness required,output.txt/non-outrejected, whole-test variant).TestSoftFailComparisondrivesdoComparisondirectly (an acceptance selftest can't:-updatewould heal the mismatch). Covers listed-file-green, unlisted/output.txt-still-red, and whole-test-green.This pull request and its description were written by Isaac.