Merge branch 'wc/testhelper-lint' into 'master'

lint: Add linter for testhelper.Run

Closes #5522

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6284

Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Reviewed-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Reviewed-by: Will Chandler <wchandler@gitlab.com>
Reviewed-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Co-authored-by: Will Chandler <wchandler@gitlab.com>

9 files changed, 210 insertions, 4 deletions

diff --git a/tools/golangci-lint/gitaly/lint.go b/tools/golangci-lint/gitaly/lint.go
index 0d49db4ed..93e4da5ac 100644
--- a/tools/golangci-lint/gitaly/lint.go
+++ b/tools/golangci-lint/gitaly/lint.go
@@ -26,6 +26,10 @@ func New(conf any) ([]*analysis.Analyzer, error) {
 			unavailableCodeAnalyzerName,
 			"included-functions",
 		)}),
+		newTesthelperRunAnalyzer(&testhelperRunAnalyzerSettings{IncludedFunctions: configStringSlicesAt(
+			testhelperRunAnalyzerName,
+			"included-functions",
+		)}),
 		newTestParamsOrder(),
 	}, nil
 }
diff --git a/tools/golangci-lint/gitaly/matcher.go b/tools/golangci-lint/gitaly/matcher.go
index 9b180a726..ec7620792 100644
--- a/tools/golangci-lint/gitaly/matcher.go
+++ b/tools/golangci-lint/gitaly/matcher.go
@@ -33,6 +33,8 @@ var funcNamePattern = regexp.MustCompile(`^\(?([^\\)].*)\)?\.(.*)$`)
 //     "gitlab.com/gitlab-org/gitaly/v15/internal/structerr.NewInternal",
 //   - A function of a struct inside a package:
 //     "(*gitlab.com/gitlab-org/gitaly/v15/internal/structerr.Error).Unwrap",
+//   - A local function call:
+//     "New(1)",
 //
 // This Matcher doesn't support interface match (yet).
 func (m *Matcher) MatchFunction(call *ast.CallExpr, rules []string) bool {
@@ -76,11 +78,18 @@ func (m *Matcher) functionName(call *ast.CallExpr) string {
 }
 
 func (m *Matcher) getFunction(call *ast.CallExpr) (*types.Func, bool) {
-	sel, ok := call.Fun.(*ast.SelectorExpr)
-	if !ok {
+	var ident *ast.Ident
+
+	switch ty := call.Fun.(type) {
+	case *ast.SelectorExpr:
+		ident = ty.Sel
+	case *ast.Ident:
+		ident = ty
+	default:
 		return nil, false
 	}
-	fn, ok := m.typesInfo.ObjectOf(sel.Sel).(*types.Func)
+
+	fn, ok := m.typesInfo.ObjectOf(ident).(*types.Func)
 	if !ok {
 		return nil, false
 	}
diff --git a/tools/golangci-lint/gitaly/testdata/src/testhelper_run_no_exec_testmain/testhelper_test.go b/tools/golangci-lint/gitaly/testdata/src/testhelper_run_no_exec_testmain/testhelper_test.go
new file mode 100644
index 000000000..94eb334f0
--- /dev/null
+++ b/tools/golangci-lint/gitaly/testdata/src/testhelper_run_no_exec_testmain/testhelper_test.go
@@ -0,0 +1,8 @@
+package testhelper_run_no_exec_testmain
+
+import (
+	"testing"
+)
+
+func TestMain(m *testing.M) { // want "testhelper.Run not called in TestMain"
+}
diff --git a/tools/golangci-lint/gitaly/testdata/src/testhelper_run_no_testmain/no_testmain.go b/tools/golangci-lint/gitaly/testdata/src/testhelper_run_no_testmain/no_testmain.go
new file mode 100644
index 000000000..364b9f101
--- /dev/null
+++ b/tools/golangci-lint/gitaly/testdata/src/testhelper_run_no_testmain/no_testmain.go
@@ -0,0 +1,6 @@
+package testhelper_run_no_testmain // want "no TestMain in package testhelper_run_no_testmain"
+
+import "testing"
+
+func TestMe(t *testing.T) {
+}
diff --git a/tools/golangci-lint/gitaly/testdata/src/testhelper_run_no_tests/no_tests.go b/tools/golangci-lint/gitaly/testdata/src/testhelper_run_no_tests/no_tests.go
new file mode 100644
index 000000000..a5ad77c85
--- /dev/null
+++ b/tools/golangci-lint/gitaly/testdata/src/testhelper_run_no_tests/no_tests.go
@@ -0,0 +1,4 @@
+package testhelper_run_no_tests
+
+func hello() { // OK, no tests in package.
+}
diff --git a/tools/golangci-lint/gitaly/testdata/src/testhelper_run_not_testhelper/not_testhelper.go b/tools/golangci-lint/gitaly/testdata/src/testhelper_run_not_testhelper/not_testhelper.go
new file mode 100644
index 000000000..28afec330
--- /dev/null
+++ b/tools/golangci-lint/gitaly/testdata/src/testhelper_run_not_testhelper/not_testhelper.go
@@ -0,0 +1,9 @@
+package testhelper_run_not_testhelper
+
+import "testing"
+
+func Run(m *testing.M) {}
+
+func TestMain(m *testing.M) { // want "TestMain should be placed in file 'testhelper_test.go'"
+	Run(m)
+}
diff --git a/tools/golangci-lint/gitaly/testdata/src/unavailable_code/unavailable_code_test.go b/tools/golangci-lint/gitaly/testdata/src/unavailable_code/unavailable_code_test.go
index 5493e3f7a..b107aa4ae 100644
--- a/tools/golangci-lint/gitaly/testdata/src/unavailable_code/unavailable_code_test.go
+++ b/tools/golangci-lint/gitaly/testdata/src/unavailable_code/unavailable_code_test.go
@@ -17,5 +17,5 @@ func errorWrapOkay() {
 }
 
 func errorWrapNotOkay() {
-	_ = NewUnavailable("hello world") // please avoid using the Unavailable status code: https://gitlab.com/gitlab-org/gitaly/-/blob/master/STYLE.md?plain=0#unavailable-code
+	_ = NewUnavailable("hello world") // want "please avoid using the Unavailable status code.*"
 }
diff --git a/tools/golangci-lint/gitaly/testhelper_run.go b/tools/golangci-lint/gitaly/testhelper_run.go
new file mode 100644
index 000000000..aa9226c02
--- /dev/null
+++ b/tools/golangci-lint/gitaly/testhelper_run.go
@@ -0,0 +1,136 @@
+package main
+
+import (
+	"fmt"
+	"go/ast"
+	"go/types"
+	"path/filepath"
+	"regexp"
+	"strings"
+
+	"golang.org/x/tools/go/analysis"
+)
+
+const testhelperRunAnalyzerName = "testhelper_run"
+
+type testhelperRunAnalyzerSettings struct {
+	IncludedFunctions []string `mapstructure:"included-functions"`
+}
+
+var toolPrefixPattern = regexp.MustCompile(`^gitlab.com/gitlab-org/gitaly(/v\d{2})?/tools`)
+
+// newTesthelperRunAnalyzer returns an analyzer to detect if a package that has tests does
+// not contain a `TestMain()` function that executes `testhelper.Run()`.
+// For more information:
+// https://gitlab.com/gitlab-org/gitaly/-/blob/master/STYLE.md?ref_type=heads#common-setup
+func newTesthelperRunAnalyzer(settings *testhelperRunAnalyzerSettings) *analysis.Analyzer {
+	return &analysis.Analyzer{
+		Name: testhelperRunAnalyzerName,
+		Doc:  `TestMain must be present and call testhelper.Run()`,
+		Run:  runTesthelperRunAnalyzer(settings.IncludedFunctions),
+	}
+}
+
+func runTesthelperRunAnalyzer(rules []string) func(*analysis.Pass) (interface{}, error) {
+	return func(pass *analysis.Pass) (interface{}, error) {
+		var hasTestMain, hasTests bool
+
+		// Blackbox test packages ending with `_test` are considered to be
+		// part of the primary package for compilation, but are scanned in a
+		// separate pass by the analyzer. The primary and test packages cannot
+		// both define `TestMain`. Only require `TestMain` in the primary package.
+		if strings.HasSuffix(pass.Pkg.Name(), "_test") {
+			return nil, nil
+		}
+
+		// Don't lint tools, they can't import `testhelper`.
+		if toolPrefixPattern.MatchString(pass.Pkg.Path()) {
+			return nil, nil
+		}
+
+		for _, file := range pass.Files {
+			if hasTestMain {
+				break
+			}
+
+			ast.Inspect(file, func(node ast.Node) bool {
+				if decl, ok := node.(*ast.FuncDecl); ok {
+					declName := decl.Name.Name
+
+					if declName == "TestMain" {
+						hasTestMain = true
+
+						analyzeTestMain(pass, decl, rules)
+						analyzeFilename(pass, file, decl)
+					}
+
+					// Actual tests must start with `Test`, helpers could take a `testing.TB`.
+					if strings.HasPrefix(declName, "Test") {
+						params := decl.Type.Params
+						for _, field := range params.List {
+							fieldType := pass.TypesInfo.TypeOf(field.Type)
+
+							// Do we have any tests in this package?
+							if types.Implements(fieldType, testingTB) {
+								hasTests = true
+							}
+						}
+					}
+				}
+				return true
+			})
+		}
+
+		// If we have tests but there's no `TestMain`, report.
+		if hasTests && !hasTestMain {
+			// We don't have a specific location for this failure, so use the location of the package name
+			// in its first file and provide the name in the error text. This list is sorted lexically by
+			// filename, so the location of `nolint` directives may not be stable when new files are added.
+			pass.Report(analysis.Diagnostic{
+				Pos:            pass.Files[0].Name.Pos(),
+				End:            pass.Files[0].Name.End(),
+				Message:        fmt.Sprintf("no TestMain in package %v", pass.Pkg.Path()),
+				SuggestedFixes: nil,
+			})
+		}
+
+		return nil, nil
+	}
+}
+
+func analyzeFilename(pass *analysis.Pass, file *ast.File, decl *ast.FuncDecl) {
+	fullpath := pass.Fset.File(file.Pos()).Name()
+	filename := filepath.Base(fullpath)
+
+	if filename != "testhelper_test.go" {
+		pass.Report(analysis.Diagnostic{
+			Pos:            decl.Pos(),
+			End:            decl.End(),
+			Message:        "TestMain should be placed in file 'testhelper_test.go'",
+			SuggestedFixes: nil,
+		})
+	}
+}
+
+func analyzeTestMain(pass *analysis.Pass, decl *ast.FuncDecl, rules []string) {
+	matcher := NewMatcher(pass)
+	var hasRun bool
+
+	ast.Inspect(decl, func(node ast.Node) bool {
+		if call, ok := node.(*ast.CallExpr); ok {
+			if matcher.MatchFunction(call, rules) {
+				hasRun = true
+			}
+		}
+		return true
+	})
+
+	if !hasRun {
+		pass.Report(analysis.Diagnostic{
+			Pos:            decl.Pos(),
+			End:            decl.End(),
+			Message:        "testhelper.Run not called in TestMain",
+			SuggestedFixes: nil,
+		})
+	}
+}
diff --git a/tools/golangci-lint/gitaly/testhelper_run_test.go b/tools/golangci-lint/gitaly/testhelper_run_test.go
new file mode 100644
index 000000000..e557d5287
--- /dev/null
+++ b/tools/golangci-lint/gitaly/testhelper_run_test.go
@@ -0,0 +1,30 @@
+package main
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"golang.org/x/tools/go/analysis/analysistest"
+)
+
+func TestTesthelperRun(t *testing.T) {
+	wd, err := os.Getwd()
+	if err != nil {
+		t.Fatalf("Failed to get wd: %s", err)
+	}
+
+	testdata := filepath.Join(wd, "testdata")
+	analyzer := newTesthelperRunAnalyzer(&testhelperRunAnalyzerSettings{IncludedFunctions: []string{
+		"testhelper_run_not_testhelper.Run",
+	}})
+	analysistest.Run(
+		t,
+		testdata,
+		analyzer,
+		"testhelper_run_no_tests",
+		"testhelper_run_no_testmain",
+		"testhelper_run_no_exec_testmain",
+		"testhelper_run_not_testhelper",
+	)
+}