test: cover help rendering and the output layer's suppression rules - #33
Merged
Conversation
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.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
cmd/help.gointernal/output/output.goOn my earlier objection
I argued against testing help output, on the grounds that it becomes a change-detector — failing whenever someone rewords a string, never when anything breaks. That objection is about how the tests are written, not whether the code should be tested, so these assert structure rather than wording:
printFilteredFlagsshows only what the allow list permitsLongfalls back toShort; a leaf command with no subcommands and no flags doesn't panicReword any description in this repo and these still pass. Delete a command from the list and they fail.
essentialGlobalFlagNamesis the one place a literal name is pinned, deliberately:--dry-runand--yesgate destructive actions, so dropping either from subcommand help hides the safety controls on exactly the pages where someone is about to mutate something.The output layer is all about suppression
Nearly every one of these guards something that must not appear:
Hintis silent outside table mode — emitted into JSON it corrupts the document a caller is about to parseWarnBoxdegrades 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 weightStop/Updatestay safe to call. Stopping twice doesn't panicYAMLListomitsnextPagewhen there is no next page, so callers test presence rather than comparing to zeroDefaultConfigpicks JSON when stdout isn't a terminal — the contract every piped invocation depends onOne test was worthless, and mutation testing caught it
The
NO_COLORpresence cases passed vacuously. Undergo teststdout isn't a TTY, soColorEnabled()returns false whateverNO_COLORdoes — a boolean misreading of the spec passed just as happily as the correct presence check.They now set
CLICOLOR_FORCE=1alongside, which makes the two readings diverge: correct code returns false becauseNO_COLORis checked first, a boolean reading returns true. The assertion can now fail, and it pins thatNO_COLORoutranksCLICOLOR_FORCEinto the bargain.Verification
Eleven mutations, all caught:
NO_COLORread as booleanHintleaks into structured outputWarnBoxgoes silentCLICOLOR_FORCEignoredDefaultConfigalways tableYAMLListemits zeronextPage--dry-rundropped from helpAlso fixed a fixture bug the tests surfaced immediately: the first
helpFixturebuilt subcommands without aRun, and cobra'sIsAvailableCommandreports false for those — so they were excluded from help for the wrong reason and the assertions would have passed vacuously.golangci-lint run— no issuesgo test -race -count=1 ./...— full suite clean-v