Repository navigation
test: derive enum members and mocked interfaces as live - #7321
Merged
Merged
Conversation
…ode check The check counted a declaration as used only where code named it, so two kinds of declarations that must stay had to be listed by hand in .deadcode-allow, and the list failed again the moment a test started naming one. Both kinds can be read off the types instead. An exported constant of a named type from its own package is an enum member. Its values often arrive by conversion, as Stripe's statuses do through SubscriptionStatus(sub.Status), so no code names the member that matches. It is now live while its type is used anywhere other than its own members' declarations, or while any sibling member is used: InvoiceStatus is never used as a type, only through string(InvoiceStatusOpen), and its uncollectible member is no less real for that. An enum whose type and members nobody uses is still reported. An interface that a type in a mocks/ package implements is mockery's input, and the generated mock never names it. It is now live, checked with types.Implements against every type the mocks declare, so .mockery.yaml is not read. An interface without methods is left out, since every mock would implement it. With both rules nothing in shellhub or cloud needs an entry, so shellhub's .deadcode-allow goes; cloud drops its own on the branch of the same name. The allowlist stays for a real exception.
Code Review CompleteThe automated review ran but did not post an updated summary — this usually means no new issues were found since the previous review. If you've pushed changes and want a fresh pass, comment |
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
1 participant
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.
Summary
The dead-code check counted a declaration as used only where code named it. Two kinds of declarations that must stay had to be listed by hand in
.deadcode-allow, and a listed one failed the check again once a test named it: cloud#2616 hit that withSubscriptionStatusCanceled. Both kinds are now derived from the types, and both allowlists go.SubscriptionStatus{Canceled,Incomplete,IncompleteExpired,Unpaid,Paused}SubscriptionStatusis used as a typeInvoiceStatusUncollectibleInvoiceStatusOpen/Draft/Paidare usedBackendstripe/mocks.MockBackendimplements itKindInvalidKindis used as a typeAn enum whose type and members nobody uses is still reported, and so is an interface nothing mocks. The allowlist mechanism stays for a real exception.
Evidence
TestUnusedDeclarations(a fixture module with a used-type enum, a used-sibling enum, an unused enum, a mocked interface and an unmocked one) reports all of them.After: it reports only the unused enum's members and the unmocked interface.
go run ./cmd/deadcode -require-cloud):no dead codeMerge Danger
Door: two-way
Blast Radius: dead-code check
Paired with shellhub-io/cloud#2617 on the same branch name, which deletes cloud's allowlist. Merge the two back to back: either one alone makes the scheduled sweep of both masters report the other repository's allowlist (stale entries under the new check, or dead symbols under the old one).
shellhub-io/cloud#2616 removes one line from cloud's
.deadcode-allow. Whichever of the two merges second needs a rebase.