Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mwbrooks merged 5 commits into from
Apr 11, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
51b3d28
refactor: Add slackcontext package to DRY context management
mwbrooks Apr 2, 2025
f5ae262
refactor: Revert changes to unrelated error codes
mwbrooks Apr 2, 2025
9987754
test: use .MockContext(...) instead of context.Background()
mwbrooks Apr 8, 2025
f1d5fa8
test: remove duplicate test for collaborator printSuccess
mwbrooks Apr 11, 2025
82f4cd6
Merge branch 'main' into mbrooks-slackcontext-tabletestcommand-expect…
mwbrooks Apr 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions cmd/app/link_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"github.com/slackapi/slack-cli/internal/iostreams"
"github.com/slackapi/slack-cli/internal/shared"
"github.com/slackapi/slack-cli/internal/shared/types"
"github.com/slackapi/slack-cli/internal/slackcontext"
"github.com/slackapi/slack-cli/internal/slackdeps"
"github.com/slackapi/slack-cli/internal/slackerror"
"github.com/slackapi/slack-cli/test/testutil"
Expand Down Expand Up @@ -569,10 +570,8 @@ func Test_Apps_LinkAppHeaderSection(t *testing.T) {
}
for name, tt := range tests {
t.Run(name, func(t *testing.T) {
// Setup parameters for test
ctx := context.Background()

// Create mocks
ctx := slackcontext.MockContext(t.Context())
clientsMock := shared.NewClientsMock()
Copy link
Member Author

@mwbrooks mwbrooks Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@zimeg zimeg Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting up ctx with other mocks is an awesome change!

Copy link
Member

@zimeg zimeg Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@mwbrooks mwbrooks Apr 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. It makes it easy to search for context.Background() and context.TODO() to prevent those from slipping into production code.
  2. 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.

clientsMock.AddDefaultMocks()

Expand Down
12 changes: 6 additions & 6 deletions cmd/app/uninstall_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func TestAppsUninstall(t *testing.T) {
testutil.TableTestCommand(t, testutil.CommandTests{
"Successfully uninstall": {
Setup: func(t *testing.T, ctx context.Context, clientsMock *shared.ClientsMock, clients *shared.ClientFactory) {
prepareCommonUninstallMocks(clients, clientsMock)
prepareCommonUninstallMocks(ctx, clients, clientsMock)
Copy link
Member Author

@mwbrooks mwbrooks Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

clientsMock.ApiInterface.On("UninstallApp", mock.Anything, mock.Anything, fakeAppID, fakeAppTeamID).
Return(nil).Once()
},
Expand All @@ -58,7 +58,7 @@ func TestAppsUninstall(t *testing.T) {
},
"Successfully uninstall with a get-manifest hook error": {
Setup: func(t *testing.T, ctx context.Context, clientsMock *shared.ClientsMock, clients *shared.ClientFactory) {
prepareCommonUninstallMocks(clients, clientsMock)
prepareCommonUninstallMocks(ctx, clients, clientsMock)
clientsMock.ApiInterface.On("UninstallApp", mock.Anything, mock.Anything, fakeAppID, fakeAppTeamID).
Return(nil).Once()
manifestMock := &app.ManifestMockObject{}
Expand All @@ -73,7 +73,7 @@ func TestAppsUninstall(t *testing.T) {
"Fail to uninstall due to API error": {
ExpectedError: slackerror.New("something went wrong"),
Setup: func(t *testing.T, ctx context.Context, clientsMock *shared.ClientsMock, clients *shared.ClientFactory) {
prepareCommonUninstallMocks(clients, clientsMock)
prepareCommonUninstallMocks(ctx, clients, clientsMock)
clientsMock.ApiInterface.On("UninstallApp", mock.Anything, mock.Anything, fakeAppID, fakeAppTeamID).
Return(slackerror.New("something went wrong")).Once()
},
Expand All @@ -82,7 +82,7 @@ func TestAppsUninstall(t *testing.T) {
CmdArgs: []string{},
ExpectedError: slackerror.New(slackerror.ErrCredentialsNotFound),
Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) {
prepareCommonUninstallMocks(cf, cm)
prepareCommonUninstallMocks(ctx, cf, cm)
appSelectMock := prompts.NewAppSelectMock()
uninstallAppSelectPromptFunc = appSelectMock.AppSelectPrompt
appSelectMock.On("AppSelectPrompt").Return(prompts.SelectedApp{App: fakeApp}, nil)
Expand All @@ -95,7 +95,7 @@ func TestAppsUninstall(t *testing.T) {
})
}

func prepareCommonUninstallMocks(clients *shared.ClientFactory, clientsMock *shared.ClientsMock) *shared.ClientFactory {
func prepareCommonUninstallMocks(ctx context.Context, clients *shared.ClientFactory, clientsMock *shared.ClientsMock) *shared.ClientFactory {

// Mock App Selection
appSelectMock := prompts.NewAppSelectMock()
Expand Down Expand Up @@ -124,7 +124,7 @@ func prepareCommonUninstallMocks(clients *shared.ClientFactory, clientsMock *sha

clients.AppClient().AppClientInterface = appClientMock

err := clients.AppClient().SaveDeployed(context.Background(), fakeApp)
err := clients.AppClient().SaveDeployed(ctx, fakeApp)
if err != nil {
panic("error setting up test; cant write apps.json")
}
Expand Down
6 changes: 4 additions & 2 deletions cmd/auth/token_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,20 @@
package auth

import (
"context"
"testing"

"github.com/slackapi/slack-cli/internal/api"
"github.com/slackapi/slack-cli/internal/shared"
"github.com/slackapi/slack-cli/internal/shared/types"
"github.com/slackapi/slack-cli/internal/slackcontext"
"github.com/slackapi/slack-cli/test/testutil"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
)

func TestTokenCommand(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())

clientsMock := shared.NewClientsMock()
clientsMock.ApiInterface.On("ValidateSession", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(api.AuthSession{UserID: &mockOrgAuth.UserID,
TeamID: &mockOrgAuth.TeamID,
Expand All @@ -42,7 +44,7 @@ func TestTokenCommand(t *testing.T) {
tokenFlag = "xoxp-example-1234"

cmd := NewTokenCommand(clients)
cmd.SetContext(context.Background())
cmd.SetContext(ctx)
testutil.MockCmdIO(clients.IO, cmd)

serviceTokenFlag = true
Expand Down
18 changes: 9 additions & 9 deletions cmd/doctor/check_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
package doctor

import (
"context"
"fmt"
"runtime"
"testing"
Expand All @@ -26,6 +25,7 @@ import (
"github.com/slackapi/slack-cli/internal/pkg/version"
"github.com/slackapi/slack-cli/internal/shared"
"github.com/slackapi/slack-cli/internal/shared/types"
"github.com/slackapi/slack-cli/internal/slackcontext"
"github.com/slackapi/slack-cli/internal/slackerror"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
Expand All @@ -34,10 +34,10 @@ import (

func TestDoctorCheckOS(t *testing.T) {
t.Run("returns the operating system version", func(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())
clientsMock := shared.NewClientsMock()
clientsMock.AddDefaultMocks()
clients := shared.NewClientFactory(clientsMock.MockClientFactory())
ctx := context.Background()
Copy link
Member

@zimeg zimeg Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

expected := Section{
Label: "Operating System",
Value: "the computer conductor",
Expand All @@ -56,10 +56,10 @@ func TestDoctorCheckOS(t *testing.T) {

func TestDoctorCheckCLIVersion(t *testing.T) {
t.Run("returns the current version of this tool", func(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())
clientsMock := shared.NewClientsMock()
clientsMock.AddDefaultMocks()
clients := shared.NewClientFactory(clientsMock.MockClientFactory())
ctx := context.Background()
expected := Section{
Label: "CLI",
Value: "this tool for building Slack apps",
Expand Down Expand Up @@ -141,13 +141,13 @@ func TestDoctorCheckProjectConfig(t *testing.T) {

for name, tt := range tests {
t.Run(name, func(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())
clientsMock := shared.NewClientsMock()
clientsMock.AddDefaultMocks()
pcm := &config.ProjectConfigMock{}
pcm.On("ReadProjectConfigFile", mock.Anything).Return(tt.projectConfig, nil)
clientsMock.Config.ProjectConfig = pcm
clients := shared.NewClientFactory(clientsMock.MockClientFactory())
ctx := context.Background()
expected := Section{
Label: "Configurations",
Value: "your project's CLI settings",
Expand Down Expand Up @@ -218,10 +218,10 @@ func TestDoctorCheckProjectDeps(t *testing.T) {

for name, tt := range tests {
t.Run(name, func(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())
clientsMock := shared.NewClientsMock()
clientsMock.AddDefaultMocks()
clients := tt.mockHookSetup(clientsMock)
ctx := context.Background()
expected := Section{
Label: "Dependencies",
Value: "requisites for development",
Expand All @@ -246,6 +246,7 @@ func TestDoctorCheckCLIConfig(t *testing.T) {

for name, tt := range tests {
t.Run(name, func(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())
clientsMock := shared.NewClientsMock()
clientsMock.AddDefaultMocks()
scm := &config.SystemConfigMock{}
Expand All @@ -254,7 +255,6 @@ func TestDoctorCheckCLIConfig(t *testing.T) {
}, nil)
clientsMock.Config.SystemConfig = scm
clients := shared.NewClientFactory(clientsMock.MockClientFactory())
ctx := context.Background()
expected := Section{
Label: "Configurations",
Value: "any adjustments to settings",
Expand Down Expand Up @@ -297,10 +297,10 @@ func TestDoctorCheckCLICreds(t *testing.T) {

for name := range tests {
t.Run(name, func(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())
clientsMock := shared.NewClientsMock()
clientsMock.AddDefaultMocks()
clients := shared.NewClientFactory(clientsMock.MockClientFactory())
ctx := context.Background()
expected := Section{
Label: "Credentials",
Value: "your Slack authentication",
Expand Down Expand Up @@ -391,10 +391,10 @@ func TestDoctorCheckProjectTooling(t *testing.T) {

for name, tt := range tests {
t.Run(name, func(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())
clientsMock := shared.NewClientsMock()
clientsMock.AddDefaultMocks()
clients := tt.mockHookSetup(clientsMock)
ctx := context.Background()
expected := Section{
Label: "Runtime",
Value: "foundations for the application",
Expand All @@ -410,7 +410,7 @@ func TestDoctorCheckProjectTooling(t *testing.T) {

func TestDoctorCheckGit(t *testing.T) {
t.Run("returns the version of git", func(t *testing.T) {
ctx := context.Background()
ctx := slackcontext.MockContext(t.Context())
gitVersion, err := deputil.GetGitVersion()
require.NoError(t, err)
expected := Section{
Expand Down
4 changes: 2 additions & 2 deletions cmd/doctor/doctor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ package doctor

import (
"bytes"
"context"
"fmt"
"runtime"
"testing"
Expand All @@ -28,6 +27,7 @@ import (
"github.com/slackapi/slack-cli/internal/pkg/version"
"github.com/slackapi/slack-cli/internal/shared"
"github.com/slackapi/slack-cli/internal/shared/types"
"github.com/slackapi/slack-cli/internal/slackcontext"
"github.com/slackapi/slack-cli/internal/slackerror"
"github.com/slackapi/slack-cli/test/testutil"
"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -401,7 +401,7 @@ func TestDoctorHook(t *testing.T) {

for name, tt := range tests {
t.Run(name, func(t *testing.T) {
ctx := context.Background()
ctx := slackcontext.MockContext(t.Context())
clientsMock := shared.NewClientsMock()
clientsMock.AddDefaultMocks()
clients := tt.mockHookSetup(clientsMock)
Expand Down
12 changes: 6 additions & 6 deletions cmd/env/add_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ func Test_Env_AddCommand(t *testing.T) {
"add a variable using arguments": {
CmdArgs: []string{"ENV_NAME", "ENV_VALUE"},
Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) {
setupEnvAddCommandMocks(cm, cf)
setupEnvAddCommandMocks(ctx, cm, cf)
},
ExpectedAsserts: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) {
cm.ApiInterface.AssertCalled(
Expand All @@ -169,7 +169,7 @@ func Test_Env_AddCommand(t *testing.T) {
"provide a variable name by argument and value by prompt": {
CmdArgs: []string{"ENV_NAME"},
Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) {
setupEnvAddCommandMocks(cm, cf)
setupEnvAddCommandMocks(ctx, cm, cf)
cm.IO.On(
"PasswordPrompt",
mock.Anything,
Expand Down Expand Up @@ -200,7 +200,7 @@ func Test_Env_AddCommand(t *testing.T) {
"provide a variable name by argument and value by flag": {
CmdArgs: []string{"ENV_NAME", "--value", "example_value"},
Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) {
setupEnvAddCommandMocks(cm, cf)
setupEnvAddCommandMocks(ctx, cm, cf)
cm.IO.On(
"PasswordPrompt",
mock.Anything,
Expand Down Expand Up @@ -231,7 +231,7 @@ func Test_Env_AddCommand(t *testing.T) {
"provide both variable name and value by prompt": {
CmdArgs: []string{},
Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) {
setupEnvAddCommandMocks(cm, cf)
setupEnvAddCommandMocks(ctx, cm, cf)
cm.IO.On(
"InputPrompt",
mock.Anything,
Expand Down Expand Up @@ -276,10 +276,10 @@ func Test_Env_AddCommand(t *testing.T) {
}

// setupEnvAddCommandMocks prepares common mocks for these tests
func setupEnvAddCommandMocks(cm *shared.ClientsMock, cf *shared.ClientFactory) {
func setupEnvAddCommandMocks(ctx context.Context, cm *shared.ClientsMock, cf *shared.ClientFactory) {
cf.SDKConfig = hooks.NewSDKConfigMock()
cm.AddDefaultMocks()
_ = cf.AppClient().SaveDeployed(context.Background(), mockApp)
_ = cf.AppClient().SaveDeployed(ctx, mockApp)

appSelectMock := prompts.NewAppSelectMock()
teamAppSelectPromptFunc = appSelectMock.TeamAppSelectPrompt
Expand Down
10 changes: 7 additions & 3 deletions cmd/feedback/feedback_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"github.com/slackapi/slack-cli/internal/config"
"github.com/slackapi/slack-cli/internal/iostreams"
"github.com/slackapi/slack-cli/internal/shared"
"github.com/slackapi/slack-cli/internal/slackcontext"
"github.com/slackapi/slack-cli/internal/slackerror"
"github.com/slackapi/slack-cli/internal/style"
"github.com/stretchr/testify/assert"
Expand All @@ -46,6 +47,7 @@ func TestFeedbackCommand(t *testing.T) {
}

// Prepare mocks
ctx := slackcontext.MockContext(t.Context())
clientsMock := shared.NewClientsMock()
clientsMock.AddDefaultMocks()

Expand All @@ -69,7 +71,7 @@ func TestFeedbackCommand(t *testing.T) {

// Execute test
cmd := NewFeedbackCommand(clients)
err := runFeedbackCommand(context.Background(), clients, cmd)
err := runFeedbackCommand(ctx, clients, cmd)
assert.NoError(t, err)
clientsMock.Browser.AssertCalled(t, "OpenURL", "https://survey.com?project_id=projectID&system_id=systemID&utm_medium=cli&utm_source=cli")
})
Expand Down Expand Up @@ -104,6 +106,7 @@ func TestFeedbackCommand(t *testing.T) {
}

// Prepare mocks
ctx := slackcontext.MockContext(t.Context())
clientsMock := shared.NewClientsMock()
clientsMock.AddDefaultMocks()

Expand Down Expand Up @@ -134,7 +137,7 @@ func TestFeedbackCommand(t *testing.T) {

// Execute test
cmd := NewFeedbackCommand(clients)
err := runFeedbackCommand(context.Background(), clients, cmd)
err := runFeedbackCommand(ctx, clients, cmd)
assert.NoError(t, err)
clientsMock.Browser.AssertCalled(t, "OpenURL", "https://survey.com?project_id=projectID&system_id=systemID&utm_medium=cli&utm_source=cli")
})
Expand Down Expand Up @@ -196,6 +199,7 @@ func TestShowSurveyMessages(t *testing.T) {
}

// Prepare mocks
ctx := slackcontext.MockContext(t.Context())
clientsMock := shared.NewClientsMock()
clientsMock.AddDefaultMocks()

Expand Down Expand Up @@ -239,7 +243,7 @@ func TestShowSurveyMessages(t *testing.T) {
SurveyStore = surveys

// Execute test
err := ShowSurveyMessages(context.Background(), clients)
err := ShowSurveyMessages(ctx, clients)
assert.NoError(t, err)
clientsMock.Browser.AssertCalled(t, "OpenURL", "https://B.com?project_id=projectID&system_id=systemID&utm_medium=cli&utm_source=cli")
clientsMock.Browser.AssertCalled(t, "OpenURL", "https://C.com?project_id=projectID&system_id=systemID&utm_medium=cli&utm_source=cli")
Expand Down
5 changes: 3 additions & 2 deletions cmd/function/access_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (

"github.com/slackapi/slack-cli/internal/shared"
"github.com/slackapi/slack-cli/internal/shared/types"
"github.com/slackapi/slack-cli/internal/slackcontext"
"github.com/slackapi/slack-cli/internal/slackerror"
"github.com/slackapi/slack-cli/test/testutil"
"github.com/spf13/afero"
Expand Down Expand Up @@ -326,6 +327,7 @@ func TestFunctionDistributionCommand_PermissionsFile(t *testing.T) {

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

@mwbrooks mwbrooks Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@zimeg zimeg Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

clientsMock := shared.NewClientsMock()
clientsMock.IO.AddDefaultMocks()
err := afero.WriteFile(clientsMock.Fs, tt.filename, []byte(tt.data), 0644)
Expand All @@ -339,7 +341,6 @@ func TestFunctionDistributionCommand_PermissionsFile(t *testing.T) {
clientsMock.ApiInterface.On("FunctionDistributionSet", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).
Return([]types.FunctionDistributionUser{}, nil)

ctx := context.Background()
clients := shared.NewClientFactory(clientsMock.MockClientFactory())
app := types.App{AppID: "A123"}

Expand Down Expand Up @@ -400,6 +401,7 @@ func TestFunctionDistributeCommand_UpdateNamedEntitiesDistribution(t *testing.T)

for name, tt := range tests {
t.Run(name, func(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())
clientsMock := shared.NewClientsMock()
clientsMock.ApiInterface.On("FunctionDistributionSet", mock.Anything, mock.Anything, mock.Anything, types.NAMED_ENTITIES, mock.Anything).
Return([]types.FunctionDistributionUser{}, nil).
Expand All @@ -414,7 +416,6 @@ func TestFunctionDistributeCommand_UpdateNamedEntitiesDistribution(t *testing.T)

app := types.App{AppID: "A123"}
function := "Ft123"
ctx := context.Background()
clients := shared.NewClientFactory(clientsMock.MockClientFactory())

err := updateNamedEntitiesDistribution(ctx, clients, app, function, tt.updatedEntities)
Expand Down
Loading
Loading