Merge pull request #33 from spencerschrock/bug/setenv-false-positive

dont flag tests which call Setenv directly or whose subtests do
kunwardeep · Dec 15, 2023 · 2ff338a · 2ff338a
2 parents 1adbbc8 + 7bc464a
commit 2ff338a
Show file tree

Hide file tree

Showing 2 changed files with 98 additions and 13 deletions.
diff --git a/pkg/paralleltest/paralleltest.go b/pkg/paralleltest/paralleltest.go
@@ -54,8 +54,10 @@ func (a *parallelAnalyzer) run(pass *analysis.Pass) (interface{}, error) {
 	inspector.Preorder(nodeFilter, func(node ast.Node) {
 		funcDecl := node.(*ast.FuncDecl)
 		var funcHasParallelMethod,
+			funcCantParallelMethod,
 			rangeStatementOverTestCasesExists,
-			rangeStatementHasParallelMethod bool
+			rangeStatementHasParallelMethod,
+			rangeStatementCantParallelMethod bool
 		var loopVariableUsedInRun *string
 		var numberOfTestRun int
 		var positionOfTestRunNode []ast.Node
@@ -77,20 +79,29 @@ func (a *parallelAnalyzer) run(pass *analysis.Pass) (interface{}, error) {
 						funcHasParallelMethod = methodParallelIsCalledInTestFunction(n, testVar)
 					}
 
+					// Check if the test calls t.Setenv, cannot be used in parallel tests or tests with parallel ancestors
+					if !funcCantParallelMethod {
+						funcCantParallelMethod = methodSetenvIsCalledInTestFunction(n, testVar)
+					}
+
 					// 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)
 
 						hasParallel := false
+						cantParallel := false
 						numberOfTestRun++
 						ast.Inspect(v, func(p ast.Node) bool {
 							if !hasParallel {
 								hasParallel = methodParallelIsCalledInTestFunction(p, innerTestVar)
 							}
+							if !cantParallel {
+								cantParallel = methodSetenvIsCalledInTestFunction(p, innerTestVar)
+							}
 							return true
 						})
-						if !hasParallel {
+						if !hasParallel && !cantParallel {
 							positionOfTestRunNode = append(positionOfTestRunNode, n)
 						}
 					}
@@ -122,6 +133,10 @@ func (a *parallelAnalyzer) run(pass *analysis.Pass) (interface{}, error) {
 								rangeStatementHasParallelMethod = methodParallelIsCalledInMethodRun(r.X, innerTestVar)
 							}
 
+							if !rangeStatementCantParallelMethod {
+								rangeStatementCantParallelMethod = methodSetenvIsCalledInMethodRun(r.X, innerTestVar)
+							}
+
 							if loopVariableUsedInRun == nil {
 								if run, ok := r.X.(*ast.CallExpr); ok {
 									loopVariableUsedInRun = loopVarReferencedInRun(run, loopVars, pass.TypesInfo)
@@ -134,12 +149,17 @@ func (a *parallelAnalyzer) run(pass *analysis.Pass) (interface{}, error) {
 			}
 		}
 
-		if !a.ignoreMissing && !funcHasParallelMethod {
+		// Descendents which call Setenv, also prevent tests from calling Parallel
+		if rangeStatementCantParallelMethod {
+			funcCantParallelMethod = true
+		}
+
+		if !a.ignoreMissing && !funcHasParallelMethod && !funcCantParallelMethod {
 			pass.Reportf(node.Pos(), "Function %s missing the call to method parallel\n", funcDecl.Name.Name)
 		}
 
 		if rangeStatementOverTestCasesExists && rangeNode != nil {
-			if !rangeStatementHasParallelMethod {
+			if !rangeStatementHasParallelMethod && !rangeStatementCantParallelMethod {
 				if !a.ignoreMissing && !a.ignoreMissingSubtests {
 					pass.Reportf(rangeNode.Pos(), "Range statement for test %s missing the call to method parallel in test Run\n", funcDecl.Name.Name)
 				}
@@ -162,27 +182,31 @@ func (a *parallelAnalyzer) run(pass *analysis.Pass) (interface{}, error) {
 }
 
 func methodParallelIsCalledInMethodRun(node ast.Node, testVar string) bool {
-	var methodParallelCalled bool
+	return targetMethodIsCalledInMethodRun(node, testVar, "Parallel")
+}
+
+func methodSetenvIsCalledInMethodRun(node ast.Node, testVar string) bool {
+	return targetMethodIsCalledInMethodRun(node, testVar, "Setenv")
+}
+
+func targetMethodIsCalledInMethodRun(node ast.Node, testVar, targetMethod string) bool {
+	var called bool
 	// nolint: gocritic
 	switch callExp := node.(type) {
 	case *ast.CallExpr:
 		for _, arg := range callExp.Args {
-			if !methodParallelCalled {
+			if !called {
 				ast.Inspect(arg, func(n ast.Node) bool {
-					if !methodParallelCalled {
-						methodParallelCalled = methodParallelIsCalledInRunMethod(n, testVar)
+					if !called {
+						called = exprCallHasMethod(n, testVar, targetMethod)
 						return true
 					}
 					return false
 				})
 			}
 		}
 	}
-	return methodParallelCalled
-}
-
-func methodParallelIsCalledInRunMethod(node ast.Node, testVar string) bool {
-	return exprCallHasMethod(node, testVar, "Parallel")
+	return called
 }
 
 func methodParallelIsCalledInTestFunction(node ast.Node, testVar string) bool {
@@ -196,6 +220,11 @@ func methodRunIsCalledInRangeStatement(node ast.Node, testVar string) bool {
 func methodRunIsCalledInTestFunction(node ast.Node, testVar string) bool {
 	return exprCallHasMethod(node, testVar, "Run")
 }
+
+func methodSetenvIsCalledInTestFunction(node ast.Node, testVar string) bool {
+	return exprCallHasMethod(node, testVar, "Setenv")
+}
+
 func exprCallHasMethod(node ast.Node, receiverName, methodName string) bool {
 	// nolint: gocritic
 	switch n := node.(type) {

diff --git a/pkg/paralleltest/testdata/src/t/t_test.go b/pkg/paralleltest/testdata/src/t/t_test.go
@@ -142,6 +142,7 @@ type notATest int
 
 func (notATest) Run(args ...interface{}) {}
 func (notATest) Parallel()               {}
+func (notATest) Setenv(_, _ string)      {}
 
 func TestFunctionWithRunLookalike(t *testing.T) {
 	t.Parallel()
@@ -161,3 +162,58 @@ func TestFunctionWithParallelLookalike(t *testing.T) { // want "Function TestFun
 func TestFunctionWithOtherTestingVar(q *testing.T) {
 	q.Parallel()
 }
+
+func TestFunctionWithSetenv(t *testing.T) {
+	// unable to call t.Parallel with t.Setenv
+	t.Setenv("foo", "bar")
+}
+
+func TestFunctionWithSetenvLookalike(t *testing.T) { // want "Function TestFunctionWithSetenvLookalike missing the call to method parallel"
+	var other notATest
+	other.Setenv("foo", "bar")
+}
+
+func TestFunctionWithSetenvChild(t *testing.T) {
+	// ancestor of setenv cant call t.Parallel
+	t.Run("1", func(t *testing.T) {
+		// unable to call t.Parallel with t.Setenv
+		t.Setenv("foo", "bar")
+		fmt.Println("1")
+	})
+}
+
+func TestFunctionSetenvChildrenCanBeParallel(t *testing.T) {
+	// unable to call t.Parallel with t.Setenv
+	t.Setenv("foo", "bar")
+	t.Run("1", func(t *testing.T) { // want "Function TestFunctionSetenvChildrenCanBeParallel missing the call to method parallel in the test run"
+		fmt.Println("1")
+	})
+	t.Run("2", func(t *testing.T) { // want "Function TestFunctionSetenvChildrenCanBeParallel missing the call to method parallel in the test run"
+		fmt.Println("2")
+	})
+}
+
+func TestFunctionRunWithSetenvSibling(t *testing.T) {
+	// ancestor of setenv cant call t.Parallel
+	t.Run("1", func(t *testing.T) {
+		// unable to call t.Parallel with t.Setenv
+		t.Setenv("foo", "bar")
+		fmt.Println("1")
+	})
+	t.Run("2", func(t *testing.T) { // want "Function TestFunctionRunWithSetenvSibling missing the call to method parallel in the test run"
+		fmt.Println("2")
+	})
+}
+
+func TestFunctionWithSetenvRange(t *testing.T) {
+	// ancestor of setenv cant call t.Parallel
+	testCases := []struct {
+		name string
+	}{{name: "foo"}}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			// unable to call t.Parallel with t.Setenv
+			t.Setenv("foo", "bar")
+		})
+	}
+}