test: assert output for collaborator list command #16

mwbrooks · 2025-04-07T16:39:58Z

Summary

This pull request is a follow-up on PR #14 (comment) that adds tests to assert the stdout output for collaborator and collaborator list.

Requirements

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

codecov · 2025-04-07T16:42:24Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.76%. Comparing base (3c995d5) to head (d4d4bd4).

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #16      +/-   ##
==========================================
- Coverage   62.81%   62.76%   -0.05%     
==========================================
  Files         210      210              
  Lines       22053    22053              
==========================================
- Hits        13852    13841      -11     
- Misses       7121     7133      +12     
+ Partials     1080     1079       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mwbrooks

Comments are the kind reviewers 🙇🏻

mwbrooks · 2025-04-07T16:47:11Z

cmd/collaborators/collaborators_test.go

+		app             types.App
+		collaborators   []types.SlackUser
+		expectedOutputs []string


thought: We don't need to export these variables and the default is typically to not export unless it's intentional. Happy to hear your thoughts @zimeg so we can start to make this a standard across the tests. 🧠

This is a super appreciated change - thank you for considering it! 🙏 ✨

I agree that we should aim to match test setups to this format also.

mwbrooks · 2025-04-07T16:47:42Z

cmd/collaborators/collaborators_test.go

+			expectedOutputs: []string{
+				"0 collaborators",
+			},


note: Assert 0 users are listed

praise: This assertion feels really good to have in place. Thanks for suggestion that we do this @zimeg 🙇🏻

@mwbrooks Thanks so much! I'm glad we're finding meaningful cases to cover here 🧪

Suggested change

expectedOutputs: []string{

"0 collaborators",

},

expectedOutputs: []string{

" 0 collaborators",

},

🤔 Nit: I forget how this is formatted in actual outputs, but adding this space might guard against "10 collaborators".

☝️ No blocker though since we're covering the meaningful cases already. That's just a wild edge perhaps?

I'm good with this suggestion 👍🏻 - matching \d0 collaborators doesn't seem too likely, but there's no harm in the space! Commit c0354de adds your suggestion with a comment explaining why we include a space, so future maintainers don't remove it.

mwbrooks · 2025-04-07T16:48:34Z

cmd/collaborators/collaborators_test.go

+			expectedOutputs: []string{
+				"1 collaborator",
+				// User info: slackbot
+				"USLACKBOT",
+				"slackbot",
+				"[email protected]",
+				string(types.OWNER),
+			},


note: Assert 1 user is listed

mwbrooks · 2025-04-07T16:49:14Z

cmd/collaborators/collaborators_test.go

+			expectedOutputs: []string{
+				"2 collaborators",
+				// User info: slackbot
+				"USLACKBOT",
+				"slackbot",
+				"[email protected]",
+				string(types.OWNER),
+				// User info: bookworm
+				"U00READER",
+				"bookworm",
+				"[email protected]",
+				string(types.READER),
+			},


note: Assert 2 users are listed

📚 🪱 🤖 ✨

What an interesting app!

mwbrooks · 2025-04-07T16:50:02Z

cmd/collaborators/collaborators_test.go

+			output := clientsMock.GetCombinedOutput()
+			for _, expectedOutput := range tt.expectedOutputs {
+				require.Contains(t, output, expectedOutput)
+			}


note: This is the meat and potatoes that implements the assertions

mwbrooks · 2025-04-07T16:50:28Z

cmd/collaborators/list_test.go

 	"github.com/stretchr/testify/require"
 )

 func TestListCommand(t *testing.T) {


note: This files changes are identical to cmd/collaborators/collaborators_test.go

zimeg

LGTM. Lot's of great tests, @mwbrooks 🫡

Thanks so much for following up on improvements to these tests. I'm so optimistic that this sets us up for later changes well. Please feel free to merge when the time is right.

zimeg · 2025-04-07T23:14:45Z

cmd/collaborators/collaborators_test.go

+		app             types.App
+		collaborators   []types.SlackUser
+		expectedOutputs []string


This is a super appreciated change - thank you for considering it! 🙏 ✨

I agree that we should aim to match test setups to this format also.

zimeg · 2025-04-07T23:17:30Z

cmd/collaborators/collaborators_test.go

+			expectedOutputs: []string{
+				"0 collaborators",
+			},


@mwbrooks Thanks so much! I'm glad we're finding meaningful cases to cover here 🧪

zimeg · 2025-04-07T23:17:44Z

cmd/collaborators/collaborators_test.go

+			expectedOutputs: []string{
+				"0 collaborators",
+			},


Suggested change

expectedOutputs: []string{

"0 collaborators",

},

expectedOutputs: []string{

" 0 collaborators",

},

🤔 Nit: I forget how this is formatted in actual outputs, but adding this space might guard against "10 collaborators".

zimeg · 2025-04-07T23:19:09Z

cmd/collaborators/collaborators_test.go

+			expectedOutputs: []string{
+				"0 collaborators",
+			},


☝️ No blocker though since we're covering the meaningful cases already. That's just a wild edge perhaps?

zimeg · 2025-04-07T23:20:09Z

cmd/collaborators/collaborators_test.go

+			expectedOutputs: []string{
+				"2 collaborators",
+				// User info: slackbot
+				"USLACKBOT",
+				"slackbot",
+				"[email protected]",
+				string(types.OWNER),
+				// User info: bookworm
+				"U00READER",
+				"bookworm",
+				"[email protected]",
+				string(types.READER),
+			},


📚 🪱 🤖 ✨

What an interesting app!

mwbrooks · 2025-04-08T22:09:13Z

Thanks for the quick review and originally suggesting this change @zimeg! 🚀 🚀 🚀

test: assert output for collaborator list command

d4d4bd4

mwbrooks added code health M-T: Test improvements and anything that improves code health semver:patch Use on pull requests to describe the release version increment labels Apr 7, 2025

mwbrooks added this to the Next Release milestone Apr 7, 2025

mwbrooks self-assigned this Apr 7, 2025

mwbrooks commented Apr 7, 2025

View reviewed changes

mwbrooks marked this pull request as ready for review April 7, 2025 16:51

mwbrooks requested a review from a team as a code owner April 7, 2025 16:51

zimeg approved these changes Apr 7, 2025

View reviewed changes

mwbrooks added 2 commits April 8, 2025 14:23

test: not match against 10 collaborators

c0354de

Merge branch 'main' into mbrooks-test-collaborator-list-output

574d8c8

mwbrooks merged commit 02eeb55 into main Apr 8, 2025
6 checks passed

mwbrooks deleted the mbrooks-test-collaborator-list-output branch April 8, 2025 21:44

zimeg mentioned this pull request Apr 10, 2025

test: use slackcontext.MockContext(...) instead of context.Background() #28

Merged

2 tasks

test: assert output for collaborator list command #16

test: assert output for collaborator list command #16

Uh oh!

Conversation

mwbrooks commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Requirements

Uh oh!

codecov bot commented Apr 7, 2025

Codecov Report

Uh oh!

mwbrooks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zimeg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mwbrooks commented Apr 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mwbrooks commented Apr 7, 2025 •

edited

Loading