refactor: remove listFunc stub from cmd/auth/list#435
Conversation
Remove the package-level `listFunc` variable that was used to stub `auth.List` in tests. The command now calls `auth.List` directly, and tests mock at the dependency layer (`Auth().Auths()`) instead, exercising the full command code path.
mwbrooks
added
code health
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #435 +/- ##
==========================================
+ Coverage 70.27% 70.37% +0.10%
==========================================
Files 220 220
Lines 18498 18498
==========================================
+ Hits 12999 13018 +19
+ Misses 4324 4305 -19
Partials 1175 1175 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mwbrooks
left a comment
mwbrooks
left a comment
There was a problem hiding this comment.
A few comments for guidance and celebration 🎉
| // Create handle to the function for testing | ||
| // TODO - Stopgap until we learn the correct way to structure our code for testing. | ||
| var listFunc = auth.List | ||
|
|
There was a problem hiding this comment.
🪵 praise: These TODO have been unsolved for some time I recall! Thanks for finding good pattern.
| func runListCommand(cmd *cobra.Command, clients *shared.ClientFactory) error { | ||
| ctx := cmd.Context() | ||
| userAuthList, err := listFunc(ctx, clients) | ||
| userAuthList, err := authpkg.List(ctx, clients) |
There was a problem hiding this comment.
note: This is the important line that removes the listFunc stub.
| t.Run(name, func(t *testing.T) { | ||
| ctx := slackcontext.MockContext(t.Context()) | ||
| clientsMock := shared.NewClientsMock() | ||
| clientsMock.Auth.On("Auths", mock.Anything).Return(tc.auths, nil) |
There was a problem hiding this comment.
🌟 praise: Love to see this! I understand this has significant increase to test coverage:
+0.10%
| // Create handle to the function for testing | ||
| // TODO - Stopgap until we learn the correct way to structure our code for testing. | ||
| var listFunc = auth.List | ||
|
|
There was a problem hiding this comment.
🪵 praise: These TODO have been unsolved for some time I recall! Thanks for finding good pattern.
2 participants
Changelog
Summary
This pull request remove the cmd-level
listFuncvariable that was used to stubauth.Listin tests.The command now calls
auth.Listdirectly and tests mock at the dependency layer (Auth().Auths()), which exercises the full command code path.The motivation is to test the waters on removing the
xFuncfunction stubs found incmd/to allow the command tests to call directly intointernal/pkg/x. This is one of the easiest commands that seems low risk.Test Steps
Requirements