test: mock context in TableTestCommand.ExpectedAsserts #24

mwbrooks · 2025-04-09T22:24:47Z

Summary

This pull request updates test/testutil/commandtests.go to pass the mocked context into ExpectedAsserts.

It's a lot of repetitive lines changes with the occasional 💎 gem hidden. Don't worry though, I'll leave GitHub comments to mark the non-repetitive areas! 🚩

The motivation is to use the mocked context throughout the table test's lifecycle. Right now, some ExpectedAsserts: func(...) implementations require a context and use context.Background(). This works today, but it would fail a function required a value from ctx.

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.

…edasserts

codecov · 2025-04-09T22:27:04Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.91%. Comparing base (bd884b6) to head (d0aa8ef).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #24      +/-   ##
==========================================
- Coverage   62.92%   62.91%   -0.01%     
==========================================
  Files         210      210              
  Lines       22127    22127              
==========================================
- Hits        13924    13922       -2     
- Misses       7115     7117       +2     
  Partials     1088     1088

☔ 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

Notes for the patient reviewers!

mwbrooks · 2025-04-09T22:28:37Z

cmd/app/add_test.go

 				cm.Config.ProjectConfig = mockProjectConfig
 			},
-			ExpectedAsserts: func(t *testing.T, cm *shared.ClientsMock) {
+			ExpectedAsserts: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) {


note: Every reference to ExpectedAsserts: func(...) had to be updated to include ctx. 🔁

Another tedious change - thanks for rolling up the sleeves - but this matches actual command execution so much better 🙏 ✨

mwbrooks · 2025-04-09T22:30:28Z

cmd/app/link_test.go

 				}
 				actualApp, err := cm.AppClient.GetDeployed(
-					context.Background(),
+					ctx,


note: The main motivation of this PR is to replace context.Background() references with the mocked context ctx. Here's one example.

Replacing these instances with the shared mock ctx IMO is a huge win for all of our tests 😭

mwbrooks · 2025-04-09T22:31:22Z

cmd/collaborators/update_test.go

 				// Set experiment flag
 				clientsMock.Config.ExperimentsFlag = append(clientsMock.Config.ExperimentsFlag, "read-only-collaborators")
-				clientsMock.Config.LoadExperiments(context.Background(), clientsMock.IO.PrintDebug)
+				clientsMock.Config.LoadExperiments(ctx, clientsMock.IO.PrintDebug)


note: Here's another example where we're replacing a context.Background() with the new, mocked context ctx

mwbrooks · 2025-04-09T22:34:57Z

cmd/externalauth/add_secret_test.go

 				// TODO this can probably be replaced by a helper that sets up an apps.json file in
 				// the right place on the afero memfs instance
-				err := clients.AppClient().SaveDeployed(context.Background(), fakeApp)
+				err := clients.AppClient().SaveDeployed(ctx, fakeApp)


note: Moar switch-a-roos

mwbrooks · 2025-04-09T22:48:09Z

internal/pkg/platform/activity_test.go

-			ctx := context.Background()
-			ctx = context.WithValue(ctx, config.CONTEXT_TOKEN, "sometoken")
 			// Create mocks
+			ctxMock := slackcontext.MockContext(context.Background())
+			ctxMock = context.WithValue(ctxMock, config.CONTEXT_TOKEN, "sometoken")


note: One of the few unique changes, where the activity test modifies a value in the context.

mwbrooks · 2025-04-09T22:49:31Z

internal/pkg/platform/activity_test.go

 			// Setup custom per-test mocks (higher priority than default mocks)
 			if tt.Setup != nil {
-				ctx = tt.Setup(t, ctx, clientsMock)
+				ctxMock = tt.Setup(t, ctxMock, clientsMock)


thought: I noticed that this activity test appears to have copy & pasted the TableTestCommand implementation, then modified it to work for an internal/pkg/ test. Probably something to revisit in the future because it could be simplified to match our other non-command table tests.

🎁 Super intersting find @mwbrooks! I agree we should revisit these tests at some point since it's also surprising to find context changes here.

Untangling that might mean leaning more into passing app and auth as arguments but I don't recall the underhappenings of this package right now! It's no blocker to the changes otherwise either!

mwbrooks · 2025-04-09T22:51:11Z

test/testutil/commandtests.go

 			// Assert mocks or other custom assertions
 			if tt.ExpectedAsserts != nil {
-				tt.ExpectedAsserts(t, clientsMock)
+				tt.ExpectedAsserts(t, ctxMock, clientsMock)


note: Here's the nuts-and-bolts of this PR. We just pass the mock context into ExpectedAsserts so that the same context can be used everywhere. 🌟

zimeg

@mwbrooks Thank you so much for calling out the gems of this PR! It's all great but this made reviewing much easier for me 📝 ✨

I'm somewhat upset to find a +0.03% improvement to test coverage but that makes sense. I was hoping for more because these changes are so meaningful to better matching commands at runtime.

These are exciting improvements to the health of the code that I'm excited to merge and include in future tests - thanks again for making these changes 🤖

zimeg · 2025-04-10T19:35:28Z

cmd/app/add_test.go

 				cm.Config.ProjectConfig = mockProjectConfig
 			},
-			ExpectedAsserts: func(t *testing.T, cm *shared.ClientsMock) {
+			ExpectedAsserts: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) {


Another tedious change - thanks for rolling up the sleeves - but this matches actual command execution so much better 🙏 ✨

zimeg · 2025-04-10T19:36:49Z

cmd/app/link_test.go

 				}
 				actualApp, err := cm.AppClient.GetDeployed(
-					context.Background(),
+					ctx,


Replacing these instances with the shared mock ctx IMO is a huge win for all of our tests 😭

zimeg · 2025-04-10T19:41:07Z

internal/pkg/platform/activity_test.go

 			// Setup custom per-test mocks (higher priority than default mocks)
 			if tt.Setup != nil {
-				ctx = tt.Setup(t, ctx, clientsMock)
+				ctxMock = tt.Setup(t, ctxMock, clientsMock)


🎁 Super intersting find @mwbrooks! I agree we should revisit these tests at some point since it's also surprising to find context changes here.

Untangling that might mean leaning more into passing app and auth as arguments but I don't recall the underhappenings of this package right now! It's no blocker to the changes otherwise either!

…edasserts

mwbrooks · 2025-04-10T21:07:33Z

I'm somewhat upset to find a +0.03% improvement to test coverage but that makes sense. I was hoping for more because these changes are so meaningful to better matching commands at runtime.

Thanks for the review @zimeg and I'm glad to hear that the notes made reviewing smoother! I agree, it's tedious work with little upside. I think the main benefits are prevent errors down the road, when the values in context matter. Also, doing a full sweep now helps set better copy & paste standards going forward. 🫠

mwbrooks added 9 commits April 2, 2025 12:44

refactor: Add slackcontext package to DRY context management

4add4cc

refactor: Revert changes to unrelated error codes

6c8907a

test: mock context in TableTestCommand

f7b9014

test: mock context in TableTestCommand ExpectedAsserts

b8f68e9

test: use ctx in mocked ExpectedAsserts

9c816be

test: use ctx in mocked ExpectedAsserts to link_test.go

5f69d23

test: use ctx in mocked ExpectedAsserts to update_test.go

c4b7697

test: update context references for .Setup

ad91292

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

8b92a2a

…edasserts

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

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

mwbrooks self-assigned this Apr 9, 2025

mwbrooks commented Apr 9, 2025

View reviewed changes

mwbrooks marked this pull request as ready for review April 9, 2025 22:53

mwbrooks requested a review from a team April 9, 2025 22:56

zimeg approved these changes Apr 10, 2025

View reviewed changes

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

d0aa8ef

…edasserts

mwbrooks merged commit f2a26f0 into main Apr 10, 2025
6 checks passed

mwbrooks deleted the mbrooks-slackcontext-tabletestcommand-expectedasserts branch April 10, 2025 21:15

test: mock context in TableTestCommand.ExpectedAsserts #24

test: mock context in TableTestCommand.ExpectedAsserts #24

Uh oh!

Conversation

mwbrooks commented Apr 9, 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 9, 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!

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!

mwbrooks commented Apr 10, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mwbrooks commented Apr 9, 2025 •

edited

Loading

codecov bot commented Apr 9, 2025 •

edited

Loading