test: cover transfer eligibility; credit cross-package coverage - #29
Merged
Conversation
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.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
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.
Two changes, both driven by looking at where coverage was rather than at the aggregate.
1.
-coverpkg=./...— the per-file numbers were wronggo testcredits coverage only to the package under test, so code exercised from another package's tests counts for nothing. That made several files look untested when they're among the best-covered in the repo:cmd/cmdutil/cmdutil.gocmd/cmdutil/errors.gocmd/cmdutil/confirm.gointernal/output/output.gointernal/api/client.goEvery one is called constantly by the command tests.
This isn't about the badge — the aggregate only moves 63.6% → 66.7%. It's that
cmdutil.go 0%would send the next person to write tests that already exist. I nearly did exactly that before checking.Cost: ~9% on the test job (21.0s → 22.9s measured locally with
-race).2.
runEligibilitywas at 0.0%The whole command was untested. It's what 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 can't succeed.
Ten cases, 0.0% → 95.2% (
transfer.go58.4% → 64.9%):/core/v1/transfers/eligibility/{domain}, domain canonicalized (EXAMPLE.COM→example.com)atName=false→ recommendstransfer create, never mentions internal-inatName=true→ recommendstransfer internal-inwith the "requires enterprise reseller approval" caveat. Without it, ordinary users get sent to a call that 403s for everyone outside the allowlist — the exact regression the caveat was added foratName=true+supportsInternalTransfer=false→ still internal-in, pinning that the TLD-level flag doesn't flip the recommendation on its ownVerification
Mutation-tested, not just passing. Two mutations, both caught:
if result.AtNameSo the assertions fail for the reason they claim, rather than passing incidentally.
golangci-lint run— no issuesgo test -race -count=1 ./...— full suite cleango vet ./...cleanWhat's next, if you want it
From the corrected profile, the remaining gaps in priority order:
cmd/domain/requirements.go(26%, TLD registration rules), then a mechanical error-path sweep acrosscmd/dns,cmd/url,cmd/email(~66–71% each, same shape of gap).cmd/help.go,cmd/cmdutil/complete.go, andinternal/updateare all 0% but are help text, shell completion, and a network version check — number-movement rather than confidence, worth deciding on deliberately.