From 7b652fdbf72ad194c45cd0e511e332092ab7d02f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 3 Nov 2026 16:23:31 +0000 Subject: [PATCH 1/4] Initial plan From af3b9c19983b3d14c69be5d2a98ea16eb7d60459 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 3 Nov 2026 16:34:23 +0000 Subject: [PATCH 2/4] test(console): cover rendering options and formatter boundaries Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/console/render_test.go | 363 +++++++++++++++++++++++++------------ 1 file changed, 245 insertions(+), 118 deletions(-) diff --git a/pkg/console/render_test.go b/pkg/console/render_test.go index f69a2c194e2..8cea5e344d6 100644 --- a/pkg/console/render_test.go +++ b/pkg/console/render_test.go @@ -4,9 +4,11 @@ package console import ( "reflect" + "strconv" "strings" "testing" + lipgloss "charm.land/lipgloss/v2" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -125,6 +127,95 @@ func TestRenderMap(t *testing.T) { assert.Contains(t, output, "value2", "map output should contain value2") } +func TestRenderStruct_UnsupportedInputs(t *testing.T) { + t.Parallel() + tests := []struct { + name string + input any + }{ + {"nil", nil}, + {"nil interface", any(nil)}, + {"nil struct pointer", (*TestOverview)(nil)}, + {"integer", 42}, + {"string", "hello"}, + {"boolean", true}, + {"channel", make(chan int)}, + {"function", func() {}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Empty(t, RenderStruct(tt.input)) + }) + } +} + +func TestRenderStruct_UnexportedFields(t *testing.T) { + t.Parallel() + data := struct { + name string + count int + enabled bool + channel chan int + callback func() + }{ + name: "hidden", + count: 42, + enabled: true, + channel: make(chan int), + callback: func() {}, + } + + assert.Equal(t, " name : hidden\n count : 42\n enabled : true\n channel : chan int\n callback: func()\n\n", RenderStruct(data)) +} + +func TestRenderStructWithOptions(t *testing.T) { + t.Parallel() + value := strings.Repeat("x", 180) + tests := []struct { + name string + data any + }{ + {"slice of structs", []struct{ Path string }{{value}}}, + {"map", map[string]string{"Path": value}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + for _, destination := range []struct { + name string + stderr bool + render func(any) string + }{ + {"stdout", false, RenderStructStdout}, + {"stderr", true, RenderStruct}, + } { + t.Run(destination.name, func(t *testing.T) { + t.Parallel() + unbounded := RenderStructWithOptions(tt.data, RenderOptions{Stderr: destination.stderr}) + assert.Equal(t, destination.render(tt.data), unbounded, "zero width should preserve default rendering") + assert.Contains(t, unbounded, value, "zero width should not wrap long values") + + for _, width := range []int{24, 48} { + t.Run(strconv.Itoa(width), func(t *testing.T) { + t.Parallel() + output := RenderStructWithOptions(tt.data, RenderOptions{MaxWidth: width, Stderr: destination.stderr}) + require.NotEmpty(t, output) + assert.Contains(t, output, "Path") + assert.Equal(t, len(value), strings.Count(output, "x"), "wrapping must preserve the value") + for line := range strings.SplitSeq(output, "\n") { + assert.LessOrEqual(t, lipgloss.Width(line), width, "line %q exceeds MaxWidth", line) + } + }) + } + }) + } + }) + } +} + func TestParseConsoleTag(t *testing.T) { t.Parallel() tests := []struct { @@ -287,9 +378,10 @@ func TestBuildTableConfig(t *testing.T) { config := buildTableConfig(val, "Test Jobs") assert.Len(t, config.Headers, 3, "expected 3 headers (excluding omitempty empty fields)") - assert.Len(t, config.Rows, 2, "expected 2 rows in table output") + require.Len(t, config.Rows, 2, "expected 2 rows in table output") // Check first row + require.Len(t, config.Rows[0], 3, "expected a cell for each header") assert.Equal(t, "job-1", config.Rows[0][0], "first row name should be job-1") } @@ -360,6 +452,12 @@ func TestFormatNumber(t *testing.T) { }{ // Zero {0, "0"}, + // Negative numbers remain unabridged. + {-1, "-1"}, + {-999, "-999"}, + {-1000, "-1000"}, + {-1000000, "-1000000"}, + {-1000000000, "-1000000000"}, // Small numbers (under 1000) {5, "5"}, {42, "42"}, @@ -401,8 +499,40 @@ func TestFormatNumber(t *testing.T) { } for _, test := range tests { - result := FormatNumber(test.input) - assert.Equal(t, test.expected, result, "FormatNumber(%d) mismatch", test.input) + t.Run(strconv.Itoa(test.input), func(t *testing.T) { + t.Parallel() + assert.Equal(t, test.expected, FormatNumber(test.input)) + }) + } +} + +func TestFormatTokens(t *testing.T) { + t.Parallel() + tests := []struct { + name string + input int + expected string + }{ + {"zero", 0, "-"}, + {"single token", 1, "1"}, + {"below thousand", 999, "999"}, + {"thousand", 1000, "1.0K"}, + {"fractional thousand", 1500, "1.5K"}, + {"rounded thousand", 9999, "10.0K"}, + {"below million", 999_999, "1000.0K"}, + {"million", 1_000_000, "1.0M"}, + {"fractional million", 1_200_000, "1.2M"}, + {"billion", 1_000_000_000, "1000.0M"}, + {"negative single token", -1, "-1"}, + {"negative thousand", -1000, "-1000"}, + {"negative million", -1_000_000, "-1000000"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.expected, FormatTokens(tt.input)) + }) } } @@ -470,7 +600,7 @@ func TestRenderStruct_EmbeddedStruct(t *testing.T) { t.Parallel() type Base struct { Name string `console:"header:Name"` - Engine string `console:"header:Engine"` + Engine string `console:"header:Engine,omitempty"` } type Extended struct { @@ -478,48 +608,10 @@ func TestRenderStruct_EmbeddedStruct(t *testing.T) { Status string `console:"header:Status"` } - data := Extended{ - Base: Base{Name: "my-workflow", Engine: "copilot"}, - Status: "active", - } - - output := RenderStruct(data) - - // Fields from the embedded struct should appear at the top level. - assert.Contains(t, output, "my-workflow", "output should contain Name from embedded struct") - assert.Contains(t, output, "copilot", "output should contain Engine from embedded struct") - assert.Contains(t, output, "active", "output should contain Status from outer struct") - // The embedded type name should NOT appear as a section title. - assert.NotContains(t, output, "Base", "output should not contain the embedded struct type name as a section") -} - -func TestRenderStruct_EmbeddedStructOmitEmptyAndPointer(t *testing.T) { - t.Parallel() - type Base struct { - Name string `console:"header:Name"` - Engine string `console:"header:Engine,omitempty"` - } - - type Extended struct { + type WithPointer struct { *Base Status string `console:"header:Status"` } - - data := Extended{ - Base: &Base{Name: "wf"}, - Status: "active", - } - - output := RenderStruct(data) - - assert.Contains(t, output, "wf", "output should contain Name from embedded pointer struct") - assert.Contains(t, output, "active", "output should contain Status from outer struct") - assert.NotContains(t, output, "Engine", "zero omitempty field in embed should be suppressed") - assert.NotContains(t, output, "Base", "output should not contain the embedded pointer type name as a section") -} - -func TestRenderStruct_NestedEmbeddedStruct(t *testing.T) { - t.Parallel() type Inner struct { Name string `console:"header:Name"` } @@ -534,19 +626,47 @@ func TestRenderStruct_NestedEmbeddedStruct(t *testing.T) { Status string `console:"header:Status"` } - output := RenderStruct(Outer{ - Middle: Middle{ - Inner: Inner{Name: "wf"}, - Engine: "copilot", + tests := []struct { + name string + data any + contains []string + notContains []string + }{ + { + name: "value", + data: Extended{Base: Base{Name: "my-workflow", Engine: "copilot"}, Status: "active"}, + contains: []string{"my-workflow", "copilot", "active"}, + notContains: []string{"Base"}, }, - Status: "ok", - }) - - assert.Contains(t, output, "wf") - assert.Contains(t, output, "copilot") - assert.Contains(t, output, "ok") - assert.NotContains(t, output, "Middle") - assert.NotContains(t, output, "Inner") + { + name: "pointer with omitted empty field", + data: WithPointer{Base: &Base{Name: "wf"}, Status: "active"}, + contains: []string{"wf", "active"}, + notContains: []string{"Engine", "Base"}, + }, + { + name: "nested", + data: Outer{ + Middle: Middle{Inner: Inner{Name: "wf"}, Engine: "copilot"}, + Status: "ok", + }, + contains: []string{"wf", "copilot", "ok"}, + notContains: []string{"Middle", "Inner"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + output := RenderStruct(tt.data) + for _, text := range tt.contains { + assert.Contains(t, output, text) + } + for _, text := range tt.notContains { + assert.NotContains(t, output, text) + } + }) + } } // TestRenderSlice_EmbeddedStruct tests that a slice of structs with anonymous embedded @@ -563,77 +683,84 @@ func TestRenderSlice_EmbeddedStruct(t *testing.T) { Status string `console:"header:Status"` } - items := []Extended{ - {Base: Base{Name: "wf-1", Engine: "copilot"}, Status: "active"}, - {Base: Base{Name: "wf-2", Engine: "claude"}, Status: "disabled"}, - } - - output := RenderStruct(items) - - // All columns (including those from the embedded struct) should appear as headers. - assert.Contains(t, output, "Name", "output should contain Name column header") - assert.Contains(t, output, "Engine", "output should contain Engine column header") - assert.Contains(t, output, "Status", "output should contain Status column header") - lines := strings.Split(output, "\n") - headerLine := "" - for _, line := range lines { - if strings.Contains(line, "Name") && strings.Contains(line, "Status") { - headerLine = line - break - } - } - require.NotEmpty(t, headerLine, "table output should include a header row") - assert.Less(t, strings.Index(headerLine, "Name"), strings.Index(headerLine, "Status"), "embedded Name column must appear before outer Status column in the header row") - - // All values should be present in the table rows. - assert.Contains(t, output, "wf-1", "output should contain first workflow name") - assert.Contains(t, output, "copilot", "output should contain first engine") - assert.Contains(t, output, "active", "output should contain first status") - assert.Contains(t, output, "wf-2", "output should contain second workflow name") - assert.Contains(t, output, "claude", "output should contain second engine") - assert.Contains(t, output, "disabled", "output should contain second status") -} - -func TestRenderSlice_SkippedEmbeddedStruct(t *testing.T) { - t.Parallel() - type Base struct { - Name string `console:"header:Name"` - } - type Extended struct { + type Skipped struct { Base `console:"-"` Status string `console:"header:Status"` } - - output := RenderStruct([]Extended{{Base: Base{Name: "wf-1"}, Status: "active"}}) - - assert.NotContains(t, output, "Name") - assert.NotContains(t, output, "wf-1") - assert.Contains(t, output, "Status") - assert.Contains(t, output, "active") -} - -func TestRenderSlice_EmbeddedPointerStruct(t *testing.T) { - t.Parallel() - type Base struct { + type PointerBase struct { Name string `console:"header:Name"` } - - type Extended struct { - *Base + type WithPointer struct { + *PointerBase Status string `console:"header:Status"` } - items := []Extended{ - {Base: &Base{Name: "wf-1"}, Status: "active"}, - {Status: "missing"}, + tests := []struct { + name string + data any + headers []string + values []string + notContains []string + emptyCell bool + }{ + { + name: "value", + data: []Extended{ + {Base: Base{Name: "wf-1", Engine: "copilot"}, Status: "active"}, + {Base: Base{Name: "wf-2", Engine: "claude"}, Status: "disabled"}, + }, + headers: []string{"Name", "Engine", "Status"}, + values: []string{"wf-1", "copilot", "active", "wf-2", "claude", "disabled"}, + notContains: []string{"Base"}, + }, + { + name: "skipped", + data: []Skipped{{Base: Base{Name: "wf-1"}, Status: "active"}}, + headers: []string{"Status"}, + values: []string{"active"}, + notContains: []string{"Name", "Engine", "wf-1"}, + }, + { + name: "pointer including nil row", + data: []WithPointer{ + {PointerBase: &PointerBase{Name: "wf-1"}, Status: "active"}, + {Status: "missing"}, + }, + headers: []string{"Name", "Status"}, + values: []string{"wf-1", "active", "missing"}, + notContains: []string{"PointerBase"}, + emptyCell: true, + }, } - output := RenderStruct(items) - - assert.Contains(t, output, "Name", "output should contain Name column header from embedded pointer struct") - assert.Contains(t, output, "Status", "output should contain Status column header") - assert.Contains(t, output, "wf-1", "output should contain first workflow name") - assert.Regexp(t, `(?m)^│\s*│missing│$`, output, "output should render an empty Name cell for rows with a nil embedded pointer") - assert.Contains(t, output, "missing", "output should contain second status") - assert.NotContains(t, output, "Base", "output should not contain the embedded pointer type name as a column") + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + output := RenderStruct(tt.data) + headerLine := "" + for line := range strings.SplitSeq(output, "\n") { + if strings.Contains(line, tt.headers[0]) { + headerLine = line + break + } + } + require.NotEmpty(t, headerLine, "table output should include a header row") + previous := -1 + for _, header := range tt.headers { + require.Contains(t, headerLine, header) + position := strings.Index(headerLine, header) + assert.Greater(t, position, previous, "columns should follow field declaration order") + previous = position + } + for _, text := range tt.values { + assert.Contains(t, output, text) + } + for _, text := range tt.notContains { + assert.NotContains(t, output, text) + } + if tt.emptyCell { + assert.Regexp(t, `(?m)^│\s*│missing│$`, output, "nil embedded pointer should render an empty Name cell") + } + }) + } } From 37f2301d76954ec4240348f46851e56114113292 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 3 Nov 2026 19:22:31 +0000 Subject: [PATCH 3/4] docs(adr): add draft ADR-74562 for console rendering characterization tests --- ...cterization-tests-for-console-rendering.md | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 docs/adr/74562-table-driven-characterization-tests-for-console-rendering.md diff --git a/docs/adr/74562-table-driven-characterization-tests-for-console-rendering.md b/docs/adr/74562-table-driven-characterization-tests-for-console-rendering.md new file mode 100644 index 00000000000..f07c6a94c3b --- /dev/null +++ b/docs/adr/74562-table-driven-characterization-tests-for-console-rendering.md @@ -0,0 +1,60 @@ +# ADR-74562: Table-Driven Characterization Tests for Console Rendering and Number Formatting + +**Date**: 2026-11-03 +**Status**: Draft +**Deciders**: [TODO: verify] pelikhan, Copilot coding agent + +--- + +### Context + +`pkg/console` renders CLI output by reflecting over structs, slices, and maps (`RenderStruct`, `RenderStructStdout`, `RenderStructWithOptions`). It also formats counts for display (`FormatNumber`, `FormatTokens`). Before this change, `FormatTokens` had no direct tests. The `RenderOptions` `MaxWidth` and `Stderr` options had no direct tests either. Reflection edge cases were also untested: nil values, scalars, channels, functions, and unexported fields. The embedded-struct tests were split across several near-duplicate functions with linear assertions, so one failure could hide others. Issue #74185 asked for better test quality in this package. The PR is test-only and does not change any production code. + +### Decision + +We will test `pkg/console` rendering and formatting with table-driven, `t.Parallel()` subtests. Each case will be a named subtest, and we will use `require` for preconditions before indexing into results (for example, `config.Rows[0]`). These tests are characterization tests: they pin the current behavior of the formatters and renderers as the contract. That includes these edge cases: + +- `FormatTokens(0)` returns `"-"`. +- Negative inputs to `FormatNumber` and `FormatTokens` are returned unabridged, for example `"-1000000"`. +- Values just below a unit boundary round up within the smaller unit, for example `FormatTokens(999_999)` returns `"1000.0K"`. +- Values of one billion or more stay in millions, for example `FormatTokens(1_000_000_000)` returns `"1000.0M"`. +- With `MaxWidth` set, no rendered line is wider than that limit (measured with `lipgloss.Width`), and wrapping keeps the full value. + +The main reason is regression safety at low cost. Named, parallel subtests isolate failures. A pinned contract makes any future change to the formatting rules a deliberate, visible test change. + +### Alternatives Considered + +#### Alternative 1: Keep separate linear test functions per scenario + +We could have kept separate functions such as `TestRenderStruct_EmbeddedStruct`, `TestRenderStruct_EmbeddedStructOmitEmptyAndPointer`, and `TestRenderSlice_SkippedEmbeddedStruct`, and added more functions for the new cases. This needs less restructuring and keeps each scenario's custom assertion messages. We did not choose it because it duplicates type declarations and assertion logic, and because a failed assertion partway through a function hides the checks that follow. + +#### Alternative 2: Specify the intended formatting behavior and fix the formatters + +Instead of pinning current output, we could have defined the behavior we want and changed `render.go` to match. For example, negatives could be abbreviated symmetrically (`-1.0K`), `999_999` could promote to `1.0M`, and a billion unit could be added. We did not choose this because the PR is scoped as a test-only quality improvement (issue #74185), and changing user-visible CLI output would need its own review. [TODO: verify whether the current negative and boundary outputs are intended or just tolerated.] + +#### Alternative 3: Golden-file snapshot tests for rendered output + +Full rendered output could be compared against checked-in golden files. That catches every visual change, but the tests become brittle when styling, borders, or terminal width handling changes. Targeted assertions such as `Contains`, column order, and line width express the actual contract more precisely. + +### Consequences + +#### Positive +- `FormatTokens`, the `RenderOptions` `MaxWidth` and `Stderr` options, and reflection edge cases now have direct coverage. +- Named parallel subtests report each failing case on its own and run concurrently, so failures are easier to diagnose. +- `require` preconditions turn out-of-range index panics into clear test failures. +- Consolidating the embedded-struct tests into tables removes duplicated type declarations and assertion code. + +#### Negative +- Pinning current output turns behavior that may be accidental into a tested contract: unabridged negatives, `"1000.0K"` just below one million, and `"1000.0M"` for billions. Anyone who later improves the formatting must update these tests on purpose. +- The table-driven tests use generic assertion messages (for example, `assert.Contains(t, output, text)`), so failures carry less per-scenario detail than the custom messages they replace. +- Nested subtests for destination × width × data shape are harder to read than flat tests. +- `TestRenderStruct_UnexportedFields` asserts an exact output string, so any change to padding or alignment breaks it. + +#### Neutral +- The test file now imports `charm.land/lipgloss/v2` to measure rendered line width. This is already a dependency of the package. +- No production code changes; runtime behavior is unchanged. +- The table-driven, parallel-subtest pattern matches existing conventions in the repository's Go tests. [TODO: verify whether to adopt it as a package-wide convention.] + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* From 441a2d8a16bdfa05b8cbff617b517139fd86f7d4 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 3 Nov 2026 22:30:31 +0000 Subject: [PATCH 4/4] Apply filtered changes --- pkg/console/render_test.go | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/pkg/console/render_test.go b/pkg/console/render_test.go index 8cea5e344d6..79af21298c3 100644 --- a/pkg/console/render_test.go +++ b/pkg/console/render_test.go @@ -134,7 +134,6 @@ func TestRenderStruct_UnsupportedInputs(t *testing.T) { input any }{ {"nil", nil}, - {"nil interface", any(nil)}, {"nil struct pointer", (*TestOverview)(nil)}, {"integer", 42}, {"string", "hello"}, @@ -151,7 +150,7 @@ func TestRenderStruct_UnsupportedInputs(t *testing.T) { } } -func TestRenderStruct_UnexportedFields(t *testing.T) { +func TestRenderStruct_RendersUnexportedFields(t *testing.T) { t.Parallel() data := struct { name string @@ -167,7 +166,10 @@ func TestRenderStruct_UnexportedFields(t *testing.T) { callback: func() {}, } - assert.Equal(t, " name : hidden\n count : 42\n enabled : true\n channel : chan int\n callback: func()\n\n", RenderStruct(data)) + output := RenderStruct(data) + for _, text := range []string{"name", "hidden", "count", "42", "enabled", "true"} { + assert.Contains(t, output, text) + } } func TestRenderStructWithOptions(t *testing.T) { @@ -519,10 +521,10 @@ func TestFormatTokens(t *testing.T) { {"thousand", 1000, "1.0K"}, {"fractional thousand", 1500, "1.5K"}, {"rounded thousand", 9999, "10.0K"}, - {"below million", 999_999, "1000.0K"}, + {"upper thousands", 999_400, "999.4K"}, {"million", 1_000_000, "1.0M"}, {"fractional million", 1_200_000, "1.2M"}, - {"billion", 1_000_000_000, "1000.0M"}, + {"tens of millions", 12_300_000, "12.3M"}, {"negative single token", -1, "-1"}, {"negative thousand", -1000, "-1000"}, {"negative million", -1_000_000, "-1000000"}, @@ -599,6 +601,11 @@ func TestRenderStruct_NilPointerToStruct(t *testing.T) { func TestRenderStruct_EmbeddedStruct(t *testing.T) { t.Parallel() type Base struct { + Name string `console:"header:Name"` + Engine string `console:"header:Engine"` + } + + type OptionalBase struct { Name string `console:"header:Name"` Engine string `console:"header:Engine,omitempty"` } @@ -609,7 +616,7 @@ func TestRenderStruct_EmbeddedStruct(t *testing.T) { } type WithPointer struct { - *Base + *OptionalBase Status string `console:"header:Status"` } type Inner struct { @@ -638,9 +645,14 @@ func TestRenderStruct_EmbeddedStruct(t *testing.T) { contains: []string{"my-workflow", "copilot", "active"}, notContains: []string{"Base"}, }, + { + name: "value with empty required field", + data: Extended{Base: Base{Name: "wf"}, Status: "active"}, + contains: []string{"wf", "Engine", "active"}, + }, { name: "pointer with omitted empty field", - data: WithPointer{Base: &Base{Name: "wf"}, Status: "active"}, + data: WithPointer{OptionalBase: &OptionalBase{Name: "wf"}, Status: "active"}, contains: []string{"wf", "active"}, notContains: []string{"Engine", "Base"}, },