test: mock context in TableTestCommand #20

mwbrooks · 2025-04-07T22:25:44Z

Summary

Depends on PR #7 and ready for review when PR #7 is merged.

This pull request updates testutil.TableTestCommand to use a mocked context and executes the command with the provided context.

Currently, the testutil.TableTestCommand doesn't define a context. This has 2 drawbacks:

Each test uses it's own context.Background() and must manually mock the context to include required value that are initialized when a CLI command is run (e.g. SessionID).
Mocked context values - typically done in .Setup(...) - are not available to the Cobra command or any cmd.Context() calls.

By having testutil.TableTestCommand mock the context, pass it to .Setup, and then execute the command with the same context, we match how the CLI initializes and executes a command in main.go and cmd/root.go.

A small improvement may be for the .Setup to return a ctx. However, this is only important when the context is modified and we don't do that right now.

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.

mwbrooks

I hope the 200 lines don't scare you off. It should be easy scrolling and I've left a few comments to guide you through the repetition to the important parts!

mwbrooks · 2025-04-07T22:27:12Z

test/testutil/commandtests.go

-	ExpectedAsserts       func(*testing.T, *shared.ClientsMock)                        // Optional
-	ExpectedError         error                                                        // Optional
-	ExpectedErrorStrings  []string                                                     // Optional
+	Setup                 func(*testing.T, context.Context, *shared.ClientsMock, *shared.ClientFactory) // Optional


note: 💠 This is the main change of the pull request and the arguments below are only showing as changed because the comment indentation was adjusted by the formatter.

mwbrooks · 2025-04-07T22:28:42Z

test/testutil/commandtests.go

 	for name, tt := range commandTests {
 		t.Run(name, func(t *testing.T) {
 			// Create mocks
+			ctxMock := slackcontext.MockContext(t.Context())


note: Here we mock the context with the standard values that a new CLI process will initialize.

🎉 It's so neat that this can be used for both setup and execution. One context for all!

Yea, it's nice. I wonder how we can document that values set in MockContext are the values "guaranteed" to exist when a Cobra command executes (cmd.ExecuteContext(ctx))? Perhaps we can document this in the header of slackcontext.go? For example, Golang's context.go has a large comment block above the package context to explain the usage of it.

Wow I'm fascinated with the godoc standards of this upstream. I know go claims to not have ideal standards here, but we should feel generous with this documentation! 📚 ✨

mwbrooks · 2025-04-07T22:29:13Z

test/testutil/commandtests.go

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


note: Here we pass the mocked context into .Setup and in the future we could return the ctx in case it's been modified by .Setup

mwbrooks · 2025-04-07T22:30:15Z

test/testutil/commandtests.go

 			// Execute the command
 			cmd.SetArgs(tt.CmdArgs)
-			err := cmd.Execute()
+			err := cmd.ExecuteContext(ctxMock)


note: ⚡ We now execute the command with the context, so that cmd.Context() will return this context. This matches how root.go executes the CLI with cmd.ExecuteContext(ctx).

One small change to arguments and one giant change for testing commands 🚀 ✨

mwbrooks · 2025-04-07T22:31:19Z

cmd/app/add_test.go

 		"errors if not run in a project directory": {
 			ExpectedError: slackerror.New(slackerror.ErrInvalidAppDirectory),
-			Setup: func(t *testing.T, cm *shared.ClientsMock, cf *shared.ClientFactory) {
+			Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) {


note: This change is made about 200 times 😆 Scroll down to the very bottom to see the updates to testutil.TableTestCommand that implement this feature. ⬇️

Praises @mwbrooks! This is a tedious but meaningful change 🎁

zimeg

🧪 What a wonderful and focused PR this is for testing improvements.

I'm excited for this as more commands begin to follow this table testing pattern and the code being tested uses a single context. These are all great changes being made 🚢 💨

zimeg · 2025-04-09T16:56:48Z

cmd/app/add_test.go

 		"errors if not run in a project directory": {
 			ExpectedError: slackerror.New(slackerror.ErrInvalidAppDirectory),
-			Setup: func(t *testing.T, cm *shared.ClientsMock, cf *shared.ClientFactory) {
+			Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) {


Praises @mwbrooks! This is a tedious but meaningful change 🎁

zimeg · 2025-04-09T16:59:23Z

test/testutil/commandtests.go

 			// Execute the command
 			cmd.SetArgs(tt.CmdArgs)
-			err := cmd.Execute()
+			err := cmd.ExecuteContext(ctxMock)


One small change to arguments and one giant change for testing commands 🚀 ✨

zimeg · 2025-04-09T17:04:02Z

test/testutil/commandtests.go

 	for name, tt := range commandTests {
 		t.Run(name, func(t *testing.T) {
 			// Create mocks
+			ctxMock := slackcontext.MockContext(t.Context())


🎉 It's so neat that this can be used for both setup and execution. One context for all!

mwbrooks · 2025-04-09T17:11:41Z

Thanks for taking the time to review this PR @zimeg! 🙇🏻 I agree, it'll be nice to get our tests to be more consistent. I didn't expect this work to move that forward, but it is! 🧪

mwbrooks added 3 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

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

Base automatically changed from mbrooks-slackcontext to main April 8, 2025 23:31

Merge branch 'main' into mbrooks-slackcontext-tabletestcommand

abc6f4c

mwbrooks marked this pull request as ready for review April 8, 2025 23:39

mwbrooks requested a review from a team as a code owner April 8, 2025 23:39

zimeg approved these changes Apr 9, 2025

View reviewed changes

mwbrooks merged commit 4098541 into main Apr 9, 2025
6 checks passed

mwbrooks deleted the mbrooks-slackcontext-tabletestcommand branch April 9, 2025 17:11

zimeg mentioned this pull request Apr 10, 2025

ci: attempt deletion of branch prereleases from prior versions on merge #30

Merged

2 tasks

test: mock context in TableTestCommand #20

test: mock context in TableTestCommand #20

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!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mwbrooks commented Apr 7, 2025 •

edited

Loading