testquality: the allowlist key is the package's, and held by a test

cmd/testquality/main.go

@@ -9,12 +9,6 @@ import (
 	"os"
 	"path/filepath"
 	"sort"
-	"strings"
-
-	"golang.org/x/tools/go/analysis"
-	"golang.org/x/tools/go/analysis/passes/inspect"
-	"golang.org/x/tools/go/ast/inspector"
-	"golang.org/x/tools/go/packages"
 
 	"github.com/spin-stack/go-tools/internal/allow"
 	"github.com/spin-stack/go-tools/internal/testquality"
@@ -35,7 +29,7 @@ func main() {
 		os.Exit(2)
 	}
 
-	findings, err := analyze(patterns)
+	findings, err := testquality.Findings(patterns...)
 	if err != nil {
 		fmt.Fprintf(os.Stderr, "%v\n", err)
 		os.Exit(2)
@@ -44,11 +38,11 @@ func main() {
 	var unexplained []string
 	used := map[string]bool{}
 	for _, f := range findings {
-		if allowed[f.key] {
-			used[f.key] = true
+		if allowed[f.Key()] {
+			used[f.Key()] = true
 			continue
 		}
-		unexplained = append(unexplained, fmt.Sprintf("%s: %s", f.pos, f.message))
+		unexplained = append(unexplained, fmt.Sprintf("%s:%d: %s", relative(f.Pos.Filename), f.Pos.Line, f.Message))
 	}
 
 	// Stale exemptions fail too, or the list stops shrinking.
@@ -69,73 +63,6 @@ func main() {
 	fmt.Printf("testquality: %d exemption(s), no new findings\n", len(allowed))
 }
 
-type finding struct {
-	key     string // package path + test name, which is what an allow-list line names
-	pos     string
-	message string
-}
-
-// tests is the build tags a test of another build is written under, in every spin-stack
-// repository: analyzed with them, a test behind one is held like any other.
-const tests = "integration,e2e"
-
-func analyze(patterns []string) ([]finding, error) {
-	cfg := &packages.Config{
-		Mode: packages.NeedName | packages.NeedFiles | packages.NeedSyntax |
-			packages.NeedTypes | packages.NeedTypesInfo | packages.NeedDeps |
-			packages.NeedImports | packages.NeedModule,
-		Tests:      true,
-		BuildFlags: []string{"-tags=" + tests},
-	}
-	pkgs, err := packages.Load(cfg, patterns...)
-	if err != nil {
-		return nil, fmt.Errorf("loading packages: %w", err)
-	}
-
-	var findings []finding
-	for _, pkg := range pkgs {
-		if len(pkg.Syntax) == 0 {
-			continue
-		}
-		pass := &analysis.Pass{
-			Analyzer:  testquality.Analyzer,
-			Fset:      pkg.Fset,
-			Files:     pkg.Syntax,
-			Pkg:       pkg.Types,
-			TypesInfo: pkg.TypesInfo,
-			Report: func(d analysis.Diagnostic) {
-				pos := pkg.Fset.Position(d.Pos)
-				findings = append(findings, finding{
-					key:     key(pkg.PkgPath, d.Message),
-					pos:     relative(pos.Filename) + ":" + fmt.Sprint(pos.Line),
-					message: d.Message,
-				})
-			},
-			ResultOf: map[*analysis.Analyzer]any{},
-		}
-		// Supplied directly: singlechecker has nowhere to put the allow-list.
-		pass.ResultOf[inspect.Analyzer] = inspector.New(pkg.Syntax)
-		if _, err := testquality.Analyzer.Run(pass); err != nil {
-			return nil, fmt.Errorf("%s: %w", pkg.PkgPath, err)
-		}
-	}
-	return findings, nil
-}
-
-// key omits the line number, which would break on every edit above a test.
-func key(pkgPath, message string) string {
-	name, _, _ := strings.Cut(message, " ")
-	return packageName(pkgPath) + ":" + name
-}
-
-// packageName strips go/packages' ` [p.test]` and `_test` so allow-list lines are writable by hand.
-func packageName(pkgPath string) string {
-	if base, _, ok := strings.Cut(pkgPath, " ["); ok {
-		pkgPath = base
-	}
-	return strings.TrimSuffix(pkgPath, "_test")
-}
-
 func relative(path string) string {
 	wd, err := os.Getwd()
 	if err != nil {

internal/testquality/findings.go

@@ -0,0 +1,81 @@
+package testquality
+
+import (
+	"fmt"
+	"go/token"
+	"strings"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/inspect"
+	"golang.org/x/tools/go/ast/inspector"
+	"golang.org/x/tools/go/packages"
+)
+
+// Finding is one test the Analyzer flagged, in a package of a module.
+type Finding struct {
+	Package string // import path without go/packages' ` [p.test]` and `_test`
+	Test    string
+	Pos     token.Position
+	Message string
+}
+
+// Key is what an allowlist line names. It omits the line number, which would break on every
+// edit above a test, and the package is the one a person writes, not the one go/packages loaded.
+func (f Finding) Key() string { return f.Package + ":" + f.Test }
+
+// tests is the build tags a test of another build is written under, in every spin-stack
+// repository: analyzed with them, a test behind one is held like any other.
+const tests = "integration,e2e"
+
+// Findings runs the Analyzer over the packages patterns name, their tests included, from the
+// working directory's module.
+func Findings(patterns ...string) ([]Finding, error) {
+	cfg := &packages.Config{
+		Mode: packages.NeedName | packages.NeedFiles | packages.NeedSyntax |
+			packages.NeedTypes | packages.NeedTypesInfo | packages.NeedDeps |
+			packages.NeedImports | packages.NeedModule,
+		Tests:      true,
+		BuildFlags: []string{"-tags=" + tests},
+	}
+	pkgs, err := packages.Load(cfg, patterns...)
+	if err != nil {
+		return nil, fmt.Errorf("loading packages: %w", err)
+	}
+
+	var findings []Finding
+	for _, pkg := range pkgs {
+		if len(pkg.Syntax) == 0 {
+			continue
+		}
+		pass := &analysis.Pass{
+			Analyzer:  Analyzer,
+			Fset:      pkg.Fset,
+			Files:     pkg.Syntax,
+			Pkg:       pkg.Types,
+			TypesInfo: pkg.TypesInfo,
+			Report: func(d analysis.Diagnostic) {
+				test, _, _ := strings.Cut(d.Message, " ")
+				findings = append(findings, Finding{
+					Package: packagePath(pkg.PkgPath),
+					Test:    test,
+					Pos:     pkg.Fset.Position(d.Pos),
+					Message: d.Message,
+				})
+			},
+			ResultOf: map[*analysis.Analyzer]any{},
+		}
+		// Supplied directly: singlechecker has nowhere to put the allowlist.
+		pass.ResultOf[inspect.Analyzer] = inspector.New(pkg.Syntax)
+		if _, err := Analyzer.Run(pass); err != nil {
+			return nil, fmt.Errorf("%s: %w", pkg.PkgPath, err)
+		}
+	}
+	return findings, nil
+}
+
+func packagePath(pkgPath string) string {
+	if base, _, ok := strings.Cut(pkgPath, " ["); ok {
+		pkgPath = base
+	}
+	return strings.TrimSuffix(pkgPath, "_test")
+}

internal/testquality/testdata/mod/go.mod

@@ -0,0 +1,3 @@
+module example.com/mod
+
+go 1.27

internal/testquality/testdata/mod/pkg/e2e_test.go

@@ -0,0 +1,7 @@
+//go:build e2e
+
+package pkg
+
+import "testing"
+
+func TestEndToEnd(t *testing.T) {}

internal/testquality/testdata/mod/pkg/external_test.go

@@ -0,0 +1,5 @@
+package pkg_test
+
+import "testing"
+
+func TestExternal(t *testing.T) {}

internal/testquality/testdata/mod/pkg/internal_test.go

@@ -0,0 +1,5 @@
+package pkg
+
+import "testing"
+
+func TestInternal(t *testing.T) {}

internal/testquality/testdata/mod/pkg/pkg.go

@@ -0,0 +1 @@
+package pkg

internal/testquality/testdata/mod/pkg/tagged_test.go

@@ -0,0 +1,7 @@
+//go:build integration
+
+package pkg
+
+import "testing"
+
+func TestTagged(t *testing.T) {}

internal/testquality/testquality_test.go

@@ -1,6 +1,8 @@
 package testquality_test
 
 import (
+	"maps"
+	"path/filepath"
 	"testing"
 
 	"golang.org/x/tools/go/analysis/analysistest"
@@ -20,3 +22,28 @@ func TestTestsThatCanFailAreNotFlagged(t *testing.T) {
 func TestTestsThatNeverRunAreFlagged(t *testing.T) {
 	analysistest.Run(t, analysistest.TestData(), testquality.Analyzer, "skipped")
 }
+
+// The keys are what every repository's allowlist is written in: the package as a person names it,
+// whether the test is in the package or beside it, and a test behind a build tag held too.
+func TestFindingsAreKeyedByThePackageAPersonWrites(t *testing.T) {
+	t.Chdir(filepath.Join("testdata", "mod"))
+
+	findings, err := testquality.Findings("./...")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	got := map[string]string{}
+	for _, f := range findings {
+		got[f.Key()] = filepath.Base(f.Pos.Filename)
+	}
+	want := map[string]string{
+		"example.com/mod/pkg:TestInternal": "internal_test.go",
+		"example.com/mod/pkg:TestExternal": "external_test.go",
+		"example.com/mod/pkg:TestTagged":   "tagged_test.go",
+		"example.com/mod/pkg:TestEndToEnd": "e2e_test.go",
+	}
+	if !maps.Equal(got, want) {
+		t.Errorf("findings = %v, want %v", got, want)
+	}
+}