Repository navigation
Strengthen console rendering tests and formatter boundary coverage - #74562
Conversation
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
This comment has been minimized.
This comment has been minimized.
|
✅ Ponytail Reviewer completed successfully!
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
What this change does: This expands console tests for unsupported inputs, rendering options, formatter boundaries, and embedded structs. It also consolidates several embedded-struct cases into table-driven subtests. The added coverage is generally focused; one redundant case remains.
Should fix:
-
Duplicate nil case (
pkg/console/render_test.go:L137)- What this is: The
niltable value andany(nil)both become the same nil interface. - Problem: Both subtests call
RenderStruct(nil)and assert the same empty output, so one test adds no coverage. - Fix: Remove the
nil interfacerow. - If we skip it: The suite carries a redundant subtest for no extra behavior.
- What this is: The
Verdict: fix 1 first.
Lean: -1 lines possible.
Generated by ✂️ Ponytail Reviewer for #74562 · codex · routed: gpt56 max · ~3 AIC · ⌖ ~1 AIC · ⊞ 118
Comment /ponytail to run again
| input any | ||
| }{ | ||
| {"nil", nil}, | ||
| {"nil interface", any(nil)}, |
There was a problem hiding this comment.
- Duplicate nil case —
nilin ananyfield andany(nil)produce the same nil interface, so both subtests callRenderStruct(nil)and assert the same result. Delete this row; it adds one redundant test without coverage.
There was a problem hiding this comment.
🟡 Changes recommended
The shared Base.Engine refactor weakens test coverage by no longer verifying that an empty required embedded field remains present, leaving a regression untested.
1 open issue
What changed in this PR
This test-only pull request expands console formatter and reflective-rendering coverage without changing production behavior.
Changes:
- Adds boundary and negative cases for
FormatTokensandFormatNumber. - Covers rendering width, output destinations, slices, maps, unsupported inputs, and unexported fields.
- Consolidates embedded-struct cases into table-driven tests and improves failure isolation.
- Retains coverage for non-
omitemptyembedded fields and validates header preconditions before indexing.
The embedded-field coverage regression and missing header precondition should be addressed before approval. This pull request requires changes.
| File | Summary |
|---|---|
pkg/console/render_test.go |
Expands formatter and reflection coverage, validates rendering options and width boundaries, and consolidates embedded-struct test cases. |
Review effort: Lite (auto)
| type Base struct { | ||
| Name string `console:"header:Name"` | ||
| Engine string `console:"header:Engine"` | ||
| Engine string `console:"header:Engine,omitempty"` |
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (>100 new lines) but does not have a linked Architecture Decision Record (ADR). Why the gate ran: the PR adds 245 lines under 📄 Draft ADR committed: The draft records the decision this PR makes implicitly: the new tests pin current
If any of these outputs is not intended, it is better to decide that now, before the tests lock it in.
📋 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 Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
There was a problem hiding this comment.
Reviewed the test-only changes. One coverage regression should be addressed: the shared embedded fixture now uses omitempty, so the suite no longer verifies zero-valued non-omitempty fields. No production-code correctness issues were identified in the changed lines.
Highlights
- Preserve separate fixtures for ordinary embedded fields and
omitemptybehavior. - The added formatter and width cases otherwise provide useful boundary coverage.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~4 AIC · ⌖ ~1 AIC · ⊞ 1.6K
Comment /review to run again
| type Base struct { | ||
| Name string `console:"header:Name"` | ||
| Engine string `console:"header:Engine"` | ||
| Engine string `console:"header:Engine,omitempty"` |
There was a problem hiding this comment.
This changes the shared Base.Engine fixture to omitempty, so the value-embedded case no longer verifies that a zero-valued non-omitempty embedded field is rendered.
💡 Preserve the original coverage
The previous value case used console:"header:Engine", while only the pointer/omitted-empty case used omitempty; consolidating them removes coverage for the non-omitempty behavior. Keep the original fixture for the value case and introduce a separate embedded type (or tag) for the omitempty case.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd (triage classified this as a tests-only change). The new coverage is mostly solid, with a few tests that don't verify what their names claim.
📋 Key Themes & Highlights
Key Themes
- Stderr option is not verified: flipping the
options.Stderrbranch inRenderStructWithOptionskeeps every test green, so the stderr subtests cannot catch a destination regression. - Redundant nil case:
"nil"and"nil interface"are the same input when the field type isany. - Snapshot-style test name:
TestRenderStruct_UnexportedFieldsasserts exact whole-output equality, and its name suggests the opposite of what it checks. - Rounding artifacts pinned as spec:
999_999→"1000.0K"and1_000_000_000→"1000.0M"lock in current output that reads oddly.
Positive Highlights
- ✅ Verified locally: the
FormatTokensthreshold mutation is caught byTestFormatTokens. - ✅ Table-driven conversions keep the existing assertions and add
requirepreconditions before indexing, which improves failure isolation. - ✅ Embedded-struct and nil-pointer row cases are well chosen, and no production code changed.
- ✅ The new tests pass on the branch (
go test ./pkg/console/).
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · routed: haiku55 xhigh · ~7 AIC · ⌖ ~1 AIC · ⊞ 5.4K
Comment /matt to run again
| } { | ||
| t.Run(destination.name, func(t *testing.T) { | ||
| t.Parallel() | ||
| unbounded := RenderStructWithOptions(tt.data, RenderOptions{Stderr: destination.stderr}) |
There was a problem hiding this comment.
[/tdd] The Stderr dimension is not actually verified. Swapping the if options.Stderr branch in RenderStructWithOptions still passes every test, because in a non-TTY test run both destinations render identically. The subtest names and the PR description overstate what is covered.
💡 Suggested fix
Either add a seam so the test can observe which destination was selected, or drop the stderr dimension and keep only the stdout path. Verified locally: flipping the branch leaves TestRenderStructWithOptions green.
@copilot please address this.
| input any | ||
| }{ | ||
| {"nil", nil}, | ||
| {"nil interface", any(nil)}, |
There was a problem hiding this comment.
[/tdd] {"nil", nil} and {"nil interface", any(nil)} are the same input, because the field type is any. One of these rows is redundant.
💡 Suggested fix
Drop the "nil interface" row. A typed nil pointer is already covered by "nil struct pointer".
@copilot please address this.
| } | ||
| } | ||
|
|
||
| func TestRenderStruct_UnexportedFields(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] The name suggests unexported fields are skipped, but the exact-string assertion shows they are rendered (name, count, enabled, and so on). The whole-output equality also makes the test fail on any alignment change.
💡 Suggested change
Rename it to describe the behaviour, for example TestRenderStruct_RendersUnexportedFields, and assert each row with assert.Contains so the test reads as a specification rather than a snapshot.
@copilot please address this.
| {"thousand", 1000, "1.0K"}, | ||
| {"fractional thousand", 1500, "1.5K"}, | ||
| {"rounded thousand", 9999, "10.0K"}, | ||
| {"below million", 999_999, "1000.0K"}, |
There was a problem hiding this comment.
[/tdd] 999_999 expecting "1000.0K" (and 1_000_000_000 expecting "1000.0M") pins a rounding artifact. A value that should read as 1.0M is shown as 1000.0K, and the table makes that look like intended behaviour.
💡 Suggested change
Add a short comment on these rows stating that they document current output. A future rounding fix then becomes a deliberate test update rather than a silent spec change. Alternatively, leave them out of this PR and track the rounding issue separately.
@copilot please address this.
|
Note The pull request was not created — a fallback review issue was created instead due to protected file changes: #74606 PR finisher: submitted for publication (not confirmed pushed): merge of
Validation: Review replies/resolutions deferred until the fix is published. CI needs a maintainer re-trigger. PR finisher dispatch requested for the following outstanding work (see dispatch summary). Sous-chef head: 37f2301
|
|
Note The pull request was not created — a fallback review issue was created instead due to protected file changes: #74632 PR finisher: submitted for publication (not yet confirmed pushed): merged Validation: Review replies/resolutions are deferred until the fix is published. CI needs a maintainer re-trigger after the push. Sous-chef head: 37f2301
|
|
Note The pull request was not created — a fallback review issue was created instead due to protected file changes: #74640 PR finisher: Merged
Validation: Next: CI is stale after the push and needs a maintainer re-trigger. Review thread replies and resolutions are deferred to a later pass, once the fix is published. PR finisher dispatch requested for the following outstanding work (see the dispatch summary above). Sous-chef head: 37f2301
|
|
Commit pushed:
|
PR FinisherSubmitted for publication (not yet confirmed pushed): Validation: Remaining:
PR finisher dispatch requested for the following outstanding work (summary routed from Sous Chef). Sous-chef head: 37f2301
|
|
Note The pull request was not created — a fallback review issue was created instead due to protected file changes: #74657 PR finisher pass: merged Local validation: Sous-chef head: 441a2d8
|
Console tests lacked direct coverage for token formatting, rendering options, and reflection edge cases. This test-only change expands coverage and improves failure isolation without changing production behavior.
FormatTokensboundaries and negatives.FormatNumbercases to named subtests and cover negative inputs.MaxWidthandStderroptions with struct slices and maps.requirefor preconditions before indexing table results.