From 9427dac823beafd67f23726fe8630336d39c9595 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 26 Oct 2026 19:50:05 +0100 Subject: [PATCH] The checker says loudly when a build module hid a failure's cause (issue 1901) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A failed test with no cause, or a passing line as its cause, was worded as if it were the failure: a try of mesh-catalog failed with a passing '✔' line of the first test file. Under mesh-sdk's failure contract the verdict's words and its status line now start with 'the build module hid the failure ( at )', and the check still fails. --- modules/mesh-delivery-checker/go.mod | 2 +- modules/mesh-delivery-checker/go.sum | 4 +- .../internal/check/failure_words_test.go | 8 +-- .../internal/check/judge.go | 35 +++++++++++- .../internal/check/package_cause_test.go | 53 ++++++++++++++++--- .../internal/check/results.go | 15 +++--- 6 files changed, 94 insertions(+), 23 deletions(-) diff --git a/modules/mesh-delivery-checker/go.mod b/modules/mesh-delivery-checker/go.mod index 4e2c71e..3ff92ac 100644 --- a/modules/mesh-delivery-checker/go.mod +++ b/modules/mesh-delivery-checker/go.mod @@ -3,7 +3,7 @@ module mesh-delivery-checker go 1.26.0 require ( - git.novox.be/novox/mesh-sdk/go v0.1.14-0.20261026142434-65ee43f07583 + git.novox.be/novox/mesh-sdk/go v0.1.14-0.20261026184535-e40dd0f56a78 github.com/nats-io/nats-server/v2 v2.11.17 github.com/nats-io/nats.go v1.54.0 ) diff --git a/modules/mesh-delivery-checker/go.sum b/modules/mesh-delivery-checker/go.sum index e793069..e6fb97c 100644 --- a/modules/mesh-delivery-checker/go.sum +++ b/modules/mesh-delivery-checker/go.sum @@ -1,5 +1,5 @@ -git.novox.be/novox/mesh-sdk/go v0.1.14-0.20261026142434-65ee43f07583 h1:ikD/ggf9/5k23sxJOX1PFcGzb4M3+QeC66AT0MyQ9Zs= -git.novox.be/novox/mesh-sdk/go v0.1.14-0.20261026142434-65ee43f07583/go.mod h1:ouERNpZizzwZ0HLEtAypsY9FR20FyUgUYRni6J7kVCY= +git.novox.be/novox/mesh-sdk/go v0.1.14-0.20261026184535-e40dd0f56a78 h1:7CdODXGPk15cuLyQfXI8HbiSbUYIvNAa1hwQ8laYxPE= +git.novox.be/novox/mesh-sdk/go v0.1.14-0.20261026184535-e40dd0f56a78/go.mod h1:ouERNpZizzwZ0HLEtAypsY9FR20FyUgUYRni6J7kVCY= github.com/antithesishq/antithesis-sdk-go v0.7.0-default-no-op h1:Z/MZK75wC/NSrkgqeNIa7jexam9uWzhLmFTSCPI/kn0= github.com/antithesishq/antithesis-sdk-go v0.7.0-default-no-op/go.mod h1:FQyySiasQQM8735Ddel3MRojmy4dA1IqCeyJ5jmPMbI= github.com/google/go-tpm v0.9.8 h1:slArAR9Ft+1ybZu0lBwpSmpwhRXaa85hWtMinMyRAWo= diff --git a/modules/mesh-delivery-checker/internal/check/failure_words_test.go b/modules/mesh-delivery-checker/internal/check/failure_words_test.go index ef95b47..e12369e 100644 --- a/modules/mesh-delivery-checker/internal/check/failure_words_test.go +++ b/modules/mesh-delivery-checker/internal/check/failure_words_test.go @@ -42,7 +42,7 @@ func TestTheStatusLineNamesTheFailureNotTheFirstLoggedLine(t *testing.T) { // 19 f := fatalAfterLog() f.Cause = "" v = levelFailure(&target{}, "unit", Tested{Failed: []Failure{f}}) - if !strings.HasPrefix(v.Failed.Words, "agreement_catalogue_test.go:130") { + if !strings.HasPrefix(afterHid(v.Failed.Words), "agreement_catalogue_test.go:130") { t.Errorf("words = %q", v.Failed.Words) } } @@ -64,10 +64,10 @@ func TestAnOlderBuildModulesFailureIsAcceptedWithItsWholeOutput(t *testing.T) { t.Fatalf("1901/AC3: err = %v, cause = %q", err, f.Cause) } v := levelFailure(&target{}, "unit", Tested{Failed: []Failure{f}}) - if v.Failed.Words != "x_test.go:1: logged\nx_test.go:2: the old kept lines" { + if afterHid(v.Failed.Words) != "x_test.go:1: logged\nx_test.go:2: the old kept lines" { t.Errorf("1901/AC3: words = %q", v.Failed.Words) } - if got := firstFailed(*v); !strings.Contains(got, "logged") || strings.Contains(got, "\n") { - t.Errorf("1901/AC3: status = %q", got) + if got := firstFailed(*v); !strings.Contains(got, "the build module hid the failure") || strings.Contains(got, "\n") { + t.Errorf("1901/AC3, AC8: status = %q", got) } } diff --git a/modules/mesh-delivery-checker/internal/check/judge.go b/modules/mesh-delivery-checker/internal/check/judge.go index f493289..f104530 100644 --- a/modules/mesh-delivery-checker/internal/check/judge.go +++ b/modules/mesh-delivery-checker/internal/check/judge.go @@ -654,6 +654,12 @@ const MaxWords = 4096 // no cause told apart (an older build module) the cause is the output's first "panic:", "fatal error:" or "signal:" // line, read before the output is cut, or the words say the tests passed and the program failed after them. func failureWords(f Failure) string { + return failureWordsAfter(f, "") +} + +// failureWordsAfter is failureWords with the line loud put first, within the same MaxWords: what the checker itself says of +// the failure (that the build module hid it), which the status line then names. +func failureWordsAfter(f Failure, loud string) string { out, passed := dropSummary(strings.TrimSpace(f.Output)) cause := strings.TrimSpace(f.Cause) if cause == "" { @@ -669,11 +675,14 @@ func failureWords(f Failure) string { if len(cause) > MaxCause { cause = strings.ToValidUTF8(cause[:MaxCause], "") + "..." } - // The words keep MaxWords exactly: the cause and its newline, the marker and the kept lines (review round 1). + // The words keep MaxWords exactly: the loud line, the cause and their newlines, the marker and the kept lines. budget := MaxWords if cause != "" { budget -= len(cause) + 1 } + if loud != "" { + budget -= len(loud) + 1 + } if len(out) > budget { const marker = "... " tail := out[len(out)-(budget-len(marker)):] @@ -686,9 +695,31 @@ func failureWords(f Failure) string { if cause != "" && !strings.HasPrefix(firstLine(out), cause) { out = strings.TrimRight(cause+"\n"+out, "\n") } + if loud != "" { + out = strings.TrimRight(loud+"\n"+out, "\n") + } return out } +// hidWords is the checker's loud line for a failure the build module sent hiding its cause (issue 1901, its thorough +// fix): with no cause, or a passing line as its cause, under the failure contract (buildstep.Hidden). It names the build +// module and the version it says it is (Tested.By, " with mesh-sdk "); one from before the contract +// says nothing of itself. "" when the failure keeps the contract. The check fails either way: only its words change. +func hidWords(module, by string, f Failure) string { + why := buildstep.Hidden(f) + if why == "" { + return "" + } + version := "a version before the failure contract" + if module == "" { + module = "an unnamed build module" + } + if m, v, ok := strings.Cut(by, " with "); ok && m != "" { + module, version = m, v + } + return fmt.Sprintf("the build module hid the failure (%s at %s): %s", module, version, why) +} + // MaxCause is the most a cause in a failure's words holds: one long line (the 1901 review: a cause of a kilobyte could // push the words past MaxWords). const MaxCause = 1024 @@ -775,7 +806,7 @@ func levelFailure(t *target, level string, results ...Tested) *delivery.Verdict if t.module == buildrun.NodeBuildModule && f.Package == "" { name = res.Module } - all = append(all, delivery.FailedFirst{Test: level + ": " + name, Words: failureWords(f)}) + all = append(all, delivery.FailedFirst{Test: level + ": " + name, Words: failureWordsAfter(f, hidWords(t.module, res.By, f))}) } } if len(all) == 0 { // no result: a level said failed with nothing to name diff --git a/modules/mesh-delivery-checker/internal/check/package_cause_test.go b/modules/mesh-delivery-checker/internal/check/package_cause_test.go index 344fed8..a1282c9 100644 --- a/modules/mesh-delivery-checker/internal/check/package_cause_test.go +++ b/modules/mesh-delivery-checker/internal/check/package_cause_test.go @@ -3,6 +3,8 @@ package check import ( "strings" "testing" + + "git.novox.be/novox/mesh-sdk/go/delivery" ) // What the build module before this fix sent for two packages that failed after their tests passed (issue 1901, its third @@ -38,11 +40,11 @@ func TestAPackageFailureIsNeverWordedPass(t *testing.T) { // 1901/AC5 t.Errorf("1901/AC5: the words keep %q: %q", l, v.Failed.Words) } } - if !strings.HasPrefix(v.Failed.Words, c.want) { - t.Errorf("1901/AC5: words start %q, want %q", firstLine(v.Failed.Words), c.want) + if !strings.HasPrefix(afterHid(v.Failed.Words), c.want) { + t.Errorf("1901/AC5: words after the loud line start %q, want %q", firstLine(afterHid(v.Failed.Words)), c.want) } - if got := firstFailed(*v); !strings.Contains(got, c.want) || strings.HasSuffix(got, ": PASS") { - t.Errorf("1901/AC5: status = %q", got) + if got := firstFailed(*v); !strings.Contains(got, "the build module hid the failure") || strings.HasSuffix(got, ": PASS") { + t.Errorf("1901/AC5, AC8: status = %q", got) } }) } @@ -63,7 +65,7 @@ func TestALongCauseIsCutToALinesBound(t *testing.T) { // 1901/AC5 func TestAPackageStoppedWithOnlyItsFailLineIsStillWorded(t *testing.T) { // 1901/AC5 for _, out := range []string{"FAIL\texample.com/timeout\t2.017s", "FAIL\texample.com/exiting\t0.006s", "FAIL\texample.com/killed\t0.006s"} { v := levelFailure(&target{}, "integration", Tested{Failed: []Failure{{Package: "example.com/x", Output: out}}}) - if !strings.HasPrefix(v.Failed.Words, "the package failed with no test to name and no panic, signal or build error in its output") { + if !strings.HasPrefix(afterHid(v.Failed.Words), "the package failed with no test to name and no panic, signal or build error in its output") { t.Errorf("1901/AC5: %q is worded %q", out, v.Failed.Words) } } @@ -74,7 +76,7 @@ func TestAPackageStoppedWithOnlyItsFailLineIsStillWorded(t *testing.T) { // 1901 func TestOnlyGosExactFormsAreSummaryOrCause(t *testing.T) { // 1901/AC5 out := " x_test.go:3: ok so far\nok then: the seat is free\n panic: child retried and recovered\npanic: the real one\nFAIL\tp\t1.0s" v := levelFailure(&target{}, "unit", Tested{Failed: []Failure{{Package: "p", Output: out}}}) - if !strings.HasPrefix(v.Failed.Words, "panic: the real one") || !strings.Contains(v.Failed.Words, "ok then: the seat is free") { + if !strings.HasPrefix(afterHid(v.Failed.Words), "panic: the real one") || !strings.Contains(v.Failed.Words, "ok then: the seat is free") { t.Errorf("1901/AC5: words = %q", v.Failed.Words) } } @@ -90,3 +92,42 @@ func TestTheWordsKeepTheirBoundExactly(t *testing.T) { // 1901/AC5 } } } + +// afterHid is a verdict's words after the checker's loud line, which a failure with no cause from an older build module +// now starts with (1901/AC8). +func afterHid(w string) string { + if strings.HasPrefix(w, "the build module hid the failure") { + _, rest, _ := strings.Cut(w, "\n") + return rest + } + return w +} + +// A failure that hides its cause (none, or a passing line) is said loudly first in the words and in the status line, +// naming the build module and the version it says it is, and the level still fails; a failure that keeps the contract +// is worded as before (1901/AC8). +func TestAFailureThatHidesItsCauseIsSaidLoudly(t *testing.T) { // 1901/AC8 + by := "mesh-build-node with mesh-sdk v0.1.14-0.20261026184535-e40dd0f56a78" + for _, c := range []struct { + name, module, by, cause, output, loud string + }{ + {"no cause", "mesh-build-node", by, "", "test/b.test.ts: something", "the build module hid the failure (mesh-build-node at mesh-sdk v0.1.14-0.20261026184535-e40dd0f56a78): it sent no cause"}, + {"a passing line as its cause", "mesh-build-node", by, "\u2714 the same merge has the same id (8.006913ms)", "", "the build module hid the failure (mesh-build-node at mesh-sdk v0.1.14-0.20261026184535-e40dd0f56a78): its cause is a passing line"}, + {"from before the contract", "mesh-build-node", "", "", "\u2714 the same merge has the same id (8.006913ms)", "the build module hid the failure (mesh-build-node at a version before the failure contract): it sent no cause"}, + {"a real cause", "mesh-build-node", by, "one plus one is not three", "not ok 3 - fails", ""}, + } { + t.Run(c.name, func(t *testing.T) { + v := levelFailure(&target{module: c.module}, "unit", Tested{By: c.by, Failed: []Failure{{Package: "modules/gitea", Test: "fails", Cause: c.cause, Output: c.output}}}) + if v.State != delivery.VerdictFail { + t.Errorf("1901/AC8: the verdict is %q, want it failed", v.State) + } + line := firstFailed(*v) + switch { + case c.loud == "" && strings.Contains(v.Failed.Words, "hid the failure"): + t.Errorf("1901/AC8: a failure that keeps the contract is said hidden: %q", v.Failed.Words) + case c.loud != "" && (!strings.HasPrefix(v.Failed.Words, c.loud) || !strings.Contains(line, c.loud) || !strings.Contains(v.Failures[0].Words, "hid the failure")): + t.Errorf("1901/AC8: words %q, status %q, want both to start with %q", v.Failed.Words, line, c.loud) + } + }) + } +} diff --git a/modules/mesh-delivery-checker/internal/check/results.go b/modules/mesh-delivery-checker/internal/check/results.go index bd61b15..7fc8ee6 100644 --- a/modules/mesh-delivery-checker/internal/check/results.go +++ b/modules/mesh-delivery-checker/internal/check/results.go @@ -4,6 +4,7 @@ import ( "encoding/json" "strings" + "git.novox.be/novox/mesh-sdk/go/buildstep" "git.novox.be/novox/mesh-sdk/go/delivery" ) @@ -58,14 +59,9 @@ type GoPackage struct { Skipped int `json:"skipped,omitempty"` } -// Failure is a test that failed, or a package that failed with no test to name. -type Failure struct { - Package string `json:"package"` - Test string `json:"test,omitempty"` - Output string `json:"output,omitempty"` - // Cause is the first line of what the test reported as its error, when the build module told it apart (issue 1901). - Cause string `json:"cause,omitempty"` -} +// Failure is a test that failed, or a package that failed with no test to name: the build modules' failure contract +// (buildstep.Failure, issue 1901), its cause never empty and never a passing line from a build module that keeps it. +type Failure = buildstep.Failure // TestTime is one test and the seconds it took. type TestTime struct { @@ -98,6 +94,9 @@ type Tested struct { // JudgedBy is the program that ran the step (ADR 0535 §5): "installed", or "candidate " under runner // workspace; absent from a build module that does not take the runner. JudgedBy string `json:"judged_by,omitempty"` + // By is the build module that answered and the SDK it was built with (buildstep.By); absent from a build module from + // before the failure contract. + By string `json:"by,omitempty"` } // Digested is the answer of the Go build module's digest step: one digest per package that has tests at the level, by import