test: cover small units; delete dead IsYes/IsDryRun - #31
Merged
Conversation
Deletion first, because it is the more useful half. cmd.IsYes() and cmd.IsDryRun() in root.go had no callers. Commands use cmdutil.IsYes(cmd)/cmdutil.IsDryRun(cmd), the context-based pair, at roughly forty sites; the root.go versions read a package-level struct and were never referenced. Package cmd is imported only by main.go, so exporting them reached nobody. They showed up as 0% coverage, which is exactly the trap: the cheap response is to write a test and turn them green, which would have preserved dead code and made the number look better for it. Confirmed dead by deleting them and building. Then the genuinely cheap units, about 55 statements, 68.3% -> 69.2%: - ParseFormat/ParseColorMode validate --output and --color. Both lower their input, so `--output JSON` should work and nothing proved it did. The tests also assert the error names the valid choices, since "unknown format" alone leaves the user guessing. " json" with a leading space is pinned as an error rather than a synonym. - IsNotFound is what turns a bare 404 into "domain not found — run 'namecom domain list'" across the CLI. It matches through wrapping, so the wrapped cases matter as much as the direct one; commands routinely add context with %w first. 403/500/400 are pinned as not-404, and a plain error whose text merely contains "404" is pinned as not matching. Worth having before anyone acts on the pending errors.AsType hint. - UsageError stays matchable and unwrappable, and NewUsageError(nil) returns a nil error rather than a non-nil interface holding a nil pointer — the typed-nil trap that would make `if err != nil` true on success. - The update cache round-trips, honors its 24h TTL on both sides of the boundary, degrades silently to "no cached value" on any malformed content, and writes 0600/0700. Every failure path there is deliberately silent, so nothing else would have noticed it breaking. Verified by mutation, ten of them, all caught: dropping either ToLower, shortening the format error, widening IsNotFound to any 4xx, forcing it false, breaking UsageError.Unwrap, disabling the TTL check, reporting an empty cached version as valid, and loosening either the cache file or cache directory permissions.
|
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.
The deletion is the more useful half
cmd.IsYes()andcmd.IsDryRun()inroot.gohad no callers. Commands usecmdutil.IsYes(cmd)/cmdutil.IsDryRun(cmd)— the context-based pair, ~40 sites. Theroot.goversions read a package-level struct and were referenced nowhere; packagecmdis imported only bymain.go, so exporting them reached no one.They showed up as 0% coverage, and that's exactly the trap: the cheap response is to write a test and turn them green, which preserves dead code and makes the number look better for it. Confirmed dead by deleting them and building.
Then the genuinely cheap units
~55 statements, 68.3% → 69.2%.
ParseFormat/ParseColorMode— validate--outputand--color. Both lowercase their input, so--output JSONshould work and nothing proved it did. Tests also assert the error names the valid choices, since "unknown format" alone leaves the user guessing." json"with a leading space is pinned as an error rather than a synonym.IsNotFound— three statements, but it's what turns a bare 404 into "domain not found — runnamecom domain list" across the CLI. It matches through wrapping, so the wrapped cases matter as much as the direct one; commands routinely add context with%wfirst. 403/500/400 pinned as not-404, and a plain error whose text contains "404" pinned as not matching. Worth having before anyone acts on the pendingerrors.AsTypecompiler hint.UsageError/NewUsageError— stays matchable and unwrappable, andNewUsageError(nil)returns a nil error rather than a non-nil interface holding a nil pointer. That's the typed-nil trap that makesif err != niltrue on success.The update cache — round-trips, honors its 24h TTL on both sides of the boundary, degrades silently to "no cached value" on any malformed content, and writes
0600/0700. Every failure path there is silent by design, so nothing else would have noticed it breaking.Verification
Ten mutations, all caught:
ToLowerinParseFormatUsageError.UnwrapToLowerinParseColorModeIsNotFoundto any 4xx0600→0644IsNotFoundfalse0700→0755golangci-lint run— no issuesgo test -race -count=1 ./...— full suite cleango build ./...passes with the dead functions removedWhere I'd stop
This is a reasonable end point for coverage work. What remains at 0% is
cmd/help.go,cmd/cmdutil/complete.go, and the interactivehuhforms — help text, shell completion, and TUI prompts. Covering those means asserting that help output contains strings you just wrote: a change-detector that fails when you reword help and never when something breaks.The code that can lose someone a domain or charge them money is the well-covered part now, which is the outcome worth having.