From cec9b746387d5382c3dc0612af4595d403f29890 Mon Sep 17 00:00:00 2001 From: patramsey Date: Sun, 2 Aug 2026 17:34:18 -0600 Subject: [PATCH] test: cover help rendering and the output layer's suppression rules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cmd/help.go 0% -> 68.8%, internal/output/output.go 64.2% -> 70.3%, repo 69.2% -> 71.4%. I argued against this on the grounds that help tests become change-detectors — failing whenever someone rewords a string, never when anything breaks. That objection is about HOW the tests are written, not whether the code can be tested, so these assert structure rather than wording: - Available commands appear; hidden ones do not. A hidden command in help advertises something unsupported. - Hidden flags are not rendered, and flag names, shorthands and usage strings survive. - printFilteredFlags shows only what the allow list permits. - With colour off, no escape sequence reaches the writer — help is routinely piped into a file, a pager, or an agent. - Long falls back to Short; a leaf command with no subcommands and no flags does not panic. essentialGlobalFlagNames is the one place a literal name is pinned, and deliberately: --dry-run and --yes are the flags that gate destructive actions, so dropping either from subcommand help hides the safety controls on exactly the pages where someone is about to mutate something. On the output side the theme is suppression — nearly every one of these guards something that must NOT appear: - Hint is silent outside table mode; emitted into JSON it corrupts the document a caller is about to parse. - WarnBox degrades to plain prefixed lines rather than drawing a box into a pipe, but must not go silent — its warnings are the ones that earned extra weight. - Spinners are inert on a non-TTY, and Stop/Update stay safe to call. Stopping twice does not panic. - YAMLList omits nextPage when there is no next page, so callers can test for presence rather than compare against zero. - DefaultConfig picks JSON when stdout is not a terminal, which is the contract every piped invocation depends on. One test was initially worthless and mutation testing caught it. The NO_COLOR presence cases passed vacuously: under `go test` stdout is not a TTY, so ColorEnabled returns false whatever NO_COLOR does, and a boolean reading of it passed too. They now set CLICOLOR_FORCE=1 alongside, which makes the two readings diverge — correct code returns false because NO_COLOR is checked first, a boolean reading returns true — so the assertion can fail, and it pins that NO_COLOR outranks CLICOLOR_FORCE into the bargain. Eleven mutations, all caught: NO_COLOR read as a boolean, the two env vars' precedence swapped, CLICOLOR_FORCE ignored, DefaultConfig always returning table, YAMLList emitting a zero nextPage, Hint leaking into structured output, WarnBox going silent, hidden commands and hidden flags leaking into help, and --dry-run dropped from subcommand help. Also fixed a fixture bug the tests surfaced immediately: the first helpFixture built subcommands without a Run, and cobra's IsAvailableCommand reports false for those, so they were excluded from help for the wrong reason and the assertions would have passed vacuously. --- cmd/help_test.go | 195 +++++++++++++++++++++++++++++ internal/output/output_test.go | 217 +++++++++++++++++++++++++++++++++ 2 files changed, 412 insertions(+) create mode 100644 cmd/help_test.go diff --git a/cmd/help_test.go b/cmd/help_test.go new file mode 100644 index 0000000..81c9059 --- /dev/null +++ b/cmd/help_test.go @@ -0,0 +1,195 @@ +package cmd + +import ( + "bytes" + "regexp" + "strings" + "testing" + + "github.com/charmbracelet/lipgloss" + "github.com/spf13/cobra" + "github.com/spf13/pflag" +) + +// noStyle is the identity style function, so tests assert on the text +// printFlags produces rather than on lipgloss rendering. +func noStyle(_ lipgloss.Style, s string) string { return s } + +// ansiRE strips SGR escape sequences so a coloured rendering can be compared +// for content rather than for bytes. +var ansiRE = regexp.MustCompile(`\x1b\[[0-9;]*m`) + +func stripANSI(s string) string { return ansiRE.ReplaceAllString(s, "") } + +// These assert the STRUCTURE of help output, not its wording. Asserting exact +// phrasing would produce a change-detector: it would fail every time someone +// reworded a description and never when something actually broke. What is +// pinned here is what a user would notice missing — a command absent from the +// list, a hidden command leaking, flags losing their descriptions, colour +// escapes leaking into a pipe. + +func helpFixture() *cobra.Command { + root := &cobra.Command{ + Use: "namecom", + Short: "short description", + Long: "namecom is the command-line interface for name.com", + } + // Run matters: cobra's IsAvailableCommand reports false for a command with + // no Run and no subcommands, so a fixture without it is excluded from help + // for the wrong reason and the assertions below would pass vacuously. + noop := func(*cobra.Command, []string) {} + root.AddCommand( + &cobra.Command{Use: "domain", Short: "manage domains", Run: noop}, + &cobra.Command{Use: "dns", Short: "manage DNS records", Run: noop}, + &cobra.Command{Use: "secret", Short: "hidden thing", Hidden: true, Run: noop}, + ) + root.Flags().StringP("output", "o", "table", "output format") + root.Flags().Bool("dry-run", false, "print the request without sending it") + root.Flags().String("internal", "", "not for users") + _ = root.Flags().MarkHidden("internal") + return root +} + +func TestPrintHelp_ListsAvailableCommandsAndHidesHidden(t *testing.T) { + var buf bytes.Buffer + printHelp(&buf, helpFixture(), false) + got := buf.String() + + for _, want := range []string{"domain", "dns"} { + if !strings.Contains(got, want) { + t.Errorf("help output should list the %q command, got:\n%s", want, got) + } + } + // A hidden command appearing in help is a real defect: it advertises + // something unsupported. + if strings.Contains(got, "secret") { + t.Errorf("help output must not list hidden commands, got:\n%s", got) + } + if strings.Contains(got, "hidden thing") { + t.Errorf("help output must not list a hidden command's description, got:\n%s", got) + } +} + +func TestPrintHelp_IncludesDescriptionAndUsage(t *testing.T) { + var buf bytes.Buffer + cmd := helpFixture() + printHelp(&buf, cmd, false) + got := buf.String() + + if !strings.Contains(got, cmd.Long) { + t.Errorf("help should include the long description, got:\n%s", got) + } + if !strings.Contains(got, cmd.UseLine()) { + t.Errorf("help should include the usage line %q, got:\n%s", cmd.UseLine(), got) + } +} + +// Short is the fallback when a command has no Long. A command whose help +// renders with no description at all is the failure this prevents. +func TestPrintHelp_FallsBackToShortDescription(t *testing.T) { + cmd := &cobra.Command{Use: "solo", Short: "the only description there is"} + var buf bytes.Buffer + printHelp(&buf, cmd, false) + if !strings.Contains(buf.String(), "the only description there is") { + t.Errorf("help should fall back to Short when Long is empty, got:\n%s", buf.String()) + } +} + +// Help is routinely piped (`namecom --help | less`, into a file, into an +// agent). Escape sequences leaking through when colour is off corrupts all of +// those. +func TestPrintHelp_ColorFlagControlsEscapeSequences(t *testing.T) { + var plain, colored bytes.Buffer + printHelp(&plain, helpFixture(), false) + printHelp(&colored, helpFixture(), true) + + if strings.ContainsRune(plain.String(), '\x1b') { + t.Errorf("colour disabled must produce no escape sequences, got:\n%q", plain.String()) + } + // Both renderings must still carry the same information. + for _, want := range []string{"domain", "dns", "output"} { + if !strings.Contains(stripANSI(colored.String()), want) { + t.Errorf("coloured help lost %q", want) + } + } +} + +// Commands with no subcommands and no flags must not panic or emit a stray +// empty "Commands:"/"Flags:" section. +func TestPrintHelp_LeafCommandDoesNotPanic(t *testing.T) { + var buf bytes.Buffer + printHelp(&buf, &cobra.Command{Use: "leaf", Short: "a leaf", Run: func(*cobra.Command, []string) {}}, false) + if buf.Len() == 0 { + t.Error("help for a leaf command produced no output at all") + } +} + +func TestStyledHelp_WritesToCommandOutput(t *testing.T) { + cmd := helpFixture() + var buf bytes.Buffer + cmd.SetOut(&buf) + styledHelp(cmd, nil) + if !strings.Contains(buf.String(), "domain") { + t.Errorf("styledHelp should write help to the command's output writer, got:\n%s", buf.String()) + } +} + +// ---- flag rendering --------------------------------------------------------- + +func TestPrintFlags_RendersNamesShorthandsAndUsage(t *testing.T) { + fs := pflag.NewFlagSet("t", pflag.ContinueOnError) + fs.StringP("output", "o", "table", "output format") + fs.Bool("yes", false, "skip confirmation prompts") + fs.String("secret", "", "should not appear") + _ = fs.MarkHidden("secret") + + var buf bytes.Buffer + printFlags(&buf, fs, false, noStyle) + got := buf.String() + + for _, want := range []string{"--output", "-o", "output format", "--yes", "skip confirmation prompts"} { + if !strings.Contains(got, want) { + t.Errorf("flag output missing %q, got:\n%s", want, got) + } + } + // A hidden flag in help advertises something unsupported. + if strings.Contains(got, "--secret") { + t.Errorf("hidden flags must not be rendered, got:\n%s", got) + } +} + +// Subcommand help shows a curated subset of global flags. --dry-run and --yes +// are pinned by name deliberately: they are the flags that gate destructive +// actions, and dropping either from subcommand help hides the safety controls +// from exactly the pages where someone is about to mutate something. +func TestEssentialGlobalFlagNames_IncludesTheSafetyFlags(t *testing.T) { + essential := essentialGlobalFlagNames() + for _, want := range []string{"dry-run", "yes", "output", "quiet"} { + if !essential[want] { + t.Errorf("--%s should be shown on subcommand help pages", want) + } + } + // It is a filter, not a passthrough — if it ever returns everything, the + // filtering below stops meaning anything. + if essential["debug"] || essential["token"] { + t.Error("noisy flags (--debug, --token) should be filtered out of subcommand help") + } +} + +func TestPrintFilteredFlags_ShowsOnlyAllowedFlags(t *testing.T) { + fs := pflag.NewFlagSet("t", pflag.ContinueOnError) + fs.String("output", "table", "output format") + fs.Bool("dry-run", false, "print the request without sending it") + fs.String("token", "", "API token") + + var buf bytes.Buffer + printFilteredFlags(&buf, fs, map[string]bool{"output": true, "dry-run": true}, false, noStyle) + got := buf.String() + + if !strings.Contains(got, "--output") || !strings.Contains(got, "--dry-run") { + t.Errorf("allowed flags should be rendered, got:\n%s", got) + } + if strings.Contains(got, "--token") { + t.Errorf("a flag outside the allow list must not be rendered, got:\n%s", got) + } +} diff --git a/internal/output/output_test.go b/internal/output/output_test.go index 5771acd..d5b0cc4 100644 --- a/internal/output/output_test.go +++ b/internal/output/output_test.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/json" "errors" + "os" "strings" "testing" "time" @@ -490,3 +491,219 @@ func TestRedAmber_ColorWrapsButPreservesText(t *testing.T) { t.Errorf("Amber = %q, want it to still contain %q", got, "careful") } } + +// ---- DefaultConfig ---------------------------------------------------------- + +// DefaultConfig decides the output format from whether stdout is a terminal. +// Under `go test` stdout is a pipe, which is the same situation as any script +// or agent invoking the CLI — so this asserts the machine-readable default that +// piping is supposed to produce. +func TestDefaultConfig_NonTTYDefaultsToJSON(t *testing.T) { + c := DefaultConfig() + if c.Format != FormatJSON { + t.Errorf("Format = %q with a non-TTY stdout, want %q — piped output must be machine-readable", c.Format, FormatJSON) + } + if c.Color != ColorAuto { + t.Errorf("Color = %q, want %q", c.Color, ColorAuto) + } + if c.Writer == nil || c.EWriter == nil { + t.Error("DefaultConfig must populate both writers") + } +} + +// ---- ColorEnabled ----------------------------------------------------------- + +// NO_COLOR is presence-based by specification (https://no-color.org): an empty +// value still disables colour. Treating it as a boolean would re-enable colour +// for `NO_COLOR=` and `NO_COLOR=0`, which the spec explicitly forbids. +// +// Every presence case below ALSO sets CLICOLOR_FORCE=1. Without it these +// assertions are vacuous: under `go test` stdout is not a TTY, so ColorEnabled +// falls through to false no matter what NO_COLOR does, and a boolean reading of +// NO_COLOR passes anyway. With CLICOLOR_FORCE=1 the two readings diverge — +// correct code returns false because NO_COLOR is checked first, a boolean +// reading returns true — so the test can fail, and it simultaneously pins that +// NO_COLOR outranks CLICOLOR_FORCE. +func TestColorEnabled_ExplicitModesIgnoreEnv(t *testing.T) { + t.Setenv("NO_COLOR", "1") + if !(&Config{Color: ColorAlways}).ColorEnabled() { + t.Error("ColorAlways must ignore NO_COLOR") + } + _ = os.Unsetenv("NO_COLOR") + t.Setenv("CLICOLOR_FORCE", "1") + if (&Config{Color: ColorNever}).ColorEnabled() { + t.Error("ColorNever must ignore CLICOLOR_FORCE") + } +} + +func TestColorEnabled_NoColorIsPresenceBasedAndOutranksForce(t *testing.T) { + for _, val := range []string{"1", "0", "", "false", "no"} { + t.Run("NO_COLOR="+val, func(t *testing.T) { + t.Setenv("CLICOLOR_FORCE", "1") + t.Setenv("NO_COLOR", val) + if (&Config{Color: ColorAuto}).ColorEnabled() { + t.Errorf("NO_COLOR=%q must disable colour even with CLICOLOR_FORCE=1 — "+ + "presence disables, regardless of value", val) + } + }) + } +} + +func TestColorEnabled_ForceEnablesWithoutNoColor(t *testing.T) { + _ = os.Unsetenv("NO_COLOR") + t.Setenv("CLICOLOR_FORCE", "1") + if !(&Config{Color: ColorAuto}).ColorEnabled() { + t.Error("CLICOLOR_FORCE=1 should enable colour when NO_COLOR is absent") + } +} + +func TestColorEnabled_AutoIsOffWhenPipedWithNoEnv(t *testing.T) { + _ = os.Unsetenv("NO_COLOR") + _ = os.Unsetenv("CLICOLOR_FORCE") + if (&Config{Color: ColorAuto}).ColorEnabled() { + t.Error("ColorAuto should be off when stdout is a pipe and no env forces it") + } +} + +// ---- YAMLList --------------------------------------------------------------- + +// The pagination envelope is a documented output contract: nextPage is omitted +// when there is no next page, so a script can test for its presence rather than +// comparing it to zero. +func TestYAMLList_PaginationEnvelope(t *testing.T) { + tests := []struct { + name string + nextPage *int32 + total int32 + wantNext bool + }{ + {"no next page", nil, 3, false}, + {"explicit zero is not a next page", int32Ptr(0), 3, false}, + {"a real next page is included", int32Ptr(2), 30, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var buf bytes.Buffer + c := &Config{Format: FormatYAML, Color: ColorNever, Writer: &buf, EWriter: &bytes.Buffer{}} + if err := c.YAMLList([]string{"a", "b"}, tt.nextPage, tt.total); err != nil { + t.Fatalf("YAMLList: %v", err) + } + got := buf.String() + if strings.Contains(got, "nextPage") != tt.wantNext { + t.Errorf("nextPage present = %v, want %v, got:\n%s", !tt.wantNext, tt.wantNext, got) + } + if !strings.Contains(got, "data:") { + t.Errorf("envelope should carry a data key, got:\n%s", got) + } + }) + } +} + +func int32Ptr(i int32) *int32 { return &i } + +// ---- Hint / WarnBox suppression -------------------------------------------- + +// Hint is commentary. Emitting it in JSON or YAML mode would corrupt the +// document that a caller is about to parse. +func TestHint_OnlyInTableMode(t *testing.T) { + for _, f := range []Format{FormatJSON, FormatYAML} { + var buf bytes.Buffer + c := &Config{Format: f, Color: ColorNever, Writer: &buf, EWriter: &bytes.Buffer{}} + c.Hint("run something else") + if buf.Len() != 0 { + t.Errorf("Hint must be silent in %s mode, got: %q", f, buf.String()) + } + } + var buf bytes.Buffer + c := &Config{Format: FormatTable, Color: ColorNever, Writer: &buf, EWriter: &bytes.Buffer{}} + c.Hint("run something else") + if !strings.Contains(buf.String(), "run something else") { + t.Errorf("Hint should print in table mode, got: %q", buf.String()) + } +} + +// WarnBox degrades to plain prefixed lines outside table mode rather than +// drawing a box into a pipe — but it must not go silent, because the warnings +// it carries are the ones that warrant extra weight. +func TestWarnBox_DegradesButStaysVisible(t *testing.T) { + for _, f := range []Format{FormatJSON, FormatYAML} { + var errBuf bytes.Buffer + c := &Config{Format: f, Color: ColorNever, Writer: &bytes.Buffer{}, EWriter: &errBuf} + c.WarnBox("first line", "second line") + got := errBuf.String() + for _, want := range []string{"first line", "second line"} { + if !strings.Contains(got, want) { + t.Errorf("WarnBox lost %q in %s mode, got: %q", want, f, got) + } + } + if strings.ContainsAny(got, "╭╰│") { + t.Errorf("WarnBox must not draw a border in %s mode, got: %q", f, got) + } + } +} + +// ---- Spinners --------------------------------------------------------------- + +// Spinners animate on stderr. Under a pipe — every CI run, every script — they +// must be inert and their stop/update calls must stay safe to call anyway. +func TestSpinners_NoOpWhenNotATTY(t *testing.T) { + var errBuf bytes.Buffer + c := &Config{Format: FormatTable, Color: ColorNever, Writer: &bytes.Buffer{}, EWriter: &errBuf} + + stop := c.Spin("working…") + if stop == nil { + t.Fatal("Spin must return a callable stop function even when inert") + } + stop() + stop() // stopping twice must not panic + + s := c.StartSpinner("working…") + if s == nil { + t.Fatal("StartSpinner must return a spinner even when inert") + } + s.Update("still working…") + s.Stop() + s.Stop() + + if errBuf.Len() != 0 { + t.Errorf("spinners must write nothing to a non-TTY stderr, got: %q", errBuf.String()) + } +} + +func TestSpin_SilentInQuietAndStructuredModes(t *testing.T) { + for _, tc := range []struct { + name string + cfg *Config + }{ + {"quiet", &Config{Format: FormatTable, QuietMode: true}}, + {"json", &Config{Format: FormatJSON}}, + {"yaml", &Config{Format: FormatYAML}}, + } { + t.Run(tc.name, func(t *testing.T) { + var errBuf bytes.Buffer + tc.cfg.Writer = &bytes.Buffer{} + tc.cfg.EWriter = &errBuf + tc.cfg.Spin("working…")() + tc.cfg.StartSpinner("working…").Stop() + if errBuf.Len() != 0 { + t.Errorf("spinner should be silent, got: %q", errBuf.String()) + } + }) + } +} + +// ---- TTY predicates --------------------------------------------------------- + +// These wrap term.IsTerminal. Under `go test` all three streams are pipes, so +// the assertion is that they agree with that rather than returning a constant. +func TestTTYPredicates_ReportNonTTYUnderTest(t *testing.T) { + if IsStderrTTY() { + t.Error("IsStderrTTY() = true under `go test`, where stderr is a pipe") + } + if isStdoutTTY() { + t.Error("isStdoutTTY() = true under `go test`, where stdout is a pipe") + } + if IsInteractive() { + t.Error("IsInteractive() = true under `go test`, where stdin is not a terminal") + } +}