cmd/vet: add subtestnames analyzer; fix all existing violations
Add a new vet analyzer that checks t.Run subtest names don't contain characters requiring quoting when re-running via "go test -run". This enforces the style guide rule: don't use spaces or punctuation in subtest names. The analyzer flags: - Direct t.Run calls with string literal names containing spaces, regex metacharacters, quotes, or other problematic characters - Table-driven t.Run(tt.name, ...) calls where tt ranges over a slice/map literal with bad name field values Also fix all 978 existing violations across 81 test files, replacing spaces with hyphens and shortening long sentence-like names to concise hyphenated forms. Updates #19242 Change-Id: Ib0ad96a111bd8e764582d1d4902fe2599454ab65 Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
This commit is contained in:
Brad Fitzpatrick
committed by
Brad Fitzpatrick
Brad Fitzpatrick
parent
0f02c20c5e
commit
5ef3713c9f
@@ -0,0 +1,227 @@
|
||||
// Copyright (c) Tailscale Inc & contributors
|
||||
// SPDX-License-Identifier: BSD-3-Clause
|
||||
|
||||
// Package subtestnames checks that t.Run subtest names don't contain characters
|
||||
// that require quoting or escaping when re-running via "go test -run".
|
||||
//
|
||||
// Go's testing package rewrites subtest names: spaces become underscores,
|
||||
// non-printable characters get escaped, and regex metacharacters require
|
||||
// escaping in -run patterns. This makes it hard to re-run specific subtests
|
||||
// or search for them in code.
|
||||
//
|
||||
// This analyzer flags:
|
||||
// - Direct t.Run calls with string literal names containing bad characters
|
||||
// - t.Run calls using tt.name (or similar) where tt ranges over a slice/map
|
||||
// of test cases with string literal names containing bad characters
|
||||
package subtestnames
|
||||
|
||||
import (
|
||||
"go/ast"
|
||||
"go/token"
|
||||
"strconv"
|
||||
"strings"
|
||||
"unicode"
|
||||
|
||||
"golang.org/x/tools/go/analysis"
|
||||
"golang.org/x/tools/go/analysis/passes/inspect"
|
||||
"golang.org/x/tools/go/ast/inspector"
|
||||
)
|
||||
|
||||
// Analyzer checks that t.Run subtest names are clean for use with "go test -run".
|
||||
var Analyzer = &analysis.Analyzer{
|
||||
Name: "subtestnames",
|
||||
Doc: "check that t.Run subtest names don't require quoting when re-running via go test -run",
|
||||
Requires: []*analysis.Analyzer{inspect.Analyzer},
|
||||
Run: run,
|
||||
}
|
||||
|
||||
// badChars are characters that are problematic in subtest names.
|
||||
// Spaces are rewritten to underscores by testing.rewrite, and regex
|
||||
// metacharacters require escaping in -run patterns.
|
||||
const badChars = " \t\n\r^$.*+?()[]{}|\\'\"#"
|
||||
|
||||
// hasBadChar reports whether s contains any character that would be
|
||||
// problematic in a subtest name.
|
||||
func hasBadChar(s string) bool {
|
||||
return strings.ContainsAny(s, badChars) || strings.ContainsFunc(s, func(r rune) bool {
|
||||
return !unicode.IsPrint(r)
|
||||
})
|
||||
}
|
||||
|
||||
// hasBadDash reports whether s starts or ends with a dash, which is
|
||||
// problematic in subtest names because "go test -run" may interpret a
|
||||
// leading dash as a flag, and trailing dashes are confusing.
|
||||
func hasBadDash(s string) bool {
|
||||
return strings.HasPrefix(s, "-") || strings.HasSuffix(s, "-")
|
||||
}
|
||||
|
||||
func run(pass *analysis.Pass) (any, error) {
|
||||
insp := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
|
||||
|
||||
// Build a stack of enclosing nodes so we can find the RangeStmt
|
||||
// enclosing a given t.Run call.
|
||||
nodeFilter := []ast.Node{
|
||||
(*ast.RangeStmt)(nil),
|
||||
(*ast.CallExpr)(nil),
|
||||
}
|
||||
|
||||
var rangeStack []*ast.RangeStmt
|
||||
|
||||
insp.Nodes(nodeFilter, func(n ast.Node, push bool) bool {
|
||||
switch n := n.(type) {
|
||||
case *ast.RangeStmt:
|
||||
if push {
|
||||
rangeStack = append(rangeStack, n)
|
||||
} else {
|
||||
rangeStack = rangeStack[:len(rangeStack)-1]
|
||||
}
|
||||
return true
|
||||
case *ast.CallExpr:
|
||||
if !push {
|
||||
return true
|
||||
}
|
||||
checkCallExpr(pass, n, rangeStack)
|
||||
return true
|
||||
}
|
||||
return true
|
||||
})
|
||||
|
||||
return nil, nil
|
||||
}
|
||||
|
||||
func checkCallExpr(pass *analysis.Pass, call *ast.CallExpr, rangeStack []*ast.RangeStmt) {
|
||||
// Check if this is a t.Run(...) or b.Run(...) call.
|
||||
sel, ok := call.Fun.(*ast.SelectorExpr)
|
||||
if !ok || sel.Sel.Name != "Run" || len(call.Args) < 2 {
|
||||
return
|
||||
}
|
||||
|
||||
// Verify the receiver is *testing.T, *testing.B, or *testing.F.
|
||||
if !isTestingTBF(pass, sel) {
|
||||
return
|
||||
}
|
||||
|
||||
nameArg := call.Args[0]
|
||||
|
||||
// Case 1: Direct string literal, e.g. t.Run("foo bar", ...)
|
||||
if lit, ok := nameArg.(*ast.BasicLit); ok && lit.Kind == token.STRING {
|
||||
val, err := strconv.Unquote(lit.Value)
|
||||
if err != nil {
|
||||
return
|
||||
}
|
||||
if hasBadChar(val) {
|
||||
pass.Reportf(lit.Pos(), "subtest name %s contains characters that require quoting in go test -run patterns", lit.Value)
|
||||
} else if hasBadDash(val) {
|
||||
pass.Reportf(lit.Pos(), "subtest name %s starts or ends with '-' which is problematic in go test -run patterns", lit.Value)
|
||||
}
|
||||
return
|
||||
}
|
||||
|
||||
// Case 2: Selector expression like tt.name, tc.name, etc.
|
||||
// where tt is a range variable over a slice/map of test cases.
|
||||
selExpr, ok := nameArg.(*ast.SelectorExpr)
|
||||
if !ok {
|
||||
return
|
||||
}
|
||||
ident, ok := selExpr.X.(*ast.Ident)
|
||||
if !ok {
|
||||
return
|
||||
}
|
||||
|
||||
// Find the enclosing range statement where ident is the value variable.
|
||||
for i := len(rangeStack) - 1; i >= 0; i-- {
|
||||
rs := rangeStack[i]
|
||||
valIdent, ok := rs.Value.(*ast.Ident)
|
||||
if !ok || valIdent.Obj != ident.Obj {
|
||||
continue
|
||||
}
|
||||
// Found the range statement. Check the source being iterated.
|
||||
checkRangeSource(pass, rs.X, selExpr.Sel)
|
||||
return
|
||||
}
|
||||
}
|
||||
|
||||
// isTestingTBF checks whether sel looks like a method call on *testing.T, *testing.B, or *testing.F.
|
||||
func isTestingTBF(pass *analysis.Pass, sel *ast.SelectorExpr) bool {
|
||||
typ := pass.TypesInfo.TypeOf(sel.X)
|
||||
if typ != nil {
|
||||
s := typ.String()
|
||||
return s == "*testing.T" || s == "*testing.B" || s == "*testing.F"
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
// checkRangeSource examines the expression being ranged over and checks
|
||||
// composite literal elements for bad subtest name fields.
|
||||
func checkRangeSource(pass *analysis.Pass, rangeExpr ast.Expr, fieldName *ast.Ident) {
|
||||
switch x := rangeExpr.(type) {
|
||||
case *ast.Ident:
|
||||
if x.Obj == nil {
|
||||
return
|
||||
}
|
||||
switch decl := x.Obj.Decl.(type) {
|
||||
case *ast.AssignStmt:
|
||||
// e.g. tests := []struct{...}{...}
|
||||
for _, rhs := range decl.Rhs {
|
||||
checkCompositeLit(pass, rhs, fieldName)
|
||||
}
|
||||
case *ast.ValueSpec:
|
||||
// e.g. var tests = []struct{...}{...}
|
||||
for _, val := range decl.Values {
|
||||
checkCompositeLit(pass, val, fieldName)
|
||||
}
|
||||
}
|
||||
case *ast.CompositeLit:
|
||||
checkCompositeLit(pass, x, fieldName)
|
||||
}
|
||||
}
|
||||
|
||||
// checkCompositeLit checks a composite literal (slice/map) for elements
|
||||
// that have a field with a bad subtest name.
|
||||
func checkCompositeLit(pass *analysis.Pass, expr ast.Expr, fieldName *ast.Ident) {
|
||||
comp, ok := expr.(*ast.CompositeLit)
|
||||
if !ok {
|
||||
return
|
||||
}
|
||||
|
||||
for _, elt := range comp.Elts {
|
||||
// For map literals, check the value.
|
||||
if kv, ok := elt.(*ast.KeyValueExpr); ok {
|
||||
elt = kv.Value
|
||||
}
|
||||
checkStructLitField(pass, elt, fieldName)
|
||||
}
|
||||
}
|
||||
|
||||
// checkStructLitField checks a struct literal for a field with the given name
|
||||
// that contains a bad subtest name string.
|
||||
func checkStructLitField(pass *analysis.Pass, expr ast.Expr, fieldName *ast.Ident) {
|
||||
comp, ok := expr.(*ast.CompositeLit)
|
||||
if !ok {
|
||||
return
|
||||
}
|
||||
|
||||
for _, elt := range comp.Elts {
|
||||
kv, ok := elt.(*ast.KeyValueExpr)
|
||||
if !ok {
|
||||
continue
|
||||
}
|
||||
key, ok := kv.Key.(*ast.Ident)
|
||||
if !ok || key.Name != fieldName.Name {
|
||||
continue
|
||||
}
|
||||
lit, ok := kv.Value.(*ast.BasicLit)
|
||||
if !ok || lit.Kind != token.STRING {
|
||||
continue
|
||||
}
|
||||
val, err := strconv.Unquote(lit.Value)
|
||||
if err != nil {
|
||||
continue
|
||||
}
|
||||
if hasBadChar(val) {
|
||||
pass.Reportf(lit.Pos(), "subtest name %s contains characters that require quoting in go test -run patterns", lit.Value)
|
||||
} else if hasBadDash(val) {
|
||||
pass.Reportf(lit.Pos(), "subtest name %s starts or ends with '-' which is problematic in go test -run patterns", lit.Value)
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,15 @@
|
||||
// Copyright (c) Tailscale Inc & contributors
|
||||
// SPDX-License-Identifier: BSD-3-Clause
|
||||
|
||||
package subtestnames
|
||||
|
||||
import (
|
||||
"testing"
|
||||
|
||||
"golang.org/x/tools/go/analysis/analysistest"
|
||||
)
|
||||
|
||||
func TestAnalyzer(t *testing.T) {
|
||||
testdata := analysistest.TestData()
|
||||
analysistest.Run(t, testdata, Analyzer, "example")
|
||||
}
|
||||
@@ -0,0 +1,112 @@
|
||||
// Copyright (c) Tailscale Inc & contributors
|
||||
// SPDX-License-Identifier: BSD-3-Clause
|
||||
|
||||
package example
|
||||
|
||||
import "testing"
|
||||
|
||||
func TestDirect(t *testing.T) {
|
||||
// Bad: spaces
|
||||
t.Run("that everything's cool", func(t *testing.T) {}) // want `subtest name "that everything's cool" contains characters that require quoting`
|
||||
|
||||
// Bad: apostrophe
|
||||
t.Run("it's working", func(t *testing.T) {}) // want `subtest name "it's working" contains characters that require quoting`
|
||||
|
||||
// Bad: regex metacharacters
|
||||
t.Run("test(foo)", func(t *testing.T) {}) // want `subtest name "test\(foo\)" contains characters that require quoting`
|
||||
t.Run("test[0]", func(t *testing.T) {}) // want `subtest name "test\[0\]" contains characters that require quoting`
|
||||
t.Run("a|b", func(t *testing.T) {}) // want `subtest name "a\|b" contains characters that require quoting`
|
||||
t.Run("a*b", func(t *testing.T) {}) // want `subtest name "a\*b" contains characters that require quoting`
|
||||
t.Run("a+b", func(t *testing.T) {}) // want `subtest name "a\+b" contains characters that require quoting`
|
||||
t.Run("a.b", func(t *testing.T) {}) // want `subtest name "a\.b" contains characters that require quoting`
|
||||
t.Run("^start", func(t *testing.T) {}) // want `subtest name "\^start" contains characters that require quoting`
|
||||
t.Run("end$", func(t *testing.T) {}) // want `subtest name "end\$" contains characters that require quoting`
|
||||
t.Run("a{2}", func(t *testing.T) {}) // want `subtest name "a\{2\}" contains characters that require quoting`
|
||||
t.Run("a?b", func(t *testing.T) {}) // want `subtest name "a\?b" contains characters that require quoting`
|
||||
t.Run("a\\b", func(t *testing.T) {}) // want `subtest name "a\\\\b" contains characters that require quoting`
|
||||
|
||||
// Bad: double quotes
|
||||
t.Run("say \"hello\"", func(t *testing.T) {}) // want `subtest name "say \\"hello\\"" contains characters that require quoting`
|
||||
|
||||
// Bad: hash
|
||||
t.Run("comment#1", func(t *testing.T) {}) // want `subtest name "comment#1" contains characters that require quoting`
|
||||
|
||||
// Bad: leading/trailing dash
|
||||
t.Run("-leading-dash", func(t *testing.T) {}) // want `subtest name "-leading-dash" starts or ends with '-' which is problematic`
|
||||
t.Run("trailing-dash-", func(t *testing.T) {}) // want `subtest name "trailing-dash-" starts or ends with '-' which is problematic`
|
||||
t.Run("-both-", func(t *testing.T) {}) // want `subtest name "-both-" starts or ends with '-' which is problematic`
|
||||
|
||||
// Good: clean names
|
||||
t.Run("zero-passes", func(t *testing.T) {})
|
||||
t.Run("simple_test", func(t *testing.T) {})
|
||||
t.Run("CamelCase", func(t *testing.T) {})
|
||||
t.Run("with-dashes", func(t *testing.T) {})
|
||||
t.Run("123", func(t *testing.T) {})
|
||||
t.Run("comma,separated", func(t *testing.T) {})
|
||||
t.Run("colon:value", func(t *testing.T) {})
|
||||
t.Run("slash/path", func(t *testing.T) {})
|
||||
t.Run("equals=sign", func(t *testing.T) {})
|
||||
}
|
||||
|
||||
func TestTableDriven(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
val int
|
||||
}{
|
||||
{name: "bad space name", val: 1}, // want `subtest name "bad space name" contains characters that require quoting`
|
||||
{name: "good-name", val: 2},
|
||||
{name: "also(bad)", val: 3}, // want `subtest name "also\(bad\)" contains characters that require quoting`
|
||||
{name: "it's-bad", val: 4}, // want `subtest name "it's-bad" contains characters that require quoting`
|
||||
{name: "clean-name", val: 5},
|
||||
{name: "-leading-dash", val: 6}, // want `subtest name "-leading-dash" starts or ends with '-' which is problematic`
|
||||
}
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {})
|
||||
}
|
||||
}
|
||||
|
||||
func TestTableDrivenVar(t *testing.T) {
|
||||
var tests = []struct {
|
||||
name string
|
||||
val int
|
||||
}{
|
||||
{name: "has spaces", val: 1}, // want `subtest name "has spaces" contains characters that require quoting`
|
||||
{name: "ok-name", val: 2},
|
||||
}
|
||||
for _, tc := range tests {
|
||||
t.Run(tc.name, func(t *testing.T) {})
|
||||
}
|
||||
}
|
||||
|
||||
func TestTableDrivenMap(t *testing.T) {
|
||||
tests := map[string]struct {
|
||||
name string
|
||||
val int
|
||||
}{
|
||||
"key1": {name: "bad name here", val: 1}, // want `subtest name "bad name here" contains characters that require quoting`
|
||||
"key2": {name: "ok-name", val: 2},
|
||||
}
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {})
|
||||
}
|
||||
}
|
||||
|
||||
func TestNotTesting(t *testing.T) {
|
||||
// Not a t.Run call, should not trigger.
|
||||
s := struct{ Run func(string, func()) }{}
|
||||
s.Run("bad name here", func() {})
|
||||
}
|
||||
|
||||
func TestDynamicName(t *testing.T) {
|
||||
// Dynamic name, not a string literal — should not trigger.
|
||||
name := getName()
|
||||
t.Run(name, func(t *testing.T) {})
|
||||
}
|
||||
|
||||
func getName() string { return "foo" }
|
||||
|
||||
func BenchmarkDirect(b *testing.B) {
|
||||
// Also check b.Run.
|
||||
b.Run("bad name here", func(b *testing.B) {}) // want `subtest name "bad name here" contains characters that require quoting`
|
||||
b.Run("good-name", func(b *testing.B) {})
|
||||
}
|
||||
+2
-1
@@ -9,6 +9,7 @@ import (
|
||||
|
||||
"golang.org/x/tools/go/analysis/unitchecker"
|
||||
"tailscale.com/cmd/vet/jsontags"
|
||||
"tailscale.com/cmd/vet/subtestnames"
|
||||
)
|
||||
|
||||
//go:embed jsontags_allowlist
|
||||
@@ -20,5 +21,5 @@ func init() {
|
||||
}
|
||||
|
||||
func main() {
|
||||
unitchecker.Main(jsontags.Analyzer)
|
||||
unitchecker.Main(jsontags.Analyzer, subtestnames.Analyzer)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user