feat(console): golden tests for non-TTY ProgressBar outputs + TTY/non-TTY comment clarification#47042
Conversation
…larify TTY/non-TTY comment Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds deterministic golden coverage for ProgressBar non-TTY rendering and clarifies TTY rendering behavior.
Changes:
- Adds six golden test cases for determinate and indeterminate output.
- Documents the TTY/non-TTY percentage-rendering split.
- Makes no production logic or dependency changes.
Show a summary per file
| File | Description |
|---|---|
pkg/console/golden_test.go |
Adds non-TTY progress golden tests. |
pkg/console/progress.go |
Clarifies rendering responsibilities. |
.../determinate_0pct.golden |
Captures 0% output. |
.../determinate_50pct.golden |
Captures 50% output. |
.../determinate_100pct.golden |
Captures 100% output. |
.../determinate_zero_total.golden |
Captures zero-total output. |
.../indeterminate_no_data.golden |
Captures no-data processing output. |
.../indeterminate_with_data.golden |
Captures processing output with bytes. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/8 changed files
- Comments generated: 0
- Review effort level: Medium
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #47042 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (default_business_additions=50, threshold=100). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The golden test coverage and comment clarification look correct. All 6 non-TTY sub-cases are deterministic and the golden files match the code logic. The total==0 edge case is guarded before division. The struct literal for indeterminate mode is safe because the bubbles progress field is only accessed on the TTY path. No blocking issues.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 15.6 AIC · ⌖ 4.34 AIC · ⊞ 4.9K
There was a problem hiding this comment.
Two issues need fixing before merge
The golden tests added here have two structural flaws that undermine their value:
-
t.Skip()on TTY (line 325): silently skips the entire test for any local developer running in a terminal. Golden tests that are routinely skipped rot without anyone noticing — the value of golden coverage depends on them always running. -
Zero-value
progress.Model(lines 350, 355):&ProgressBar{indeterminate: true}bypasses the constructor, leaving the embeddedprogress.Modeluninitialized. The skip guard on line 325 is the only thing preventingViewAs()from being called on a zero-value model, creating a fragile and implicit coupling.
The fix for both: inject/mock isTTY so tests always exercise the non-TTY path (removing the need for the skip guard), and use the constructor for indeterminate bars.
The progress.go comment clarification is fine as-is.
🔎 Code quality review by PR Code Quality Reviewer · 38.2 AIC · ⌖ 4.63 AIC · ⊞ 5.6K
Comment /review to run again
| // The tests always exercise the non-TTY code path because isTTY() returns false in CI | ||
| // (stdout is not a terminal when running tests). | ||
| func TestGolden_ProgressBarNonTTY(t *testing.T) { | ||
| if isTTY() { |
There was a problem hiding this comment.
t.Skip() on TTY makes these golden tests permanently invisible to local developers: any developer running tests in a terminal will silently skip the entire suite, allowing golden files to diverge undetected.
💡 Suggested fix
Inject a isTTYFunc func() bool field on ProgressBar (defaulting to the real isTTY) and override it in tests to always return false. This guarantees the non-TTY code path is exercised in every environment, removing the need for the skip guard:
// In test setup:
bar := NewProgressBar(1024)
bar.isTTYFunc = func() bool { return false } // force non-TTY
golden.RequireEqual(t, []byte(bar.Update(0)))Golden tests that skip on developer machines will silently rot — by the time CI catches it, the golden files may have been regenerated without review.
| }) | ||
|
|
||
| t.Run("indeterminate_no_data", func(t *testing.T) { | ||
| bar := &ProgressBar{indeterminate: true} |
There was a problem hiding this comment.
&ProgressBar{indeterminate: true} bypasses the constructor, leaving progress.Model zero-valued: if isTTY() returns true (e.g. the skip guard is removed or mocked), p.progress.ViewAs() will be called on a zero-value model and will likely panic or produce garbage.
💡 Suggested fix
Use NewProgressBar(0) and set indeterminate: true separately, or add a constructor NewIndeterminateProgressBar():
// safe: model is properly initialized
bar := NewProgressBar(0)
bar.indeterminate = true
// or
bar := NewIndeterminateProgressBar()Right now the only thing keeping these tests from hitting the zero-value model is the t.Skip() guard on line 325. That is a fragile coupling between two unrelated lines.
There was a problem hiding this comment.
🧪 Test Quality Sentinel Report
Analyzed 6 test(s): 6 design, 0 implementation, 0 violation(s).
📊 Metrics (6 tests)
| Metric | Value |
|---|---|
| Analyzed | 6 (Go: 6, JS: 0) |
| ✅ Design | 6 (100%) |
| 0 (0%) | |
| Edge/error coverage | 3 (50%) |
| Duplicate clusters | 0 |
| Inflation | YES (golden_test.go +40 vs progress.go +4) |
| 🚨 Violations | 0 |
| Test | File | Classification | Issues |
|---|---|---|---|
determinate_0pct |
golden_test.go |
design_test / behavioral_contract | 0% boundary edge case ✅ |
determinate_50pct |
golden_test.go |
design_test / behavioral_contract | Happy-path |
determinate_100pct |
golden_test.go |
design_test / behavioral_contract | 100% boundary edge case ✅ |
determinate_zero_total |
golden_test.go |
design_test / behavioral_contract | Zero-division edge case ✅ |
indeterminate_no_data |
golden_test.go |
design_test / behavioral_contract | Happy-path |
indeterminate_with_data |
golden_test.go |
design_test / behavioral_contract | Happy-path |
⚠️ Flagged Tests (1)
Test inflation (pkg/console/golden_test.go) — golden_test.go adds 40 lines while the paired production change (progress.go) adds only 4 lines (10:1 ratio, threshold 2:1). This is expected for a shared golden test file covering multiple components; the new tests provide genuine behavioral coverage. Score penalty applied per rules, but no code change is required.
Verdict
✅ Passed. 0% implementation tests (threshold: 30%). All 6 tests are golden snapshot tests enforcing non-TTY output contracts for
ProgressBar. Build tag//go:build !integrationis present. TheisTTY()guard ensures tests self-skip in TTY environments. Score penalty is solely from the inflation ratio (shared golden test file vs. small targeted production change).
✅ Test Quality Sentinel: 75/100. 0% implementation tests (threshold: 30%).
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — one observation on test construction, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Struct literal vs constructor: Indeterminate tests use
&ProgressBar{indeterminate: true}directly becauseNewIndeterminateProgressBar()is wasm-only. This works, but promoting the constructor to the non-wasm build would decouple tests from unexported fields. - Golden file trailing newlines: All 6 golden files are missing a trailing newline (
\ No newline at end of file). This is cosmetic but can cause noisy diffs with some editors/tools.
Positive Highlights
- ✅ Clean, focused PR with zero production logic changes
- ✅ Excellent coverage of edge cases: 0%, 50%, 100%, zero-total, and both indeterminate variants
- ✅ The
isTTY()skip guard is the right pattern — tests are reliable in CI and honest about their environment dependency - ✅ Clarifying comments in
progress.goare accurate and prevent accidental "unification" of the TTY/non-TTY branches
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 30.6 AIC · ⌖ 4.54 AIC · ⊞ 6.6K
Comment /matt to run again
| }) | ||
|
|
||
| t.Run("indeterminate_no_data", func(t *testing.T) { | ||
| bar := &ProgressBar{indeterminate: true} |
There was a problem hiding this comment.
[/tdd] The indeterminate tests use a raw struct literal &ProgressBar{indeterminate: true} instead of a constructor. NewIndeterminateProgressBar() only exists in the wasm build, so the literal is currently the right choice — but this signals the constructor should be promoted to the non-wasm build so tests can use the public API and avoid coupling to unexported fields.
💡 Suggestion
Add NewIndeterminateProgressBar() to progress.go (non-wasm) alongside NewProgressBar(), then update both test cases:
bar := NewIndeterminateProgressBar()
golden.RequireEqual(t, []byte(bar.Update(0)))This keeps tests decoupled from internal struct layout and makes the constructor discoverable for non-wasm callers.
@copilot please address this.
|
@copilot run pr-finishser skill |
…rogressBar; remove t.Skip from golden test Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Addressed both issues flagged by the PR Code Quality Reviewer in commit
|
|
🎉 This pull request is included in a new release. Release: |
From the Go Fan bubbles/v2 review: the non-TTY
ProgressBar.Updatebranches are pure and deterministic — ideal candidates for golden coverage using the already-presentcharmbracelet/x/exp/goldendep. Also adds a comment clarifying the TTY vs non-TTY percentage rendering split.Changes
Golden tests (
pkg/console/golden_test.go): addsTestGolden_ProgressBarNonTTYwith 6 sub-cases covering both output paths:determinate_0pct0% (0B/1.0KB)determinate_50pct50% (512.0MB/1.00GB)determinate_100pct100% (1.0KB/1.0KB)determinate_zero_total100% (0B/0B)indeterminate_no_dataProcessing...indeterminate_with_dataProcessing... (512.0MB)Clarifying comment (
pkg/console/progress.go): documents that in non-TTY mode the percentage is hand-built viafmt.Sprintf, while in TTY mode it is rendered by bubbles'ViewAs(whereShowPercentagedefaults to true) — so the two branches aren't accidentally "unified" in a way that breaks either.No new dependencies; no production logic changes.