diff --git a/lab/internal/molecule/accept_protected_failures.go b/lab/internal/molecule/accept_protected_failures.go index 00e1bd9..99a088a 100644 --- a/lab/internal/molecule/accept_protected_failures.go +++ b/lab/internal/molecule/accept_protected_failures.go @@ -322,7 +322,8 @@ func failingProtectedChecks(p readiness.Plan, result verification.Result) ([]fai // result stopped on and its observation. The protected view must have failed for // that check's own exit, not for setup, a definition change or a source mutation. // An earlier check may have failed only where the view accepted it under declared -// deltas. +// deltas. The checks that ran must be the plan's regression checks in plan order, up to +// the one it stopped on, less the covered checks protectedRanChecks leaves out. func failingProtectedCheck(p readiness.Plan, result verification.Result) (readiness.Check, verification.Observation, error) { v := result.Protected if v == nil || v.RequiredChecksPassed { @@ -369,8 +370,10 @@ func failingProtectedCheck(p readiness.Plan, result verification.Result) (readin // protectedRanChecks returns the observations of protected view v that ran on their own, // in plan order, and the plan's regression checks they are matched against. A targeted // check the view recorded as covered by a baseline check (a CoveredBy observation in -// Checks, or a not-run record when that baseline failed) never ran, so it is left out of -// both; each such record must name the covering check the plan itself gives it. +// Checks, or a CoveredBy not-run record when that baseline failed or an earlier required +// check stopped the view before it) never ran on its own, so it is left out of both, +// even when plan order lists it before the check the view stopped on; each such record +// must name the covering check the plan itself gives it. func protectedRanChecks(p readiness.Plan, v *verification.Result) ([]verification.Observation, []readiness.Check, error) { planned := map[string]readiness.Check{} for _, c := range p.Analysis.Checks { diff --git a/lab/internal/molecule/accept_protected_failures_test.go b/lab/internal/molecule/accept_protected_failures_test.go index 1fb072c..3b3502e 100644 --- a/lab/internal/molecule/accept_protected_failures_test.go +++ b/lab/internal/molecule/accept_protected_failures_test.go @@ -3,6 +3,7 @@ package molecule import ( "bytes" "context" + "encoding/json" "os" "path/filepath" "reflect" @@ -123,6 +124,25 @@ func acceptBaselineFixture(t *testing.T, mode string) (Options, Record) { } func acceptFixtureFiles(t *testing.T, mode string, tests map[string]string, baseline bool, argv []string) (Options, Record) { + t.Helper() + return acceptFixtureWith(t, mode, tests, nil, func(a *readiness.Analysis, manifest string) { + a.Checks = append(a.Checks, acceptRegressionCheck(acceptCheck, manifest, "Module tests", argv...)) + if baseline { + a.Scope = append(a.Scope, readiness.ScopedPath{Path: acceptOtherTest, Reason: "Another test of the package", Evidence: []string{"issue"}}) + a.Checks = append(a.Checks, acceptRegressionCheck(acceptBaseline, manifest, "Formula baseline", "go", "test", "-timeout", "20m", "./...")) + } + }) +} + +// acceptRegressionCheck is a required regression check in lab defined by its manifest. +func acceptRegressionCheck(id, manifest, reason string, argv ...string) readiness.Check { + return readiness.Check{ID: id, Category: "regression", Argv: append([]string{}, argv...), Dir: "lab", TimeoutMS: fixtureCheckTimeoutMS, Required: true, Definitions: []string{manifest}, Reason: reason, Evidence: []string{"issue", manifest}} +} + +// acceptFixtureWith runs the accept fixture: prepare, when set, changes the source +// repository or the approved Formula before the pinned test is committed, and extend +// adds the regression checks (and any scope) to the analysis after the pinned test's. +func acceptFixtureWith(t *testing.T, mode string, tests map[string]string, prepare func(o Options, repo string), extend func(a *readiness.Analysis, manifest string)) (Options, Record) { t.Helper() bin := resolvedTempDir(t) if err := writeStub(filepath.Join(bin, "go"), []byte(acceptFakeGo), 0700); err != nil { @@ -131,6 +151,9 @@ func acceptFixtureFiles(t *testing.T, mode string, tests map[string]string, base setAcceptMode(t, bin, mode) t.Setenv("PATH", bin+string(os.PathListSeparator)+os.Getenv("PATH")) o, repo := fixture(t) + if prepare != nil { + prepare(o, repo) + } files := map[string]string{"lab/go.mod": "module example.test/lab\n"} for file, body := range tests { files[file] = body @@ -159,11 +182,7 @@ func acceptFixtureFiles(t *testing.T, mode string, tests map[string]string, base } } a.Scope = append(a.Scope, readiness.ScopedPath{Path: acceptTest, Reason: "The pinned behaviour changes by design", Evidence: []string{"issue"}}) - a.Checks = append(a.Checks, readiness.Check{ID: acceptCheck, Category: "regression", Argv: append([]string{}, argv...), Dir: "lab", TimeoutMS: fixtureCheckTimeoutMS, Required: true, Definitions: []string{manifest}, Reason: "Module tests", Evidence: []string{"issue", manifest}}) - if baseline { - a.Scope = append(a.Scope, readiness.ScopedPath{Path: acceptOtherTest, Reason: "Another test of the package", Evidence: []string{"issue"}}) - a.Checks = append(a.Checks, readiness.Check{ID: acceptBaseline, Category: "regression", Argv: []string{"go", "test", "-timeout", "20m", "./..."}, Dir: "lab", TimeoutMS: fixtureCheckTimeoutMS, Required: true, Definitions: []string{manifest}, Reason: "Formula baseline", Evidence: []string{"issue", manifest}}) - } + extend(&a, manifest) return a, err } r, err := runApprovedFixture(t, o) @@ -501,6 +520,119 @@ func TestAcceptProtectedFailuresSkipsCoveredTargetedChecks(t *testing.T) { } } +// The #403 order: a full-suite check listed first in the plan is covered by the later +// Formula baseline, so the protected view skips it and stops on the targeted check +// after it, which fails by design. The covered check is recorded as not run with +// CoveredBy, so --accept-protected-failures leaves it out of the plan-order prefix +// and declares the targeted check's failure, which the approved checks-only run +// accepts. +func TestAcceptProtectedFailuresAcceptsACoveredFullSuiteListedFirst(t *testing.T) { + const full, targeted = "go-test-all", "pin-tests" + prepare := func(o Options, repo string) { + if err := os.MkdirAll(filepath.Join(repo, ".github", "workflows"), 0700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(repo, filepath.FromSlash(baselineWorkflow)), []byte("name: CI\non: pull_request\njobs: {}\n"), 0600); err != nil { + t.Fatal(err) + } + raw, err := os.ReadFile(filepath.Join(o.LabDir, "formulas", "sample.yaml")) + if err != nil { + t.Fatal(err) + } + entry, _ := json.Marshal(map[string]any{"id": acceptBaseline, "argv": []string{"go", "test", "-timeout", "20m", "./..."}, "dir": "lab", "timeoutMs": fixtureCheckTimeoutMS, "workflow": baselineWorkflow, "reason": "CI runs the module tests."}) + approveFixture(t, o.LabDir, string(raw)+"requiredChecks:\n - "+string(entry)+"\n", "baseline") + } + o, r := acceptFixtureWith(t, "by-design", map[string]string{acceptTest: acceptPinnedBase}, prepare, func(a *readiness.Analysis, manifest string) { + a.Checks = append(a.Checks, + acceptRegressionCheck(full, manifest, "The full suite", "go", "test", "-timeout", "20m", "./..."), + acceptRegressionCheck(targeted, manifest, "The pinned package", "go", "test", "-json", "./pin")) + }) + parent := latestPlan(r) + var regression []string + for _, c := range parent.Plan.Analysis.Checks { + if c.Category == "regression" { + regression = append(regression, c.ID) + } + } + if want := []string{full, targeted, acceptBaseline}; !reflect.DeepEqual(regression, want) || !reflect.DeepEqual(parent.Plan.FormulaBaseline, []string{acceptBaseline}) { + t.Fatalf("regression checks %v baseline %v, want %v", regression, parent.Plan.FormulaBaseline, want) + } + v := r.Verification + if statePhase(r) != "failed" || v.Protected == nil || v.Protected.RequiredChecksPassed || !strings.HasPrefix(v.Protected.Error, "required command "+targeted+" did not pass") { + t.Fatalf("fixture did not fail only in the protected view on %s: %s %+v", targeted, statePhase(r), v.Protected) + } + for _, obs := range v.Protected.Checks { + if obs.ID == full { + t.Fatalf("the covered full suite is among the protected checks: %+v", obs) + } + } + coveredBy := map[string]string{} + for _, obs := range v.Protected.NotRun { + if obs.Result.OK || obs.NotRun != verification.NotRunEarlierFailure { + t.Fatalf("protected not-run record %+v", obs) + } + coveredBy[obs.ID] = obs.CoveredBy + } + if want := map[string]string{acceptBaseline: "", full: acceptBaseline}; !reflect.DeepEqual(coveredBy, want) { + t.Fatalf("protected not-run records %v, want %v", coveredBy, want) + } + tree := r.Capture.Tree + r, err := AmendFromLatest(context.Background(), o.LabDir, r.ID, AmendRequest{AcceptProtectedFailures: true}) + if err != nil { + t.Fatalf("the by-design failure after a covered full suite was refused: %v", err) + } + got := latestPlan(r) + if got.Plan.Continuation != "checks-only" || !reflect.DeepEqual(got.Plan.Analysis.Checks, parent.Plan.Analysis.Checks) { + t.Fatalf("the amendment changed the checks or continuation %q", got.Plan.Continuation) + } + deltas := got.Plan.ExpectedProtectedDeltas + if len(deltas) != 1 || deltas[0].CheckID != targeted || deltas[0].Package != acceptPackage || deltas[0].Test != "TestX" || deltas[0].ScopePath != acceptTest || deltas[0].DeltaScope != readiness.DeltaScopeIdentity { + t.Fatalf("deltas %+v", deltas) + } + o.Approve = got.Digest + if r, err = Run(context.Background(), o); err != nil { + t.Fatal(err) + } + v = r.Verification + if r.Readiness.Phase != "review-waiting" || v == nil || !v.RequiredChecksPassed || len(r.HarnessAttempts) != 1 || r.Capture.Tree != tree || len(v.AcceptedDeltas) == 0 { + t.Fatalf("checks-only did not accept the declared failure: phase %s %+v", r.Readiness.Phase, v) + } + for _, d := range v.AcceptedDeltas { + if d.Test != "TestX" || d.Package != acceptPackage { + t.Fatalf("accepted deltas %+v", v.AcceptedDeltas) + } + } +} + +// TestAcceptProtectedFailuresLeavesOutACheckCoveredByANotRunBaseline: a covered check +// listed before the check the protected view stopped on is left out of the plan-order +// prefix only through a CoveredBy record the plan backs. Without one, as a record +// written before CoveredBy was kept, the run is still refused. +func TestAcceptProtectedFailuresLeavesOutACheckCoveredByANotRunBaseline(t *testing.T) { + t.Parallel() + tree, derived := strings.Repeat("a", 40), strings.Repeat("b", 40) + full := readiness.Check{ID: "go-test-all", Category: "regression", Argv: []string{"go", "test", "-timeout", "20m", "./..."}, Dir: "lab", Required: true} + targeted := readiness.Check{ID: acceptCheck, Category: "regression", Argv: []string{"go", "test", "-json", "./pin"}, Dir: "lab", Required: true} + baseline := readiness.Check{ID: acceptBaseline, Category: "regression", Argv: []string{"go", "test", "-timeout", "20m", "./..."}, Dir: "lab", Required: true} + p := readiness.Plan{FormulaBaseline: []string{baseline.ID}, Analysis: readiness.Analysis{Checks: []readiness.Check{full, targeted, baseline}}} + failing := verification.Observation{ID: targeted.ID, Category: "regression", SourceTree: derived, Command: targeted, Result: process.Result{Reconciled: true, Code: 1}} + notRun := func(c readiness.Check, coveredBy string) verification.Observation { + return verification.Observation{ID: c.ID, Category: "regression", SourceTree: derived, Command: c, CoveredBy: coveredBy, NotRun: verification.NotRunEarlierFailure, Result: process.Result{Reconciled: true, Code: -1}} + } + result := func(covered verification.Observation) verification.Result { + return verification.Result{SourceTree: tree, Protected: &verification.Result{SourceTree: derived, Setup: []verification.Observation{}, Checks: []verification.Observation{failing}, NotRun: []verification.Observation{notRun(baseline, ""), covered}, Error: "required command " + targeted.ID + " did not pass"}} + } + if c, _, err := failingProtectedCheck(p, result(notRun(full, baseline.ID))); err != nil || c.ID != targeted.ID { + t.Fatalf("a covered check left unrun by an earlier stop: %v %q", err, c.ID) + } + if _, _, err := failingProtectedCheck(p, result(notRun(full, ""))); err == nil || !strings.Contains(err.Error(), "observation 1 does not match the approved check") { + t.Fatalf("a not-run record without CoveredBy was left out of the prefix: %v", err) + } + if _, _, err := failingProtectedCheck(p, result(notRun(full, "go-vet"))); err == nil || !strings.Contains(err.Error(), "does not cover it with") { + t.Fatalf("coverage the plan does not give was accepted: %v", err) + } +} + func TestAcceptProtectedFailuresRequestValidation(t *testing.T) { t.Parallel() o, r := amendFixture(t) diff --git a/lab/internal/verification/order.go b/lab/internal/verification/order.go index 55e0e0b..b3c034d 100644 --- a/lab/internal/verification/order.go +++ b/lab/internal/verification/order.go @@ -80,7 +80,10 @@ func formatOnlyScript(script string) bool { // keeping plan order within a tier. The protected view keeps plan order: it runs only // once the candidate view's cheap checks passed (or beside it, stopped by its failure), // and --accept-protected-failures reads its run as a plan-order prefix ending at the -// check it stopped on. +// check it stopped on. A targeted check a baseline covers is skipped in that order and +// recorded with CoveredBy naming the baseline: as covered once the baseline ran, or as +// not run when the baseline failed or an earlier required check stopped the view first. +// The prefix leaves every such record out, wherever plan order lists the check. func executionOrder(commands []readiness.Check, setup int, protected bool) []int { order := make([]int, len(commands)) for i := range order { diff --git a/lab/internal/verification/order_test.go b/lab/internal/verification/order_test.go index 9a71659..7ff49bf 100644 --- a/lab/internal/verification/order_test.go +++ b/lab/internal/verification/order_test.go @@ -359,6 +359,62 @@ func TestCheckCoveredByAFailingBaselineIsNotRun(t *testing.T) { } } +// TestCheckCoveredByANotRunBaselineKeepsCoveredBy: the #403 order. A full-suite check +// the Formula baseline covers is listed first and a cheaper targeted check after it +// fails in the protected view before the baseline runs. The covered check is listed in +// NotRun as not run, with CoveredBy naming the baseline, never among the checks and +// never as evidence. The candidate view runs every check, cheapest first, as before. +func TestCheckCoveredByANotRunBaselineKeepsCoveredBy(t *testing.T) { + bin, dir, p, c := orderFixture(t, "by-design") + full := p.Analysis.Checks[1] + full.ID, full.Reason = "go-test-all", "The full suite" + pinned := p.Analysis.Checks[0] + pinned.ID, pinned.Argv = "pinned", []string{"go", "test", "-run", "TestA", "./pkg/..."} + p.Analysis.Checks = []readiness.Check{full, pinned, p.Analysis.Checks[1]} + if err := p.Validate(); err != nil { + t.Fatal(err) + } + if id, ok := CoveringCheckID(p, full); !ok || id != baselineCheck { + t.Fatalf("the full suite is not covered by the baseline: %q %v", id, ok) + } + v := runDelta(t, dir, p, c) + if got := ids(v.Checks); !reflect.DeepEqual(got, []string{full.ID, pinned.ID, baselineCheck}) || len(v.NotRun) != 0 || v.CandidateError != "" { + t.Fatalf("candidate checks %v not run %v error %q", got, ids(v.NotRun), v.CandidateError) + } + for _, o := range v.Checks { + if !o.Result.OK || o.CoveredBy != "" { + t.Fatalf("candidate record %+v", o) + } + } + if v.RequiredChecksPassed || v.Protected == nil || !strings.HasPrefix(v.Protected.Error, "required command pinned did not pass") { + t.Fatalf("protected verdict %q", v.Error) + } + if got := ids(v.Protected.Checks); !reflect.DeepEqual(got, []string{pinned.ID}) { + t.Fatalf("protected checks %v", got) + } + if got := ids(v.Protected.NotRun); !reflect.DeepEqual(got, []string{baselineCheck, full.ID}) { + t.Fatalf("protected not run %v", got) + } + baseline, covered := v.Protected.NotRun[0], v.Protected.NotRun[1] + if baseline.NotRun != NotRunEarlierFailure || baseline.CoveredBy != "" || baseline.Result.OK { + t.Fatalf("baseline record %+v", baseline) + } + if covered.NotRun != NotRunEarlierFailure || covered.CoveredBy != baselineCheck || covered.Result.OK || covered.Result.Stdout != "" || !reflect.DeepEqual(covered.Command, full) { + t.Fatalf("covered record %+v", covered) + } + if !validCoverage(p, full, covered) { + t.Fatal("the covered record does not name the plan's covering baseline") + } + if MatchesCheckObservation(p, full, covered, v.Protected.SourceTree, true) || MatchesCheckObservation(p, full, covered, v.Protected.SourceTree, false) { + t.Fatal("a covered check left unrun by an earlier stop matched") + } + // The candidate view ran all three checks cheapest first; the protected view ran the + // targeted check alone. + if got := argvLog(t, bin); len(got) != 4 || got[0] != "test -run TestA ./pkg/..." || got[1] != "test -timeout 20m ./..." || got[2] != "test -timeout 20m ./..." || strings.Contains(got[3], "./...") { + t.Fatalf("argv %q", got) + } +} + // TestCoverageNeverCrossesANestedModule: go test ./... skips a nested module, so a // targeted package in one, or below a directory holding one, is never covered by the // baseline; a package in the module itself still is. diff --git a/lab/internal/verification/run.go b/lab/internal/verification/run.go index 5ab3a81..983a445 100644 --- a/lab/internal/verification/run.go +++ b/lab/internal/verification/run.go @@ -797,7 +797,10 @@ func runViewDeltas(ctx context.Context, source, destination string, p readiness. checkIndex = append(checkIndex, i) } } - // notRun records every command in rest, and the checks those cover, as never run. + // notRun records every command in rest, and the checks those cover, as never run. A + // covered check keeps CoveredBy naming its baseline, wherever it sits in plan order, + // so --accept-protected-failures leaves it out of the plan-order prefix as it does + // when the baseline itself failed; a not-run record is never delivery evidence. notRun := func(rest []int, reason string) { for _, j := range rest { if covered[j] { @@ -805,7 +808,9 @@ func runViewDeltas(ctx context.Context, source, destination string, p readiness. } v.NotRun = append(v.NotRun, notRunObservation(commands[j], categoryOf(j), c.Tree, reason)) for _, k := range covers[j] { - v.NotRun = append(v.NotRun, notRunObservation(commands[k], categoryOf(k), c.Tree, reason)) + o := notRunObservation(commands[k], categoryOf(k), c.Tree, reason) + o.CoveredBy = commands[j].ID + v.NotRun = append(v.NotRun, o) } } }