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.* diff --git a/pkg/console/render_test.go b/pkg/console/render_test.go index f69a2c194e2..79af21298c3 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,97 @@ 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 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_RendersUnexportedFields(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() {}, + } + + output := RenderStruct(data) + for _, text := range []string{"name", "hidden", "count", "42", "enabled", "true"} { + assert.Contains(t, output, text) + } +} + +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 +380,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 +454,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 +501,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"}, + {"upper thousands", 999_400, "999.4K"}, + {"million", 1_000_000, "1.0M"}, + {"fractional million", 1_200_000, "1.2M"}, + {"tens of millions", 12_300_000, "12.3M"}, + {"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)) + }) } } @@ -473,53 +605,20 @@ func TestRenderStruct_EmbeddedStruct(t *testing.T) { Engine string `console:"header:Engine"` } - type Extended struct { - Base - 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 { + type OptionalBase struct { Name string `console:"header:Name"` Engine string `console:"header:Engine,omitempty"` } type Extended struct { - *Base + Base Status string `console:"header:Status"` } - data := Extended{ - Base: &Base{Name: "wf"}, - Status: "active", + type WithPointer struct { + *OptionalBase + Status string `console:"header:Status"` } - - 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 +633,52 @@ 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"}, + }, + { + 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{OptionalBase: &OptionalBase{Name: "wf"}, Status: "active"}, + contains: []string{"wf", "active"}, + notContains: []string{"Engine", "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: "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 +695,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") + } + }) + } }