From 8d296dc331c3ccec729be4c8c828e2c4684594f1 Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Tue, 29 Sep 2026 09:45:56 -0300 Subject: [PATCH] testquality: the allowlist key is the package's, and held by a test What a finding's key is - the package without go/packages' " [p.test]" and "_test", and the test's name - is the line every repository's allowlist is written in, and it lived in main, which has no tests: a change to either strip would have made every entry of spin's list unexplained and stale at once with nothing here failing. Findings is the package's now, and a module under testdata holds the key for a test in the package, beside it, and behind each of the two build tags. Co-Authored-By: Claude Opus 5.5 (1M context) --- cmd/testquality/main.go | 81 +------------------ internal/testquality/findings.go | 81 +++++++++++++++++++ internal/testquality/testdata/mod/go.mod | 3 + .../testquality/testdata/mod/pkg/e2e_test.go | 7 ++ .../testdata/mod/pkg/external_test.go | 5 ++ .../testdata/mod/pkg/internal_test.go | 5 ++ internal/testquality/testdata/mod/pkg/pkg.go | 1 + .../testdata/mod/pkg/tagged_test.go | 7 ++ internal/testquality/testquality_test.go | 27 +++++++ 9 files changed, 140 insertions(+), 77 deletions(-) create mode 100644 internal/testquality/findings.go create mode 100644 internal/testquality/testdata/mod/go.mod create mode 100644 internal/testquality/testdata/mod/pkg/e2e_test.go create mode 100644 internal/testquality/testdata/mod/pkg/external_test.go create mode 100644 internal/testquality/testdata/mod/pkg/internal_test.go create mode 100644 internal/testquality/testdata/mod/pkg/pkg.go create mode 100644 internal/testquality/testdata/mod/pkg/tagged_test.go diff --git a/cmd/testquality/main.go b/cmd/testquality/main.go index 47ab116..8a225ee 100644 --- a/cmd/testquality/main.go +++ b/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 { diff --git a/internal/testquality/findings.go b/internal/testquality/findings.go new file mode 100644 index 0000000..79f291f --- /dev/null +++ b/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") +} diff --git a/internal/testquality/testdata/mod/go.mod b/internal/testquality/testdata/mod/go.mod new file mode 100644 index 0000000..2f4f126 --- /dev/null +++ b/internal/testquality/testdata/mod/go.mod @@ -0,0 +1,3 @@ +module example.com/mod + +go 1.27 diff --git a/internal/testquality/testdata/mod/pkg/e2e_test.go b/internal/testquality/testdata/mod/pkg/e2e_test.go new file mode 100644 index 0000000..fa9d828 --- /dev/null +++ b/internal/testquality/testdata/mod/pkg/e2e_test.go @@ -0,0 +1,7 @@ +//go:build e2e + +package pkg + +import "testing" + +func TestEndToEnd(t *testing.T) {} diff --git a/internal/testquality/testdata/mod/pkg/external_test.go b/internal/testquality/testdata/mod/pkg/external_test.go new file mode 100644 index 0000000..f91dc3f --- /dev/null +++ b/internal/testquality/testdata/mod/pkg/external_test.go @@ -0,0 +1,5 @@ +package pkg_test + +import "testing" + +func TestExternal(t *testing.T) {} diff --git a/internal/testquality/testdata/mod/pkg/internal_test.go b/internal/testquality/testdata/mod/pkg/internal_test.go new file mode 100644 index 0000000..14a8693 --- /dev/null +++ b/internal/testquality/testdata/mod/pkg/internal_test.go @@ -0,0 +1,5 @@ +package pkg + +import "testing" + +func TestInternal(t *testing.T) {} diff --git a/internal/testquality/testdata/mod/pkg/pkg.go b/internal/testquality/testdata/mod/pkg/pkg.go new file mode 100644 index 0000000..c1caffe --- /dev/null +++ b/internal/testquality/testdata/mod/pkg/pkg.go @@ -0,0 +1 @@ +package pkg diff --git a/internal/testquality/testdata/mod/pkg/tagged_test.go b/internal/testquality/testdata/mod/pkg/tagged_test.go new file mode 100644 index 0000000..970728b --- /dev/null +++ b/internal/testquality/testdata/mod/pkg/tagged_test.go @@ -0,0 +1,7 @@ +//go:build integration + +package pkg + +import "testing" + +func TestTagged(t *testing.T) {} diff --git a/internal/testquality/testquality_test.go b/internal/testquality/testquality_test.go index f73d52c..54f2325 100644 --- a/internal/testquality/testquality_test.go +++ b/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) + } +}