test: fix collaborator command test to run standalone #14
Conversation
mwbrooks
added
code health
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14 +/- ##
==========================================
+ Coverage 62.80% 62.83% +0.03%
==========================================
Files 210 210
Lines 22053 22053
==========================================
+ Hits 13850 13858 +8
+ Misses 7126 7119 -7
+ Partials 1077 1076 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| err := cmd.Execute() | ||
| if err != nil { | ||
| assert.Fail(t, "cmd.Execute had unexpected error") | ||
| tests := map[string]struct { |
There was a problem hiding this comment.
This test is a copy and paste of cmd/collaborators/list_test.go with one small change that I mention as a GitHub reviewer comment
| clients.SDKConfig = hooks.NewSDKConfigMock() | ||
| }) | ||
|
|
||
| err := NewCommand(clients).Execute() |
There was a problem hiding this comment.
Changed this from NewListCommand(clients).Execute() to NewCommand(clinets).Execute().
zimeg
left a comment
zimeg
left a comment
There was a problem hiding this comment.
@mwbrooks Even as a "copy and paste" of other tests, this is such a wonderful improvement to the health of this code.
Adding new tests becomes more familiar and we're also testing more of the actual collaborators command which is great.
I feel lucky too since I hadn't experienced this flake, but I imagine debugging it was no fun task 🫡
I'm approving this now but left one comment around additional checks that we might want to make in this PR or in another PR with future changes if it seems unrelated to this! 🚢 ✨
| for _, collaborator := range tt.Collaborators { | ||
| clientsMock.IO.AssertCalled(t, "PrintTrace", mock.Anything, slacktrace.CollaboratorListCollaborator, []string{ | ||
| collaborator.ID, | ||
| collaborator.UserName, | ||
| collaborator.Email, | ||
| string(collaborator.PermissionType), | ||
| }) | ||
| } |
There was a problem hiding this comment.
👾 I like that we're checking the stable value used in tests here instead of stdout although I do wonder if we should make note of this to follow back up on?
I don't expect these outputs to change so often, but confirming certain values are output is somewhat nice in unit tests and IMO not so much of a hassle to update when changing related code.
No blocker at all but avoiding regression is a constant on mind ✨
There was a problem hiding this comment.
Good point! I can follow-up with another PR that updates this test for both collaborators and collaborators list 👌🏻
|
@zimeg Thanks for the quick review! It's always nice to merge these small fixes that make it a little more stable to run the tests. I'll follow-up with another small PR that adds assertions for the output based on your suggestion 🧪 |
Summary
This pull request fixes a flaky test in
cmd/collaborators.go.Previously, the test would only pass when other
package collaboratorstests ran first. After looking into the test, I realized that the test was outdated. Since thecollaboratorscommand executescollaborators listunder the hood, I copied the table test fromcollaborators list.Requirements