Tighten golden-test coverage: remove dead console fixtures and goldenize wasm all-engines#33248
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR strengthens golden-test coverage in two ways: it deletes orphaned golden fixtures (and stale stub comments) under pkg/console/testdata whose tests no longer exist, and it replaces three weak require.Contains assertions in TestWasmGolden_AllEngines with strict golden.RequireEqual comparisons against newly added per-engine fixtures.
Changes:
- Deleted 15 orphaned
.goldenfiles across fourTestGolden_*subdirectories inpkg/console/testdata, plus the corresponding stub comments inpkg/console/golden_test.go, and added a guardrail comment about path-safe subtest names. - Replaced substring assertions in
TestWasmGolden_AllEngineswith a singlegolden.RequireEqual(t, normalizeOutput(yamlOutput))call. - Added 5 new engine golden fixtures (
copilot,claude,codex,gemini,pi) capturing full normalized YAML output.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/wasm_golden_test.go | Swap weak Contains checks for strict golden comparison. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/{copilot,claude,codex,gemini,pi}.golden | New per-engine full-YAML golden fixtures backing the strict assertion. |
| pkg/console/golden_test.go | Add path-safe naming note; remove stub comments for nonexistent tests. |
| pkg/console/testdata/TestGolden_TreeRendering/*.golden (5 files) | Remove orphaned tree-rendering fixtures. |
| pkg/console/testdata/TestGolden_LayoutEmphasisBox/*.golden (4 files) | Remove orphaned emphasis-box fixtures. |
| pkg/console/testdata/TestGolden_LayoutComposition/*.golden (3 files) | Remove orphaned composition fixtures. |
| pkg/console/testdata/TestGolden_LayoutBoxRendering/*.golden (3 files) | Remove orphaned layout-box fixtures. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 22/22 changed files
- Comments generated: 0
🏗️ Design Decision Gate — ADR RequiredThis PR introduces significant changes to core test/business-logic directories (>3000 new lines under AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: The inferred decision is:
What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. The shift from substring to strict golden comparison and the deletion of orphan fixtures are exactly the kind of test-strategy choices that future contributors (and your future self) will want context for when a golden diff blows up six months from now. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd (test-driven development) and /zoom-out (broader architectural context) based on this test refactoring and cleanup PR.
Key Themes
✅ Excellent dead code removal — The 15 orphaned .golden files and their stub comments were technical debt from incomplete test implementation. This cleanup improves discoverability and removes confusion about which tests actually exist.
✅ Significant test hardening — Upgrading TestWasmGolden_AllEngines from require.Contains(...) to full golden comparison is a major quality improvement. The weak assertion pattern only checked for section presence, which could miss structural regressions. Golden files provide exact output verification.
Positive Highlights
- ✅ Path-safety guardrail — The comment in
pkg/console/golden_test.go("Keep subtest names path-safe...") is a valuable maintainability note for future test authors - ✅ Complete engine coverage — Adding per-engine golden files (
copilot.golden,claude.golden, etc.) ensures all five engine compilation paths are regression-protected - ✅ Normalization consistency — The
normalizeOutput()helper ensures golden comparisons are stable across environments
Suggestions
-
[/tdd] Document what happened to the deleted test coverage in the commit message or PR body — were
LayoutBoxRendering,LayoutComposition, etc. never implemented? Are they covered by other tests? This context helps reviewers understand whether functionality is now untested. -
[/zoom-out] Consider a follow-up task to audit whether the functionality represented by the deleted fixtures should have tests — the golden files suggested someone intended to test layout rendering, emphasis boxes, and tree rendering. If those code paths are untested, that's a gap.
Verdict
APPROVE — The cleanup is safe and the test hardening is valuable. The missing context is a minor documentation gap, not a blocker.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 4.3M
| require.Contains(t, yamlOutput, "name:", "%s engine output missing name", eng.name) | ||
| require.Contains(t, yamlOutput, "on:", "%s engine output missing on", eng.name) | ||
| require.Contains(t, yamlOutput, "jobs:", "%s engine output missing jobs", eng.name) | ||
| golden.RequireEqual(t, normalizeOutput(yamlOutput)) |
There was a problem hiding this comment.
[/tdd] Excellent upgrade from require.Contains(...) to golden.RequireEqual(...). The old assertion pattern:
require.Contains(t, yamlOutput, "name:")
require.Contains(t, yamlOutput, "jobs:")Only verified that YAML sections existed, but couldn't catch structural regressions like incorrect nesting, missing fields, or value changes. Golden comparison catches all of these.
Impact: This change means all five engine compilation paths now have exact output verification, significantly reducing the risk of silent regressions.
| "github.com/charmbracelet/x/exp/golden" | ||
| ) | ||
|
|
||
| // Keep subtest names path-safe (no "/") since golden.RequireEqual maps them to testdata paths. |
There was a problem hiding this comment.
[/tdd] Great guardrail comment. The constraint is subtle: golden.RequireEqual(t, ...) uses t.Name() to derive the fixture path, so subtest names like "layout/wide" would try to create testdata/TestFoo/layout/wide.golden, which breaks.
This is the kind of "pit of success" documentation that prevents future confusion.
🧪 Test Quality Sentinel ReportTest Quality Score: 95/100
|
The
charmbracelet/x/exp/goldenusage audit identified stale console golden fixtures with no backing tests and weak assertions inTestWasmGolden_AllEnginesthat only checked for YAML section presence. This PR removes dead fixtures/comments and upgrades engine coverage to strict golden comparisons.Console golden cleanup
.goldenfiles under:TestGolden_LayoutBoxRenderingTestGolden_LayoutCompositionTestGolden_LayoutEmphasisBoxTestGolden_TreeRenderingpkg/console/golden_test.gofor non-existent tests.Golden naming guardrail
pkg/console/golden_test.goto keep subtest names path-safe (no/) sincegolden.RequireEqualmaps subtest names totestdatapaths.Wasm engine regression hardening
TestWasmGolden_AllEnginesto assert full normalized YAML via golden files instead ofrequire.Contains(...).pkg/workflow/testdata/TestWasmGolden_AllEngines/:copilot.goldenclaude.goldencodex.goldengemini.goldenpi.goldenAssertion change (example)