lint: Add linter for testhelper.Run

Our tests will check for leaked goroutines, but only if `testhelper.Run`
is called in the package being tested. A recent production outage was
caused by leaked goroutines which our tests did not catch as the package
did not use the required helper.

Create a custom linter to validate the following:
  - Each package that has tests also contains a `TestMain`.
  - `TestMain`  calls `testhelper.Run`.
  - `TestMain` is located in a file named `testhelper_test.go`.

One limitation is that the `_test` package is scanned in a separate pass
from the primary package, preventing us from validating that either the
primary package or its `_test` package has defined `TestMain`. By
default each analysis pass has no knowledge of what other packages have
or will be scanned.

To avoid accidentally requiring `TestMain` to be defined twice, we skip
linting any package with a name ending with `_test`. However, this
potentially leads to coverage gaps if all tests are in the `_test`
package. We would pass the primary package as it has no tests, and then
skip checking the test package based on its name, missing that it did
not execute `TestMain`.

A future iteration of this linter should use `Facts` to pass information
on package state between passes, but this first implementation gets us
90% of the value.

8 files changed, 200 insertions, 0 deletions

diff --git a/.golangci.yml b/.golangci.yml
index 77b2715f7..c1346546c 100644
--- a/.golangci.yml
+++ b/.golangci.yml
@@ -147,6 +147,9 @@ linters-settings:
         unavailable_code:
           included-functions:
             - gitlab.com/gitlab-org/gitaly/v16/internal/structerr.NewUnavailable
+        testhelper_run:
+          included-functions:
+            - gitlab.com/gitlab-org/gitaly/v16/internal/testhelper.Run
 
 issues:
   exclude-use-default: false
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/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/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",
+	)
+}