From 86740f145707c7a418011c75e9480085f9c5e99b Mon Sep 17 00:00:00 2001 From: ashmrtn Date: Wed, 19 Oct 2022 10:30:28 -0700 Subject: [PATCH] Lint rule for making new contexts in tests (#1199) ## Description Create a lint rule that warns if `context.TODO` or `context.Background` are called in tests. This helps standardize the use of `tester.NewContext` and ensures log messages are properly flushed during tests. Output from the linter is similar to (sadly can't get rid of the `\\` in there) ``` pkg/services/m365/m365_test.go:49:22: use of `context.Background` forbidden because "tests should use tester\\.NewContext" (forbidigo) ids, err := UserIDs(context.Background(), acct) ``` ## Type of change - [ ] :sunflower: Feature - [ ] :bug: Bugfix - [ ] :world_map: Documentation - [ ] :robot: Test - [x] :computer: CI/Deployment - [ ] :hamster: Trivial/Minor ## Issue(s) * closes #1198 ## Test Plan - [x] :muscle: Manual - [ ] :zap: Unit test - [ ] :green_heart: E2E --- src/.golangci.yml | 28 +++++++++++++++---- src/cli/print/print_test.go | 1 + src/cli/restore/exchange_integration_test.go | 1 + src/internal/connector/onedrive/drive_test.go | 5 ++-- src/internal/kopia/conn_test.go | 4 ++- src/internal/kopia/wrapper_test.go | 1 + src/internal/observe/observe_test.go | 1 + src/pkg/services/m365/m365_test.go | 11 ++++++-- 8 files changed, 40 insertions(+), 12 deletions(-) diff --git a/src/.golangci.yml b/src/.golangci.yml index 3b57a592d..25b767257 100644 --- a/src/.golangci.yml +++ b/src/.golangci.yml @@ -7,6 +7,7 @@ linters: - gofmt - gofumpt - errcheck + - forbidigo - lll - misspell - revive @@ -19,6 +20,11 @@ linters-settings: - default - prefix(github.com/alcionai/corso) skip-generated: true + forbidigo: + forbid: + # Don't allow creating contexts without logging in tests. Use an ignore + # lower down to ensure usages of this outside of tests aren't reported. + - 'context\.(Background|TODO)(# tests should use tester\.NewContext )?' lll: line-length: 120 revive: @@ -62,21 +68,31 @@ issues: max-same-issues: 50 exclude-rules: - linters: - - revive + - revive text: "exported:.*details.DetailsModel by other packages, and that stutters" - linters: - - revive + - revive text: "exported:.*details.DetailsEntry by other packages, and that stutters" - linters: - - revive + - revive text: "exported:.*mock.MockModelStore by other packages, and that stutters" - linters: - - revive + - revive text: "unexported-return:.*unexported type selectors.exchangeCategory" - linters: - - revive + - revive text: "unexported-return:.*unexported type.*kopia.conn" - path: _test\.go linters: - - revive + - revive text: "import-shadowing:.*'suite' shadows" + # Don't output about context.Background or context.TODO in non-test files. + # Need complicated multi-part regex because golang doesn't support not regex + # operators for sequences of characters (only character sets). The below + # basically boils down to: + # 1. anything 4 characters long with .go suffix + # 2. anything >= 5 characters long that doesn't end in _test.go + - path: ^.{4}\.go|.*([^_].{4}|_[^t].{3}|_t[^e].{2}|_te[^s].{1}|_tes[^t])\.go + linters: + - forbidigo + text: "context.(Background|TODO)" diff --git a/src/cli/print/print_test.go b/src/cli/print/print_test.go index 0b7370c07..4386e73ab 100644 --- a/src/cli/print/print_test.go +++ b/src/cli/print/print_test.go @@ -22,6 +22,7 @@ func (suite *PrintUnitSuite) TestOnly() { t := suite.T() c := &cobra.Command{} // cannot use tester.NewContext() here: circular imports + //nolint:forbidigo ctx := SetRootCmd(context.Background(), c) assert.NoError(t, Only(ctx, nil)) assert.True(t, c.SilenceUsage) diff --git a/src/cli/restore/exchange_integration_test.go b/src/cli/restore/exchange_integration_test.go index 2c80d84e5..8065b73c2 100644 --- a/src/cli/restore/exchange_integration_test.go +++ b/src/cli/restore/exchange_integration_test.go @@ -181,6 +181,7 @@ func (suite *RestoreExchangeIntegrationSuite) TestExchangeRestoreCmd_badBoolFlag } suite.T().Run(set.String(), func(t *testing.T) { + //nolint:forbidigo ctx := config.SetViper(context.Background(), suite.vpr) ctx, flush := tester.WithContext(ctx) defer flush() diff --git a/src/internal/connector/onedrive/drive_test.go b/src/internal/connector/onedrive/drive_test.go index 8302e9873..ce76863c4 100644 --- a/src/internal/connector/onedrive/drive_test.go +++ b/src/internal/connector/onedrive/drive_test.go @@ -1,7 +1,6 @@ package onedrive import ( - "context" "strings" "testing" @@ -35,8 +34,10 @@ func (suite *OneDriveSuite) SetupSuite() { } func (suite *OneDriveSuite) TestCreateGetDeleteFolder() { + ctx, flush := tester.NewContext() + defer flush() + t := suite.T() - ctx := context.Background() folderIDs := []string{} folderName1 := "Corso_Folder_Test_" + common.FormatNow(common.SimpleTimeTesting) folderElements := []string{folderName1} diff --git a/src/internal/kopia/conn_test.go b/src/internal/kopia/conn_test.go index f6ee1c2a3..4c8f48fbf 100644 --- a/src/internal/kopia/conn_test.go +++ b/src/internal/kopia/conn_test.go @@ -151,7 +151,9 @@ func (suite *WrapperIntegrationSuite) TestCloseAfterWrap() { } func (suite *WrapperIntegrationSuite) TestOpenAfterClose() { - ctx := context.Background() + ctx, flush := tester.NewContext() + defer flush() + t := suite.T() k, err := openKopiaRepo(t, ctx) diff --git a/src/internal/kopia/wrapper_test.go b/src/internal/kopia/wrapper_test.go index 7932d3ed5..ff3f79806 100644 --- a/src/internal/kopia/wrapper_test.go +++ b/src/internal/kopia/wrapper_test.go @@ -1065,6 +1065,7 @@ func (suite *KopiaSimpleRepoIntegrationSuite) SetupTest() { t := suite.T() expectedDirs := 6 expectedFiles := len(suite.filesByPath) + //nolint:forbidigo suite.ctx, _ = logger.SeedLevel(context.Background(), logger.Development) c, err := openKopiaRepo(t, suite.ctx) require.NoError(t, err) diff --git a/src/internal/observe/observe_test.go b/src/internal/observe/observe_test.go index 1d9881fa7..84183f9cd 100644 --- a/src/internal/observe/observe_test.go +++ b/src/internal/observe/observe_test.go @@ -36,6 +36,7 @@ func (suite *ObserveProgressUnitSuite) TestDoesThings() { defer func() { // don't cross-contaminate other tests. observe.Complete() + //nolint:forbidigo observe.SeedWriter(context.Background(), nil) }() diff --git a/src/pkg/services/m365/m365_test.go b/src/pkg/services/m365/m365_test.go index bf8d2a881..01bc50c9b 100644 --- a/src/pkg/services/m365/m365_test.go +++ b/src/pkg/services/m365/m365_test.go @@ -1,7 +1,6 @@ package m365 import ( - "context" "testing" "github.com/stretchr/testify/require" @@ -44,9 +43,12 @@ func (suite *M365IntegrationSuite) TestUsers() { } func (suite *M365IntegrationSuite) TestUserIDs() { + ctx, flush := tester.NewContext() + defer flush() + acct := tester.NewM365Account(suite.T()) - ids, err := UserIDs(context.Background(), acct) + ids, err := UserIDs(ctx, acct) require.NoError(suite.T(), err) require.NotNil(suite.T(), ids) @@ -54,9 +56,12 @@ func (suite *M365IntegrationSuite) TestUserIDs() { } func (suite *M365IntegrationSuite) TestGetEmailAndUserID() { + ctx, flush := tester.NewContext() + defer flush() + acct := tester.NewM365Account(suite.T()) - ids, err := GetEmailAndUserID(context.Background(), acct) + ids, err := GetEmailAndUserID(ctx, acct) require.NoError(suite.T(), err) require.NotNil(suite.T(), ids)