Don't get fooled by non-testing variables

Only calls to t.Run and t.Parallel should count. Without this, the
linter might see a call to some other Run function and then complain
that it contains no call to Parallel inside.

This works by capturing the name of the testing.T parameter and only
recognizing calls to Run and Parallel on variables with that name. False
positives might occur if the parameter gets hidden.

Fixed #9

Author: rkennedy

pkg/paralleltest/paralleltest.go

@@ -42,7 +42,8 @@ func run(pass *analysis.Pass) (interface{}, error) {
 		var rangeNode ast.Node
 
 		// Check runs for test functions only
-		if !isTestFunction(funcDecl) {
+		isTest, testVar := isTestFunction(funcDecl)
+		if !isTest {
 			return
 		}
 
@@ -53,16 +54,19 @@ func run(pass *analysis.Pass) (interface{}, error) {
 				ast.Inspect(v, func(n ast.Node) bool {
 					// Check if the test method is calling t.parallel
 					if !funcHasParallelMethod {
-						funcHasParallelMethod = methodParallelIsCalledInTestFunction(n)
+						funcHasParallelMethod = methodParallelIsCalledInTestFunction(n, testVar)
 					}
 
 					// Check if the t.Run within the test function is calling t.parallel
-					if methodRunIsCalledInTestFunction(n) {
+					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
 						numberOfTestRun++
 						ast.Inspect(v, func(p ast.Node) bool {
 							if !hasParallel {
-								hasParallel = methodParallelIsCalledInTestFunction(p)
+								hasParallel = methodParallelIsCalledInTestFunction(p, innerTestVar)
 							}
 							return true
 						})
@@ -81,12 +85,15 @@ func run(pass *analysis.Pass) (interface{}, error) {
 					// nolint: gocritic
 					switch r := n.(type) {
 					case *ast.ExprStmt:
-						if methodRunIsCalledInRangeStatement(r.X) {
+						if methodRunIsCalledInRangeStatement(r.X, testVar) {
+							// r.X is a call to t.Run; find out the name of the subtest's *testing.T parameter.
+							innerTestVar := getRunCallbackParameterName(r.X)
+
 							rangeStatementOverTestCasesExists = true
 							testRunLoopIdentifier = methodRunFirstArgumentObjectName(r.X)
 
 							if !rangeStatementHasParallelMethod {
-								rangeStatementHasParallelMethod = methodParallelIsCalledInMethodRun(r.X)
+								rangeStatementHasParallelMethod = methodParallelIsCalledInMethodRun(r.X, innerTestVar)
 							}
 						}
 					}
@@ -165,7 +172,7 @@ func getLeftAndRightIdentifier(s ast.Stmt) (string, string) {
 	return leftIdentifier, rightIdentifier
 }
 
-func methodParallelIsCalledInMethodRun(node ast.Node) bool {
+func methodParallelIsCalledInMethodRun(node ast.Node, testVar string) bool {
 	var methodParallelCalled bool
 	// nolint: gocritic
 	switch callExp := node.(type) {
@@ -174,7 +181,7 @@ func methodParallelIsCalledInMethodRun(node ast.Node) bool {
 			if !methodParallelCalled {
 				ast.Inspect(arg, func(n ast.Node) bool {
 					if !methodParallelCalled {
-						methodParallelCalled = methodParallelIsCalledInRunMethod(n)
+						methodParallelCalled = methodParallelIsCalledInRunMethod(n, testVar)
 						return true
 					}
 					return false
@@ -185,32 +192,61 @@ func methodParallelIsCalledInMethodRun(node ast.Node) bool {
 	return methodParallelCalled
 }
 
-func methodParallelIsCalledInRunMethod(node ast.Node) bool {
-	return exprCallHasMethod(node, "Parallel")
+func methodParallelIsCalledInRunMethod(node ast.Node, testVar string) bool {
+	return exprCallHasMethod(node, testVar, "Parallel")
 }
 
-func methodParallelIsCalledInTestFunction(node ast.Node) bool {
-	return exprCallHasMethod(node, "Parallel")
+func methodParallelIsCalledInTestFunction(node ast.Node, testVar string) bool {
+	return exprCallHasMethod(node, testVar, "Parallel")
 }
 
-func methodRunIsCalledInRangeStatement(node ast.Node) bool {
-	return exprCallHasMethod(node, "Run")
+func methodRunIsCalledInRangeStatement(node ast.Node, testVar string) bool {
+	return exprCallHasMethod(node, testVar, "Run")
 }
 
-func methodRunIsCalledInTestFunction(node ast.Node) bool {
-	return exprCallHasMethod(node, "Run")
+func methodRunIsCalledInTestFunction(node ast.Node, testVar string) bool {
+	return exprCallHasMethod(node, testVar, "Run")
 }
-func exprCallHasMethod(node ast.Node, methodName string) bool {
+func exprCallHasMethod(node ast.Node, receiverName, methodName string) bool {
 	// nolint: gocritic
 	switch n := node.(type) {
 	case *ast.CallExpr:
 		if fun, ok := n.Fun.(*ast.SelectorExpr); ok {
-			return fun.Sel.Name == methodName
+			if receiver, ok := fun.X.(*ast.Ident); ok {
+				return receiver.Name == receiverName && fun.Sel.Name == methodName
+			}
 		}
 	}
 	return false
 }
 
+// In an expression of the form t.Run(x, func(q *testing.T) {...}), return the
+// value "q". In _most_ code, the name is probably t, but we shouldn't just
+// assume.
+func getRunCallbackParameterName(node ast.Node) string {
+	if n, ok := node.(*ast.CallExpr); ok {
+		if len(n.Args) < 2 {
+			// We want argument #2, but this call doesn't have two
+			// arguments. Maybe it's not really t.Run.
+			return ""
+		}
+		funcArg := n.Args[1]
+		if fun, ok := funcArg.(*ast.FuncLit); ok {
+			if len(fun.Type.Params.List) < 1 {
+				// Subtest function doesn't have any parameters.
+				return ""
+			}
+			firstArg := fun.Type.Params.List[0]
+			// We'll assume firstArg.Type is *testing.T.
+			if len(firstArg.Names) < 1 {
+				return ""
+			}
+			return firstArg.Names[0].Name
+		}
+	}
+	return ""
+}
+
 // Gets the object name `tc` from method t.Run(tc.Foo, func(t *testing.T)
 func methodRunFirstArgumentObjectName(node ast.Node) string {
 	// nolint: gocritic
@@ -227,30 +263,31 @@ func methodRunFirstArgumentObjectName(node ast.Node) string {
 	return ""
 }
 
-// Checks if the function has the param type *testing.T)
-func isTestFunction(funcDecl *ast.FuncDecl) bool {
+// Checks if the function has the param type *testing.T; if it does, then the
+// parameter name is returned, too.
+func isTestFunction(funcDecl *ast.FuncDecl) (bool, string) {
 	testMethodPackageType := "testing"
 	testMethodStruct := "T"
 	testPrefix := "Test"
 
 	if !strings.HasPrefix(funcDecl.Name.Name, testPrefix) {
-		return false
+		return false, ""
 	}
 
 	if funcDecl.Type.Params != nil && len(funcDecl.Type.Params.List) != 1 {
-		return false
+		return false, ""
 	}
 
 	param := funcDecl.Type.Params.List[0]
 	if starExp, ok := param.Type.(*ast.StarExpr); ok {
 		if selectExpr, ok := starExp.X.(*ast.SelectorExpr); ok {
 			if selectExpr.Sel.Name == testMethodStruct {
 				if s, ok := selectExpr.X.(*ast.Ident); ok {
-					return s.Name == testMethodPackageType
+					return s.Name == testMethodPackageType, param.Names[0].Name
 				}
 			}
 		}
 	}
 
-	return false
+	return false, ""
 }

pkg/paralleltest/testdata/src/t/t_test.go

@@ -18,8 +18,8 @@ func TestFunctionSuccessfulRangeTest(t *testing.T) {
 	}{{name: "foo"}}
 	for _, tc := range testCases {
 		tc := tc
-		t.Run(tc.name, func(t *testing.T) {
-			t.Parallel()
+		t.Run(tc.name, func(x *testing.T) {
+			x.Parallel()
 			fmt.Println(tc.name)
 		})
 	}
@@ -129,11 +129,35 @@ func TestFunctionFirstOneTestRunMissingCallToParallel(t *testing.T) {
 func TestFunctionSecondOneTestRunMissingCallToParallel(t *testing.T) {
 	t.Parallel()
 
-	t.Run("1", func(t *testing.T) {
-		t.Parallel()
+	t.Run("1", func(x *testing.T) {
+		x.Parallel()
 		fmt.Println("1")
 	})
 	t.Run("2", func(t *testing.T) { // want "Function TestFunctionSecondOneTestRunMissingCallToParallel has missing the call to method parallel in the test run"
 		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) { // want "Function TestFunctionWithParallelLookalike missing the call to method parallel"
+	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()
+}