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

mwbrooks · 2025-04-10T21:35:23Z

Summary

This pull request is part of the series that updates our tests to use slackcontext.MockContext(...) instead of context.Background().

The motivation is to continue to have consistent and reliable unit tests. Right now, many of our test use empty context such as context.Background(), which can cause unexpected errors if a function brings to use values from the context. Now that we can mock "guaranteed" context values, this PR combs through to replace .Background() references.

You might want a fresh cup of coffee ☕ to sip while scrolling through this one!

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-10T21:37:54Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.94%. Comparing base (034ae57) to head (82f4cd6).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #28      +/-   ##
==========================================
- Coverage   62.94%   62.94%   -0.01%     
==========================================
  Files         210      210              
  Lines       22127    22127              
==========================================
- Hits        13928    13927       -1     
+ Misses       7115     7114       -1     
- Partials     1084     1086       +2

☔ 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 to (hopefully) make reviewing this a little more productive and pleasant 😃

mwbrooks · 2025-04-10T21:36:17Z

cmd/app/link_test.go

-
 			// Create mocks
+			ctx := slackcontext.MockContext(t.Context())
 			clientsMock := shared.NewClientsMock()


note: This is the standard, repetitive change that you'll see. We are swapping context.Background() for slackcontext.MockContext(t.Context). 👯

Setting up ctx with other mocks is an awesome change!

Using the testing context is a neat update also! I haven't explore enough of that package, but I imagine more useful methods might be used elsewhere too.

I like using the t.Context() as well. A few benefits are:

It makes it easy to search for context.Background() and context.TODO() to prevent those from slipping into production code.

t.Context() is a cancel context and it's cancelled just before the test cleanup. So, any goroutines running and listening for <- ctx.Done() will have a chance to shutdown.

mwbrooks · 2025-04-10T21:37:38Z

cmd/app/uninstall_test.go

 		"Successfully uninstall": {
 			Setup: func(t *testing.T, ctx context.Context, clientsMock *shared.ClientsMock, clients *shared.ClientFactory) {
-				prepareCommonUninstallMocks(clients, clientsMock)
+				prepareCommonUninstallMocks(ctx, clients, clientsMock)


note: This was missed in the PR that updated .Setup: func(t, ctx, ...) to accept the mocked context.

mwbrooks · 2025-04-10T21:40:04Z

cmd/collaborators/collaborators_test.go

+
+func TestCollaboratorsCommand_PrintSuccess(t *testing.T) {


thought: I'm a little baffled where this test came from 🤔 I didn't write it, so I assume I updated it. But I cannot find the original copy of it! Either way, it tests printSuccess and I think we want to keep it. I'm assuming it was introduced by a rebase (kinda regretting doing a rebase instead of a merge).

This does seem to happen with branches rebased but a quick thanks for making reviews much more pleasant with this 🙏

I do think we can delete these tests to make later updates a bit more clear. IIRC #16 added tests that cover these cases instead of a separate unit here- 🔍

Ohhhh, great memory! I'll remove this test then!

Commit f1d5fa8 removes this test. I've confirmed that we still have test coverage after it's been removed. Thanks @zimeg!

mwbrooks · 2025-04-10T21:42:23Z

cmd/function/access_test.go


 	for name, tt := range tests {
 		t.Run(name, func(t *testing.T) {
+			ctx := slackcontext.MockContext(t.Context())


thought: It's generally best to declare the ctx at the top of the function, so that the same context is used throughout.

Edit: I should've read on! I agree so much that this is a good change.

mwbrooks · 2025-04-10T21:44:58Z

cmd/triggers/generate_test.go


 	t.Run(tt.name, func(t *testing.T) {
-		ctx, clientsMock := prepareMocks(tt.triggersListResponse, tt.globResponse, tt.triggersCreateResponse, nil /*trigger create error*/)
+		ctx, clientsMock := prepareMocks(t, tt.triggersListResponse, tt.globResponse, tt.triggersCreateResponse, nil /*trigger create error*/)


note: We need t *testing.T in order to called slackcontext.MockContext(t.Context())

mwbrooks · 2025-04-10T21:48:42Z

internal/pkg/manifest/validate_test.go


 func Test_ManifestValidate_GetManifestLocal_Error(t *testing.T) {
-	ctx, clients, _, log, appMock, authMock := setupCommonMocks()
+	ctx, clients, _, log, appMock, authMock := setupCommonMocks(t)


note: It's not obvious that this change adds a t *testing.T argument to setupCommonMocks(t). Same reason as before, so that we can call slackcontext.MockContext(t.Context()).

mwbrooks · 2025-04-10T21:49:42Z

internal/prompts/app_select_test.go

 //

 func TestPrompt_AppSelectPrompt_SelectedAuthExpired_UserReAuthenticates(t *testing.T) {
-


vent: Personal pet-peeve is newlines at the top of a function block in Golang 😆

➕ I find these make jumping around sections sometimes a hassle- Thanks for reducing our linecounts with this change!

zimeg

@mwbrooks

Now that we can mock "guaranteed" context values, this PR combs through to replace .Background() references.

How great! And thanks for keeping these changes in one PR. The review was energized without chaos. It's so much appreciated.

I left one note on added tests that I think can be removed in these changes, but these changes add a lot of reason to the tests that exist already!

🧪 ✨ 👾

zimeg · 2025-04-10T22:50:18Z

cmd/collaborators/collaborators_test.go

+
+func TestCollaboratorsCommand_PrintSuccess(t *testing.T) {


This does seem to happen with branches rebased but a quick thanks for making reviews much more pleasant with this 🙏

I do think we can delete these tests to make later updates a bit more clear. IIRC #16 added tests that cover these cases instead of a separate unit here- 🔍

zimeg · 2025-04-10T22:51:04Z

cmd/app/link_test.go

-
 			// Create mocks
+			ctx := slackcontext.MockContext(t.Context())
 			clientsMock := shared.NewClientsMock()


Setting up ctx with other mocks is an awesome change!

zimeg · 2025-04-10T22:52:35Z

cmd/app/link_test.go

-
 			// Create mocks
+			ctx := slackcontext.MockContext(t.Context())
 			clientsMock := shared.NewClientsMock()


Using the testing context is a neat update also! I haven't explore enough of that package, but I imagine more useful methods might be used elsewhere too.

zimeg · 2025-04-10T22:53:56Z

cmd/doctor/check_test.go

 		clientsMock := shared.NewClientsMock()
 		clientsMock.AddDefaultMocks()
 		clients := shared.NewClientFactory(clientsMock.MockClientFactory())
-		ctx := context.Background()


👏 Creating the ctx before other mocks is a nice change.

zimeg · 2025-04-10T22:55:19Z

cmd/function/access_test.go


 	for name, tt := range tests {
 		t.Run(name, func(t *testing.T) {
+			ctx := slackcontext.MockContext(t.Context())


Edit: I should've read on! I agree so much that this is a good change.

zimeg · 2025-04-10T22:56:33Z

cmd/triggers/create_test.go

-		prepareMocks func(*app.AppMock, *shared.ClientsMock)
+		prepareMocks func(*testing.T, context.Context, *shared.ClientsMock, *app.AppMock)


🤖 Love to see these patterns!

zimeg · 2025-04-10T23:03:06Z

internal/pkg/manifest/validate_test.go

-func setupCommonMocks() (ctx context.Context, clients *shared.ClientFactory, clientsMock *shared.ClientsMock, log *logger.Logger, mockApp types.App, mockAuth types.SlackAuth) {
+func setupCommonMocks(t *testing.T) (ctx context.Context, clients *shared.ClientFactory, clientsMock *shared.ClientsMock, log *logger.Logger, mockApp types.App, mockAuth types.SlackAuth) {


📚 No blocker but sometimes arguments and returns of this amount on a single line are no fun to parse as the reader.

No need to change it and this is opinion too! Also I've tried using linters to tackle this with linters that max a line length but handling cases where this is needed is not always convenient TBH.

Yea, that's a fair point @zimeg! If we can use our linter to enforce a max line length before declaring the function as multi-line, then I think that's best!

zimeg · 2025-04-10T23:06:20Z

internal/pkg/manifest/validate_test.go

 	"github.com/slackapi/slack-cli/internal/logger"
 	"github.com/slackapi/slack-cli/internal/shared"
 	"github.com/slackapi/slack-cli/internal/shared/types"
+	"github.com/slackapi/slack-cli/internal/slackcontext"


🔍 Other import updates also remove context and I was wondering if we're using that package in other ways here?

Edit: Ahhhh of course! It's in the setup of common mocks 👁️‍🗨️

zimeg · 2025-04-10T23:09:32Z

internal/prompts/app_select_test.go

 //

 func TestPrompt_AppSelectPrompt_SelectedAuthExpired_UserReAuthenticates(t *testing.T) {
-


➕ I find these make jumping around sections sometimes a hassle- Thanks for reducing our linecounts with this change!

…edasserts-background

mwbrooks added 3 commits April 10, 2025 14:19

refactor: Add slackcontext package to DRY context management

51b3d28

refactor: Revert changes to unrelated error codes

f5ae262

test: use .MockContext(...) instead of context.Background()

9987754

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 10, 2025

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

mwbrooks self-assigned this Apr 10, 2025

mwbrooks commented Apr 10, 2025

View reviewed changes

mwbrooks marked this pull request as ready for review April 10, 2025 21:52

mwbrooks requested a review from a team as a code owner April 10, 2025 21:52

zimeg approved these changes Apr 10, 2025

View reviewed changes

mwbrooks added 2 commits April 11, 2025 09:31

test: remove duplicate test for collaborator printSuccess

f1d5fa8

Merge branch 'main' into mbrooks-slackcontext-tabletestcommand-expect…

82f4cd6

…edasserts-background

mwbrooks merged commit 4e527aa into main Apr 11, 2025
6 checks passed

mwbrooks deleted the mbrooks-slackcontext-tabletestcommand-expectedasserts-background branch April 11, 2025 16:47

		//

		func TestPrompt_AppSelectPrompt_SelectedAuthExpired_UserReAuthenticates(t *testing.T) {

		prepareMocks func(app.AppMock, shared.ClientsMock)
		prepareMocks func(testing.T, context.Context, shared.ClientsMock, *app.AppMock)

		func setupCommonMocks() (ctx context.Context, clients shared.ClientFactory, clientsMock shared.ClientsMock, log *logger.Logger, mockApp types.App, mockAuth types.SlackAuth) {
		func setupCommonMocks(t testing.T) (ctx context.Context, clients shared.ClientFactory, clientsMock shared.ClientsMock, log logger.Logger, mockApp types.App, mockAuth types.SlackAuth) {

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

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

Uh oh!

Conversation

mwbrooks commented Apr 10, 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 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mwbrooks commented Apr 10, 2025 •

edited

Loading

codecov bot commented Apr 10, 2025 •

edited

Loading