From 8da1323054fee1167a31e061738c6408e42e3341 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20Mu=CC=88ller?= Date: Mon, 6 Jun 2022 18:28:42 -0700 Subject: [PATCH 1/3] add flag to ignore missing calls (off by default) --- main.go | 2 +- pkg/paralleltest/paralleltest.go | 39 ++++++++++++++++----------- pkg/paralleltest/paralleltest_test.go | 2 +- 3 files changed, 26 insertions(+), 17 deletions(-) diff --git a/main.go b/main.go index 90d98e3..b212111 100644 --- a/main.go +++ b/main.go @@ -7,5 +7,5 @@ import ( ) func main() { - singlechecker.Main(paralleltest.NewAnalyzer()) + singlechecker.Main(paralleltest.Analyzer) } diff --git a/pkg/paralleltest/paralleltest.go b/pkg/paralleltest/paralleltest.go index 4783aa2..92e2647 100644 --- a/pkg/paralleltest/paralleltest.go +++ b/pkg/paralleltest/paralleltest.go @@ -15,16 +15,21 @@ It also checks that the t.Parallel is used if multiple tests cases are run as pa As part of ensuring parallel tests works as expected it checks for reinitialising of the range value over the test cases.(https://tinyurl.com/y6555cy6)` -func NewAnalyzer() *analysis.Analyzer { - return &analysis.Analyzer{ - Name: "paralleltest", - Doc: Doc, - Run: run, - Requires: []*analysis.Analyzer{inspect.Analyzer}, - } +var Analyzer = &analysis.Analyzer{ + Name: "paralleltest", + Doc: Doc, + Run: run, + Requires: []*analysis.Analyzer{inspect.Analyzer}, +} + +var ignoreMissing bool + +func init() { + Analyzer.Flags.BoolVar(&ignoreMissing, "i", false, "ignore missing calls to t.Parallel") } func run(pass *analysis.Pass) (interface{}, error) { + inspector := inspector.New(pass.Files) nodeFilter := []ast.Node{ @@ -52,12 +57,12 @@ func run(pass *analysis.Pass) (interface{}, error) { case *ast.ExprStmt: ast.Inspect(v, func(n ast.Node) bool { - // Check if the test method is calling t.parallel + // Check if the test method is calling t.Parallel if !funcHasParallelMethod { funcHasParallelMethod = methodParallelIsCalledInTestFunction(n, testVar) } - // Check if the t.Run within the test function is calling t.parallel + // Check if the t.Run within the test function is calling t.Parallel if methodRunIsCalledInTestFunction(n, testVar) { // n is a call to t.Run; find out the name of the subtest's *testing.T parameter. innerTestVar := getRunCallbackParameterName(n) @@ -77,7 +82,7 @@ func run(pass *analysis.Pass) (interface{}, error) { return true }) - // Check if the range over testcases is calling t.parallel + // Check if the range over testcases is calling t.Parallel case *ast.RangeStmt: rangeNode = v @@ -114,22 +119,26 @@ func run(pass *analysis.Pass) (interface{}, error) { } } - if !funcHasParallelMethod { + if !ignoreMissing && !funcHasParallelMethod { pass.Reportf(node.Pos(), "Function %s missing the call to method parallel\n", funcDecl.Name.Name) } if rangeStatementOverTestCasesExists && rangeNode != nil { if !rangeStatementHasParallelMethod { - pass.Reportf(rangeNode.Pos(), "Range statement for test %s missing the call to method parallel in test Run\n", funcDecl.Name.Name) + if !ignoreMissing { + pass.Reportf(rangeNode.Pos(), "Range statement for test %s missing the call to method parallel in test Run\n", funcDecl.Name.Name) + } } else if loopVariableUsedInRun != nil { pass.Reportf(rangeNode.Pos(), "Range statement for test %s does not reinitialise the variable %s\n", funcDecl.Name.Name, *loopVariableUsedInRun) } } // Check if the t.Run is more than one as there is no point making one test parallel - if numberOfTestRun > 1 && len(positionOfTestRunNode) > 0 { - for _, n := range positionOfTestRunNode { - pass.Reportf(n.Pos(), "Function %s has missing the call to method parallel in the test run\n", funcDecl.Name.Name) + if !ignoreMissing { + if numberOfTestRun > 1 && len(positionOfTestRunNode) > 0 { + for _, n := range positionOfTestRunNode { + pass.Reportf(n.Pos(), "Function %s has missing the call to method parallel in the test run\n", funcDecl.Name.Name) + } } } }) diff --git a/pkg/paralleltest/paralleltest_test.go b/pkg/paralleltest/paralleltest_test.go index cd214ed..f0d6093 100644 --- a/pkg/paralleltest/paralleltest_test.go +++ b/pkg/paralleltest/paralleltest_test.go @@ -16,5 +16,5 @@ func TestAll(t *testing.T) { } testdata := filepath.Join(filepath.Dir(wd), "paralleltest", "testdata") - analysistest.Run(t, testdata, NewAnalyzer(), "t") + analysistest.Run(t, testdata, Analyzer, "t") } From 465b6c2819d1484c9eddb0caba8ff68cef72fa11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20Mu=CC=88ller?= Date: Mon, 6 Jun 2022 19:27:12 -0700 Subject: [PATCH 2/3] refactor ignore flag and add tests --- pkg/paralleltest/paralleltest.go | 14 +- pkg/paralleltest/paralleltest_test.go | 22 ++- pkg/paralleltest/testdata/src/i/i_test.go | 163 ++++++++++++++++++++++ 3 files changed, 195 insertions(+), 4 deletions(-) create mode 100644 pkg/paralleltest/testdata/src/i/i_test.go diff --git a/pkg/paralleltest/paralleltest.go b/pkg/paralleltest/paralleltest.go index 92e2647..c7da52a 100644 --- a/pkg/paralleltest/paralleltest.go +++ b/pkg/paralleltest/paralleltest.go @@ -1,6 +1,7 @@ package paralleltest import ( + "flag" "go/ast" "go/types" "strings" @@ -19,17 +20,24 @@ var Analyzer = &analysis.Analyzer{ Name: "paralleltest", Doc: Doc, Run: run, + Flags: flags(), Requires: []*analysis.Analyzer{inspect.Analyzer}, } -var ignoreMissing bool +const ignoreMissingFlag = "i" -func init() { - Analyzer.Flags.BoolVar(&ignoreMissing, "i", false, "ignore missing calls to t.Parallel") +func flags() flag.FlagSet { + options := flag.NewFlagSet("", flag.ExitOnError) + options.Bool(ignoreMissingFlag, false, "ignore missing calls to t.Parallel") + return *options } +type boolValue bool + func run(pass *analysis.Pass) (interface{}, error) { + ignoreMissing := pass.Analyzer.Flags.Lookup(ignoreMissingFlag).Value.(flag.Getter).Get().(bool) + inspector := inspector.New(pass.Files) nodeFilter := []ast.Node{ diff --git a/pkg/paralleltest/paralleltest_test.go b/pkg/paralleltest/paralleltest_test.go index f0d6093..eafd03d 100644 --- a/pkg/paralleltest/paralleltest_test.go +++ b/pkg/paralleltest/paralleltest_test.go @@ -1,6 +1,7 @@ package paralleltest import ( + "flag" "os" "path/filepath" "testing" @@ -8,8 +9,9 @@ import ( "golang.org/x/tools/go/analysis/analysistest" ) -func TestAll(t *testing.T) { +func TestMissing(t *testing.T) { t.Parallel() + wd, err := os.Getwd() if err != nil { t.Fatalf("Failed to get wd: %s", err) @@ -18,3 +20,21 @@ func TestAll(t *testing.T) { testdata := filepath.Join(filepath.Dir(wd), "paralleltest", "testdata") analysistest.Run(t, testdata, Analyzer, "t") } + +func TestIgnoreMissing(t *testing.T) { + t.Parallel() + + wd, err := os.Getwd() + if err != nil { + t.Fatalf("Failed to get wd: %s", err) + } + + options := flag.NewFlagSet("", flag.ExitOnError) + options.Bool("i", true, "") + + analyzer := *Analyzer + analyzer.Flags = *options + + testdata := filepath.Join(filepath.Dir(wd), "paralleltest", "testdata") + analysistest.Run(t, testdata, &analyzer, "i") +} diff --git a/pkg/paralleltest/testdata/src/i/i_test.go b/pkg/paralleltest/testdata/src/i/i_test.go new file mode 100644 index 0000000..f476be3 --- /dev/null +++ b/pkg/paralleltest/testdata/src/i/i_test.go @@ -0,0 +1,163 @@ +package t + +import ( + "fmt" + "testing" +) + +func NoATestFunction() {} +func TestingFunctionLooksLikeATestButIsNotWithParam() {} +func TestingFunctionLooksLikeATestButIsWithParam(i int) {} +func AbcFunctionSuccessful(t *testing.T) {} + +func TestFunctionSuccessfulRangeTest(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + }{{name: "foo"}} + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(x *testing.T) { + x.Parallel() + fmt.Println(tc.name) + }) + } +} + +func TestFunctionSuccessfulNoRangeTest(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + }{{name: "foo"}, {name: "bar"}} + + t.Run(testCases[0].name, func(t *testing.T) { + t.Parallel() + fmt.Println(testCases[0].name) + }) + t.Run(testCases[1].name, func(t *testing.T) { + t.Parallel() + fmt.Println(testCases[1].name) + }) + +} + +func TestFunctionMissingCallToParallel(t *testing.T) {} +func TestFunctionRangeMissingCallToParallel(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + }{{name: "foo"}} + + // this range loop should be okay as it does not have test Run + for _, tc := range testCases { + fmt.Println(tc.name) + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + fmt.Println(tc.name) + }) + } +} + +func TestFunctionMissingCallToParallelAndRangeNotUsingRangeValueInTDotRun(t *testing.T) { + testCases := []struct { + name string + }{{name: "foo"}} + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + fmt.Println(tc.name) + }) + } +} + +func TestFunctionRangeNotUsingRangeValueInTDotRun(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + }{{name: "foo"}} + for _, tc := range testCases { // want "Range statement for test TestFunctionRangeNotUsingRangeValueInTDotRun does not reinitialise the variable tc" + t.Run("tc.name", func(t *testing.T) { + t.Parallel() + fmt.Println(tc.name) + }) + } +} + +func TestFunctionRangeNotReInitialisingVariable(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + }{{name: "foo"}} + for _, tc := range testCases { // want "Range statement for test TestFunctionRangeNotReInitialisingVariable does not reinitialise the variable tc" + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + fmt.Println(tc.name) + }) + } +} + +func TestFunctionTwoTestRunMissingCallToParallel(t *testing.T) { + t.Parallel() + + t.Run("1", func(t *testing.T) { + fmt.Println("1") + }) + t.Run("2", func(t *testing.T) { + fmt.Println("2") + }) +} + +func TestFunctionFirstOneTestRunMissingCallToParallel(t *testing.T) { + t.Parallel() + + t.Run("1", func(t *testing.T) { + fmt.Println("1") + }) + t.Run("2", func(t *testing.T) { + t.Parallel() + fmt.Println("2") + }) +} + +func TestFunctionSecondOneTestRunMissingCallToParallel(t *testing.T) { + t.Parallel() + + t.Run("1", func(x *testing.T) { + x.Parallel() + fmt.Println("1") + }) + t.Run("2", func(t *testing.T) { + fmt.Println("2") + }) +} + +type notATest int + +func (notATest) Run(args ...interface{}) {} +func (notATest) Parallel() {} + +func TestFunctionWithRunLookalike(t *testing.T) { + t.Parallel() + + var other notATest + // These aren't t.Run, so they shouldn't be flagged as Run invocations missing calls to Parallel. + other.Run(1, 1) + other.Run(2, 2) +} + +func TestFunctionWithParallelLookalike(t *testing.T) { + var other notATest + // This isn't t.Parallel, so it doesn't qualify as a call to Parallel. + other.Parallel() +} + +func TestFunctionWithOtherTestingVar(q *testing.T) { + q.Parallel() +} From c59622a7ff8ff5715931b9c51f4da2d818b2e9c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20Mu=CC=88ller?= Date: Mon, 6 Jun 2022 19:27:20 -0700 Subject: [PATCH 3/3] document ignore flag --- README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/README.md b/README.md index 16c7260..e7d7773 100644 --- a/README.md +++ b/README.md @@ -13,6 +13,8 @@ The Go linter `paralleltest` checks that the t.Parallel gets called for the test paralleltest ./... ``` +To ignore missing calls to `t.Parallel` and only report incorrect uses of it, pass the flag `-i`. + ## Examples ### Missing t.Parallel() in the test method