From 43bc376d2a017f9c3b4d50d1be7b518bcbb64b74 Mon Sep 17 00:00:00 2001 From: patramsey Date: Sun, 2 Aug 2026 14:36:44 -0600 Subject: [PATCH] test: cover small units; delete dead IsYes/IsDryRun MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- cmd/cmdutil/errors_test.go | 86 ++++++++++++++++ cmd/root.go | 6 -- internal/output/output_test.go | 124 +++++++++++++++++++++++ internal/update/cache_test.go | 176 +++++++++++++++++++++++++++++++++ 4 files changed, 386 insertions(+), 6 deletions(-) create mode 100644 cmd/cmdutil/errors_test.go create mode 100644 internal/update/cache_test.go diff --git a/cmd/cmdutil/errors_test.go b/cmd/cmdutil/errors_test.go new file mode 100644 index 0000000..c7a27ba --- /dev/null +++ b/cmd/cmdutil/errors_test.go @@ -0,0 +1,86 @@ +package cmdutil + +import ( + "errors" + "fmt" + "testing" + + "github.com/patramsey/namecom-cli/internal/api" +) + +// IsNotFound is what turns a bare 404 into "domain not found — run 'namecom +// domain list'" across the whole CLI. It matches through wrapping, so the +// wrapped cases matter as much as the direct one: commands routinely add +// context with %w before the error reaches a caller that checks this. +func TestIsNotFound(t *testing.T) { + notFound := &api.APIError{StatusCode: 404, Message: "Domain not found"} + + tests := []struct { + name string + err error + want bool + }{ + {"nil is not a 404", nil, false}, + {"a plain error is not a 404", errors.New("boom"), false}, + {"a direct 404", notFound, true}, + {"a 404 wrapped once", fmt.Errorf("fetching domain: %w", notFound), true}, + {"a 404 wrapped twice", fmt.Errorf("outer: %w", fmt.Errorf("inner: %w", notFound)), true}, + {"a 404 behind a UsageError", &UsageError{Err: notFound}, true}, + {"403 is not a 404", &api.APIError{StatusCode: 403, Message: "Forbidden"}, false}, + {"500 is not a 404", &api.APIError{StatusCode: 500}, false}, + {"400 is not a 404", &api.APIError{StatusCode: 400}, false}, + {"an unwrapped error mentioning 404 is not a 404", errors.New("got status 404"), false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := IsNotFound(tt.err); got != tt.want { + t.Errorf("IsNotFound(%v) = %v, want %v", tt.err, got, tt.want) + } + }) + } +} + +// UsageError marks a problem with how the command was invoked, which the root +// command turns into a different exit code than an API failure. That only +// works if it stays both matchable and unwrappable. +func TestUsageError(t *testing.T) { + inner := errors.New("bad flag combination") + ue := &UsageError{Err: inner} + + if got := ue.Error(); got != inner.Error() { + t.Errorf("Error() = %q, want the inner message %q", got, inner.Error()) + } + if got := ue.Unwrap(); !errors.Is(got, inner) { + t.Errorf("Unwrap() = %v, want %v", got, inner) + } + if !errors.Is(ue, inner) { + t.Error("errors.Is(UsageError, inner) = false; the chain is broken") + } + + var target *UsageError + if !errors.As(fmt.Errorf("context: %w", ue), &target) { + t.Error("a wrapped UsageError should still be findable with errors.As") + } +} + +// NewUsageError returns nil for nil so callers can apply it straight to a +// function result. Returning a non-nil *UsageError wrapping nil would make +// `if err != nil` true on success — the classic typed-nil trap. +func TestNewUsageError(t *testing.T) { + if err := NewUsageError(nil); err != nil { + t.Errorf("NewUsageError(nil) = %v, want nil", err) + } + + inner := errors.New("boom") + err := NewUsageError(inner) + if err == nil { + t.Fatal("NewUsageError(non-nil) = nil, want an error") + } + var ue *UsageError + if !errors.As(err, &ue) { + t.Fatalf("NewUsageError returned %T, want a *UsageError", err) + } + if !errors.Is(err, inner) { + t.Error("NewUsageError should preserve the original error in the chain") + } +} diff --git a/cmd/root.go b/cmd/root.go index ad7b271..8f3ec36 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -367,12 +367,6 @@ func initContext(cmd *cobra.Command) error { return nil } -// IsYes reports whether --yes / -y was set globally (skip confirmation). -func IsYes() bool { return gf.yes } - -// IsDryRun reports whether --dry-run was set globally. -func IsDryRun() bool { return gf.dryRun } - // skipClientInit returns true for commands that don't need API credentials. func skipClientInit(cmd *cobra.Command) bool { for c := cmd; c != nil; c = c.Parent() { diff --git a/internal/output/output_test.go b/internal/output/output_test.go index 71b67c0..5771acd 100644 --- a/internal/output/output_test.go +++ b/internal/output/output_test.go @@ -366,3 +366,127 @@ func TestSuccess_StructuredFormats(t *testing.T) { } }) } + +// ---- flag value parsing ----------------------------------------------------- + +// ParseFormat and ParseColorMode validate --output and --color. Both lowercase +// their input, so mixed case must be accepted — nothing previously proved that, +// and `--output JSON` silently erroring would be a poor way to find out. +func TestParseFormat(t *testing.T) { + tests := []struct { + in string + want Format + wantErr bool + }{ + {"table", FormatTable, false}, + {"json", FormatJSON, false}, + {"yaml", FormatYAML, false}, + {"JSON", FormatJSON, false}, + {"YaMl", FormatYAML, false}, + {"TABLE", FormatTable, false}, + {"", "", true}, + {"xml", "", true}, + {"jsonl", "", true}, + {" json", "", true}, // not trimmed: a stray space is a real mistake, not a synonym + } + for _, tt := range tests { + t.Run(tt.in, func(t *testing.T) { + got, err := ParseFormat(tt.in) + if tt.wantErr { + if err == nil { + t.Fatalf("ParseFormat(%q) = %q, want an error", tt.in, got) + } + // The message has to name the valid choices; "unknown format" + // alone leaves the user guessing. + for _, want := range []string{"table", "json", "yaml"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q should list %q as a valid choice", err, want) + } + } + return + } + if err != nil { + t.Fatalf("ParseFormat(%q) unexpected error: %v", tt.in, err) + } + if got != tt.want { + t.Errorf("ParseFormat(%q) = %q, want %q", tt.in, got, tt.want) + } + }) + } +} + +func TestParseColorMode(t *testing.T) { + tests := []struct { + in string + want ColorMode + wantErr bool + }{ + {"auto", ColorAuto, false}, + {"always", ColorAlways, false}, + {"never", ColorNever, false}, + {"ALWAYS", ColorAlways, false}, + {"Never", ColorNever, false}, + {"", "", true}, + {"yes", "", true}, + {"true", "", true}, + {"none", "", true}, + } + for _, tt := range tests { + t.Run(tt.in, func(t *testing.T) { + got, err := ParseColorMode(tt.in) + if tt.wantErr { + if err == nil { + t.Fatalf("ParseColorMode(%q) = %q, want an error", tt.in, got) + } + for _, want := range []string{"auto", "always", "never"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q should list %q as a valid choice", err, want) + } + } + return + } + if err != nil { + t.Fatalf("ParseColorMode(%q) unexpected error: %v", tt.in, err) + } + if got != tt.want { + t.Errorf("ParseColorMode(%q) = %q, want %q", tt.in, got, tt.want) + } + }) + } +} + +// ---- color helpers ---------------------------------------------------------- + +// Red and Amber must return the text unchanged when color is off. A helper that +// emits escape sequences regardless would corrupt piped output and any file +// written with --debug-file. +func TestRedAmber_NoColorReturnsPlainText(t *testing.T) { + c := noColor() + for _, tt := range []struct { + name string + got string + }{ + {"Red", c.Red("failed")}, + {"Amber", c.Amber("careful")}, + } { + if strings.ContainsRune(tt.got, '\x1b') { + t.Errorf("%s with color disabled returned an escape sequence: %q", tt.name, tt.got) + } + } + if got := c.Red("failed"); got != "failed" { + t.Errorf("Red = %q, want %q unchanged", got, "failed") + } + if got := c.Amber("careful"); got != "careful" { + t.Errorf("Amber = %q, want %q unchanged", got, "careful") + } +} + +func TestRedAmber_ColorWrapsButPreservesText(t *testing.T) { + c := &Config{Color: ColorAlways} + if got := c.Red("failed"); !strings.Contains(got, "failed") { + t.Errorf("Red = %q, want it to still contain %q", got, "failed") + } + if got := c.Amber("careful"); !strings.Contains(got, "careful") { + t.Errorf("Amber = %q, want it to still contain %q", got, "careful") + } +} diff --git a/internal/update/cache_test.go b/internal/update/cache_test.go new file mode 100644 index 0000000..4553b3e --- /dev/null +++ b/internal/update/cache_test.go @@ -0,0 +1,176 @@ +package update + +import ( + "encoding/json" + "os" + "path/filepath" + "runtime" + "testing" + "time" +) + +// isolateConfigDir points os.UserConfigDir() at a temp directory. The variable +// it reads differs by platform: HOME on darwin ($HOME/Library/Application +// Support), XDG_CONFIG_HOME on unix. Setting both keeps this test honest on +// whichever one is running it rather than passing vacuously on the other. +func isolateConfigDir(t *testing.T) string { + t.Helper() + dir := t.TempDir() + t.Setenv("HOME", dir) + t.Setenv("XDG_CONFIG_HOME", filepath.Join(dir, ".config")) + if runtime.GOOS == "windows" { + t.Setenv("AppData", dir) + } + got := cacheFile() + if got == "" { + t.Fatal("cacheFile() is empty after isolating the config dir") + } + return got +} + +func TestCacheFile_UnderConfigDir(t *testing.T) { + path := isolateConfigDir(t) + if filepath.Base(path) != "version_check.json" { + t.Errorf("cache file = %q, want it to be named version_check.json", path) + } + if filepath.Base(filepath.Dir(path)) != "namecom" { + t.Errorf("cache file = %q, want it under a namecom/ directory", path) + } +} + +// The round trip is the contract: what writeCache stores, readCache returns. +func TestCache_RoundTrip(t *testing.T) { + isolateConfigDir(t) + + if got, ok := readCache(); ok { + t.Fatalf("readCache on an empty dir = (%q, true), want ok=false", got) + } + + writeCache("1.2.3") + + got, ok := readCache() + if !ok { + t.Fatal("readCache after writeCache = ok:false, want the value back") + } + if got != "1.2.3" { + t.Errorf("readCache = %q, want %q", got, "1.2.3") + } +} + +// The cache exists to keep the CLI off the network for a day. An entry older +// than the TTL must be ignored, or the check never refreshes; an entry inside +// it must be honored, or the cache does nothing and every invocation hits +// GitHub. +func TestCache_TTL(t *testing.T) { + tests := []struct { + name string + age time.Duration + wantOK bool + wantVal string + }{ + {"fresh entry is used", time.Hour, true, "1.2.3"}, + {"just inside the TTL is used", cacheTTL - time.Minute, true, "1.2.3"}, + {"just past the TTL is ignored", cacheTTL + time.Minute, false, ""}, + {"ancient entry is ignored", 30 * 24 * time.Hour, false, ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + path := isolateConfigDir(t) + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + body, err := json.Marshal(versionCache{ + CheckedAt: time.Now().Add(-tt.age), + Latest: "1.2.3", + }) + if err != nil { + t.Fatalf("Marshal: %v", err) + } + if err := os.WriteFile(path, body, 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + got, ok := readCache() + if ok != tt.wantOK { + t.Errorf("readCache ok = %v, want %v (age %s, TTL %s)", ok, tt.wantOK, tt.age, cacheTTL) + } + if got != tt.wantVal { + t.Errorf("readCache = %q, want %q", got, tt.wantVal) + } + }) + } +} + +// Every failure path in readCache is deliberately silent — a broken cache must +// degrade to "no cached value", never to an error surfacing in the CLI or a +// bogus version being reported. +func TestReadCache_BadInputIsSilentlyIgnored(t *testing.T) { + tests := []struct { + name string + content string + }{ + {"not json", "this is not json"}, + {"truncated json", `{"checked_at":`}, + {"wrong shape", `[1,2,3]`}, + {"empty file", ""}, + {"valid json, empty version", `{"checked_at":"` + time.Now().Format(time.RFC3339) + `","latest":""}`}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + path := isolateConfigDir(t) + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + if err := os.WriteFile(path, []byte(tt.content), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + if got, ok := readCache(); ok || got != "" { + t.Errorf("readCache = (%q, %v), want (\"\", false) for %s", got, ok, tt.name) + } + }) + } +} + +// The cache can hold the user's update history; it does not belong to the +// group or to other users. +func TestWriteCache_FilePermissions(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("unix permission bits do not apply on windows") + } + path := isolateConfigDir(t) + writeCache("1.2.3") + + fi, err := os.Stat(path) + if err != nil { + t.Fatalf("stat: %v", err) + } + if perm := fi.Mode().Perm(); perm&0o077 != 0 { + t.Errorf("cache file mode = %#o, want no group/other bits", perm) + } + di, err := os.Stat(filepath.Dir(path)) + if err != nil { + t.Fatalf("stat dir: %v", err) + } + if perm := di.Mode().Perm(); perm&0o077 != 0 { + t.Errorf("cache dir mode = %#o, want no group/other bits", perm) + } +} + +// writeCache swallows its errors by design; the CLI must not fail because a +// version check could not be cached. +func TestWriteCache_UnwritablePathDoesNotPanic(t *testing.T) { + path := isolateConfigDir(t) + // Occupy the directory slot with a regular file so MkdirAll must fail. + if err := os.MkdirAll(filepath.Dir(filepath.Dir(path)), 0o700); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + if err := os.WriteFile(filepath.Dir(path), []byte("not a directory"), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + writeCache("1.2.3") // must not panic + + if got, ok := readCache(); ok || got != "" { + t.Errorf("readCache = (%q, %v), want (\"\", false) when the cache could not be written", got, ok) + } +}