Repository navigation
Conversation
Two bugs made a failing subtest render as noise on flakiness.io. failureMessage kept only the first non-framing line of a failed test's output. Go attributes an assertion to the subtest, indented, starting with a bare `x_test.go:796:` line — so the message became just that prefix and the testify Error Trace / expected-vs-actual / diff were dropped, surviving only in stdout. failureError now strips ANSI and go test's framing lines (`=== RUN/PAUSE/CONT/NAME`, `--- PASS:/FAIL:/SKIP:/BENCH:`, matched *with* the colon so testify's `--- Expected` diff lines survive), dedents the per-nesting-level indentation, and keeps the rest. A panic or -timeout dump splits into headline (message) + goroutine trace (stack), which is the spec's message/stack pair. No message at all (bare t.Fail) yields a plain "test failed" instead of echoing the framing back as if it were the error. A parent test was also emitted as a duplicate leaf inside its own suite. A parent fails whenever a subtest fails, but its own output is nothing but `=== RUN` / `--- FAIL` — and failureMessage's fallback turned exactly that framing into the "error". testCarriesSignal now requires a non-passing attempt with output of its own, so a parent that failed only via a subtest no longer produces a leaf; one with its own t.Error/panic/skip still does, as features.md already documented. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H1a5Z6CCsAkJBTfrUnDmuV
Suppressing the parent leaf whenever it had no output of its own could hide a real failure: a parent that fails via a bare t.Fail() while all its subtests pass has nothing else in the report explaining it, and the package already counts as "had tests" so it never becomes an unattributedError either. Emit the leaf in that case; keep suppressing it when a subtest is already carrying the failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H1a5Z6CCsAkJBTfrUnDmuV
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7f28ab8a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if a.status == "" || a.status == report.StatusPassed { | ||
| continue | ||
| } | ||
| if cleanFailureOutput(a.output.String()) != "" { |
There was a problem hiding this comment.
Preserve bare failures on parent tests
When a test runs at least one subtest and then calls t.Fail() or t.FailNow() without logging a message, Go emits only === RUN and --- FAIL: for the parent even if every child passed. cleanFailureOutput therefore returns an empty string here and the failed parent leaf is discarded; because the package has test events, its package failure is not emitted as an unattributed error either, so the resulting report can show only passing children for a failed test run. Distinguish a bare parent failure from a failure propagated from a failed child rather than requiring non-framing output.
Useful? React with 👍 / 👎.
A flaky subtest that fails once and passes on the gotestsum rerun rendered as
noise on flakiness.io: the entity you land on is the parent test, whose only
"error" is
=== RUN/--- FAIL, and the real failing subtest's error istruncated to
browser_context_test.go:796:— the testify Error Trace,expected-vs-actual and diff are nowhere to be seen.
Seen on
playwright-go#31991243631
(
TestBrowserContextClearCookies/should_remove_cookies_by_name_and_domain,webkit), reproduced locally with the same pipeline that repo uses:
which produced exactly the screenshotted report:
{ "title": "TestBrowserContextClearCookies", "attempts": [{ "status": "failed", "errors": [{ "message": "=== RUN TestBrowserContextClearCookies\n--- FAIL: TestBrowserContextClearCookies (0.00s)" }] }, ...] } { "title": "should_remove_cookies_by_name_and_domain", "attempts": [{ "status": "failed", "errors": [{ "message": "browser_context_test.go:16:" }] }, ...] }Two bugs
1.
failureMessagekept only the first non-framing line. Go attributes anassertion to the subtest, indented, starting with a bare
x_test.go:796:line— so that prefix became the whole message and the actual failure survived only
in
stdout.failureErrornow strips ANSI and go test's framing lines (=== RUN/PAUSE/CONT/NAME,--- PASS:/FAIL:/SKIP:/BENCH:— matched with the colon so testify's--- Expecteddiff lines survive as content), dedents the per-nesting-levelindentation go test adds, and keeps the rest. A panic or
-timeoutdump splitsinto headline (
message) + goroutine trace (stack), which is exactly thespec's message/stack pair. Output with no message of its own (a bare
t.Fail())yields a plain
"test failed"rather than echoing the framing back as the error.2. A parent test was emitted as a duplicate leaf inside its own suite. A
parent fails whenever a subtest fails, but its own output is nothing but
=== RUN/--- FAIL— and bug 1's fallback (return the whole outputwhenevery line is framing) turned that framing into the "error". That leaf is the
entity in the screenshot: flaky, no steps, framing-as-error, duplicating the
subtest that actually failed.
ownLeafCarriesSignalnow requires a non-passing attempt that either carriesoutput of its own — its
t.Error/t.Fatal, a panic, a skip reason, which iswhat
features.mdalready claimed — or that no subtest accounts for. Thatsecond clause matters: a parent failing via a bare
t.Fail()while everysubtest passes has nothing else in the report explaining it, and the package
counts as "had tests" so it never becomes an
unattributedErroreither;suppressing it unconditionally would make a real failure vanish.
The parent suite keeps its
browser_context_test.golocation, so the filelink and breadcrumb are unchanged.
Result
The same stream now yields one failing entity — the subtest — with attempt 0
failed → attempt 1 passed (still flaky, not hidden) and the full assertion as
the message:
Prior art
Both halves match how the established Go→report converters behave:
go-junit-report attaches the full
captured output as the failure body and ships
-subtest-modeprecisely becausesubtest parents duplicate their children's failures
(
exclude-parents/ignore-parent-results); gotestsum's own--junitfilewriter puts the complete output in the failure contents.
Tests
5 new (multi-line testify message incl. the
--- Expectedtrap, parent thatfailed only via a subtest, parent failure no subtest explains, panic →
message/stack split, bare
t.Fail()),1 updated (
TestConvert_Subtestsnow gives the parent its own failure text, soit keeps asserting that a genuinely-failing parent is preserved).
go test ./...,go vet ./...,gofmtclean.