From a03457ef6b595cc8e6ad7d5f34c6b7da8884c3f2 Mon Sep 17 00:00:00 2001 From: patramsey Date: Sun, 2 Aug 2026 13:43:27 -0600 Subject: [PATCH] test: cover transfer eligibility; credit cross-package coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two changes, both from looking at where coverage actually was rather than at the aggregate. -coverpkg=./... in CI --------------------- `go test` credits coverage only to the package under test, so code exercised from another package's tests counts for nothing. That made the per-file numbers wrong in a way that would misdirect whoever read them next: cmd/cmdutil/cmdutil.go 0% -> 85% cmd/cmdutil/errors.go 17% -> 78% cmd/cmdutil/confirm.go 42% -> 83% internal/output/output.go 30% -> 59% internal/api/client.go 70% -> 93% Every one of those is called constantly by the command tests. The aggregate only moves 63.6% -> 66.7%, so this is not about the badge — it is that "cmdutil.go 0%" would send someone to write tests that already exist. Costs ~9% on the test job (21.0s -> 22.9s locally, with -race). transfer eligibility tests -------------------------- runEligibility was at 0.0%: the whole command was untested. It is the command someone runs *before* committing to a transfer, and its entire job is telling them which command to run next, so a wrong recommendation sends them to a call that cannot succeed. Ten cases now cover it (0.0% -> 95.2%, transfer.go 58.4% -> 64.9%): - the request reaches /core/v1/transfers/eligibility/{domain}, with the domain canonicalized (EXAMPLE.COM -> example.com) - atName=false recommends `transfer create` and does not mention internal-in - atName=true recommends `transfer internal-in` and carries the "requires enterprise reseller approval" caveat, without which ordinary users are sent to a call that 403s for everyone outside the allowlist - atName=true with supportsInternalTransfer=false still recommends internal-in, pinning that the TLD-level flag does not flip the recommendation on its own - JSON and YAML carry the fields and omit the human hint, which would not be valid alongside the document - a 404 surfaces the API's message rather than a bare status Verified by mutation, not just by passing: inverting the `if result.AtName` branch and deleting the approval caveat each fail the suite. The assertions fail for the reason they claim. --- .github/workflows/ci.yml | 13 ++- cmd/transfer/transfer_test.go | 175 ++++++++++++++++++++++++++++++++++ 2 files changed, 187 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6106b64..edf6405 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -37,7 +37,18 @@ jobs: # The coverage profile rides along on this run rather than getting its # own: a second full `go test` would roughly double this job for a # number the first run already has. - run: go test -race -count=1 -coverprofile=coverage.out ./... + # + # -coverpkg=./... credits a package for code its tests exercise in + # OTHER packages. Without it `go test` only ever credits the package + # under test, which made the per-file numbers actively wrong here: + # cmd/cmdutil/cmdutil.go reported 0% while being called by nearly every + # command test (it is 85%), errors.go 17% (78%), confirm.go 42% (83%), + # internal/output 30% (59%), internal/api/client.go 70% (93%). The + # aggregate only moves 63.6% -> 66.7%, so this is not about the badge — + # it is that "0%" on a well-tested file sends whoever reads it next to + # write tests that already exist. Costs about 9% on this job + # (21.0s -> 22.9s measured locally with -race). + run: go test -race -count=1 -coverpkg=./... -coverprofile=coverage.out ./... - name: Upload coverage # No token: Codecov accepts tokenless uploads from public repositories diff --git a/cmd/transfer/transfer_test.go b/cmd/transfer/transfer_test.go index 01a9d31..74eec25 100644 --- a/cmd/transfer/transfer_test.go +++ b/cmd/transfer/transfer_test.go @@ -10,6 +10,7 @@ import ( "path/filepath" "regexp" "strings" + "sync" "testing" "github.com/patramsey/namecom-cli/cmd/cmdutil" @@ -664,3 +665,177 @@ func TestWatchTransfer_ProgressDoesNotCorruptStructuredOutput(t *testing.T) { }) } } + +// ---- transfer eligibility --------------------------------------------------- + +// eligibilityServer serves a fixed eligibility response and records the request +// path, so tests can assert both what was rendered and what was asked for. +func eligibilityServer(t *testing.T, body string) (*httptest.Server, func() string) { + t.Helper() + var mu sync.Mutex + var path string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + path = r.URL.Path + mu.Unlock() + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(body)) + })) + t.Cleanup(srv.Close) + return srv, func() string { + mu.Lock() + defer mu.Unlock() + return path + } +} + +func cmdForEligibility(t *testing.T, srv *httptest.Server, format output.Format, stdout *bytes.Buffer) *cobra.Command { + t.Helper() + client, err := api.New(api.Options{BaseURL: srv.URL}) + if err != nil { + t.Fatalf("api.New: %v", err) + } + out := &output.Config{Format: format, Color: output.ColorNever, Writer: stdout, EWriter: &bytes.Buffer{}} + cmd := &cobra.Command{} + ctx := context.WithValue(context.Background(), cmdutil.KeyOutput, out) + ctx = context.WithValue(ctx, cmdutil.KeyClient, client) + cmd.SetContext(ctx) + return cmd +} + +func TestTransferEligibility_BadDomain(t *testing.T) { + cmd := cmdForEligibility(t, neverCalledServer(t), output.FormatTable, &bytes.Buffer{}) + if err := runEligibility(cmd, []string{"nodot"}); err == nil { + t.Fatal("expected error for domain without a dot, got nil") + } +} + +func TestTransferEligibility_HitsEligibilityEndpoint(t *testing.T) { + srv, reqPath := eligibilityServer(t, + `{"domainName":"example.com","atName":false,"supportsInternalTransfer":false}`) + cmd := cmdForEligibility(t, srv, output.FormatTable, &bytes.Buffer{}) + + // EXAMPLE.COM, not example.com: this also pins that the domain is + // canonicalized before it reaches the path, the same way transfer create is. + if err := runEligibility(cmd, []string{"EXAMPLE.COM"}); err != nil { + t.Fatalf("runEligibility: %v", err) + } + if got, want := reqPath(), "/core/v1/transfers/eligibility/example.com"; got != want { + t.Errorf("request path = %q, want %q", got, want) + } +} + +// The hint is the whole point of this command: someone runs it to find out +// which transfer command to run next. Recommending internal-in to a user whose +// domain is not at name.com sends them to a call that cannot succeed, and +// recommending it without the approval caveat sends ordinary users to a 403. +func TestTransferEligibility_RecommendsTheRightNextCommand(t *testing.T) { + tests := []struct { + name string + body string + wantCmd string + wantAbsent string + wantCaveat bool + wantBadges []string + }{ + { + name: "not at name.com recommends transfer create", + body: `{"domainName":"example.com","atName":false,"supportsInternalTransfer":false}`, + wantCmd: "transfer create example.com", + wantAbsent: "internal-in", + wantBadges: []string{"no"}, + }, + { + name: "at name.com recommends internal-in, with the approval caveat", + body: `{"domainName":"example.com","atName":true,"supportsInternalTransfer":true}`, + wantCmd: "transfer internal-in example.com", + wantAbsent: "transfer create", + wantCaveat: true, + wantBadges: []string{"yes"}, + }, + { + // atName drives the recommendation; supportsInternalTransfer is a + // TLD-level flag that must not flip it on its own. + name: "at name.com but TLD lacks internal support still recommends internal-in", + body: `{"domainName":"example.com","atName":true,"supportsInternalTransfer":false}`, + wantCmd: "transfer internal-in example.com", + wantAbsent: "transfer create", + wantCaveat: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + srv, _ := eligibilityServer(t, tt.body) + var stdout bytes.Buffer + cmd := cmdForEligibility(t, srv, output.FormatTable, &stdout) + if err := runEligibility(cmd, []string{"example.com"}); err != nil { + t.Fatalf("runEligibility: %v", err) + } + got := stdout.String() + if !strings.Contains(got, tt.wantCmd) { + t.Errorf("output should recommend %q, got:\n%s", tt.wantCmd, got) + } + if strings.Contains(got, tt.wantAbsent) { + t.Errorf("output should NOT mention %q, got:\n%s", tt.wantAbsent, got) + } + if tt.wantCaveat && !strings.Contains(got, "enterprise reseller approval") { + t.Errorf("internal-in recommendation must carry the approval caveat, got:\n%s", got) + } + if !tt.wantCaveat && strings.Contains(got, "enterprise reseller approval") { + t.Errorf("transfer create recommendation should not carry the approval caveat, got:\n%s", got) + } + for _, b := range tt.wantBadges { + if !strings.Contains(got, b) { + t.Errorf("table should render badge %q, got:\n%s", b, got) + } + } + if !strings.Contains(got, "example.com") { + t.Errorf("table should show the domain, got:\n%s", got) + } + }) + } +} + +// Structured output must carry the machine-readable fields and must not carry +// the human hint, which would not be valid JSON/YAML alongside the document. +func TestTransferEligibility_StructuredOutput(t *testing.T) { + for _, format := range []output.Format{output.FormatJSON, output.FormatYAML} { + t.Run(string(format), func(t *testing.T) { + srv, _ := eligibilityServer(t, + `{"domainName":"example.com","atName":true,"supportsInternalTransfer":true}`) + var stdout bytes.Buffer + cmd := cmdForEligibility(t, srv, format, &stdout) + if err := runEligibility(cmd, []string{"example.com"}); err != nil { + t.Fatalf("runEligibility: %v", err) + } + got := stdout.String() + for _, want := range []string{"example.com", "true"} { + if !strings.Contains(got, want) { + t.Errorf("%s output missing %q, got:\n%s", format, want, got) + } + } + if strings.Contains(got, "Run 'namecom") { + t.Errorf("%s output must not contain the human hint, got:\n%s", format, got) + } + }) + } +} + +func TestTransferEligibility_APIError(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message":"Domain not found"}`)) + })) + t.Cleanup(srv.Close) + + cmd := cmdForEligibility(t, srv, output.FormatTable, &bytes.Buffer{}) + err := runEligibility(cmd, []string{"example.com"}) + if err == nil { + t.Fatal("expected an error for a 404 response, got nil") + } + if !strings.Contains(err.Error(), "Domain not found") { + t.Errorf("error should surface the API message, got: %v", err) + } +}