mirror of
https://github.com/golang/go
synced 2024-11-18 18:44:42 -07:00
analysis/passes: report testing.Fatal* FailNow Skip* misuse in goroutines
Adds an analyzer to report an error if any tests or benchmarks have any *Fatal, FailNow, Skip* misuses in goroutines which are forbidden by the package testing, since those functions terminate the entire benchmark/test yet ideally one goroutine exiting shouldn't affect the entire benchmark/test. This first pass only works for plain goroutines and doesn't yet work with b.RunParallel. That'll be added in a subsequent CL after this one is reviewed and merged. Updates golang/go#5746 Change-Id: Ia47e5c9fd96ceced1ae9834b94f529f6ae2edaaa Reviewed-on: https://go-review.googlesource.com/c/tools/+/212920 Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Alan Donovan <adonovan@google.com>
This commit is contained in:
Emmanuel T Odeke
Emmanuel Odeke
parent
7be0a674c9
commit
53017a39ae
261
go/analysis/passes/testinggoroutine/testdata/src/a/a.go
vendored
Normal file
261
go/analysis/passes/testinggoroutine/testdata/src/a/a.go
vendored
Normal file
@ -0,0 +1,261 @@
|
||||
// Copyright 2020 The Go Authors. All rights reserved.
|
||||
// Use of this source code is governed by a BSD-style
|
||||
// license that can be found in the LICENSE file.
|
||||
|
||||
package a
|
||||
|
||||
import (
|
||||
"log"
|
||||
"sync"
|
||||
"testing"
|
||||
)
|
||||
|
||||
func TestBadFatalf(t *testing.T) {
|
||||
var wg sync.WaitGroup
|
||||
defer wg.Wait()
|
||||
|
||||
for i := 0; i < 2; i++ {
|
||||
wg.Add(1)
|
||||
go func(id int) {
|
||||
defer wg.Done()
|
||||
t.Fatalf("TestFailed: id = %v\n", id) // want "call to .+T.+Fatalf from a non-test goroutine"
|
||||
}(i)
|
||||
}
|
||||
}
|
||||
|
||||
func TestOKErrorf(t *testing.T) {
|
||||
var wg sync.WaitGroup
|
||||
defer wg.Wait()
|
||||
|
||||
for i := 0; i < 2; i++ {
|
||||
wg.Add(1)
|
||||
go func(id int) {
|
||||
defer wg.Done()
|
||||
t.Errorf("TestFailed: id = %v\n", id)
|
||||
}(i)
|
||||
}
|
||||
}
|
||||
|
||||
func BenchmarkBadFatalf(b *testing.B) {
|
||||
var wg sync.WaitGroup
|
||||
defer wg.Wait()
|
||||
|
||||
for i := 0; i < b.N; i++ {
|
||||
wg.Add(1)
|
||||
go func(id int) {
|
||||
defer wg.Done()
|
||||
b.Fatalf("TestFailed: id = %v\n", id) // want "call to .+B.+Fatalf from a non-test goroutine"
|
||||
}(i)
|
||||
}
|
||||
}
|
||||
|
||||
func TestBadFatal(t *testing.T) {
|
||||
var wg sync.WaitGroup
|
||||
defer wg.Wait()
|
||||
|
||||
for i := 0; i < 2; i++ {
|
||||
wg.Add(1)
|
||||
go func(id int) {
|
||||
defer wg.Done()
|
||||
t.Fatal("TestFailed") // want "call to .+T.+Fatal from a non-test goroutine"
|
||||
}(i)
|
||||
}
|
||||
}
|
||||
|
||||
func BenchmarkBadFatal(b *testing.B) {
|
||||
var wg sync.WaitGroup
|
||||
defer wg.Wait()
|
||||
|
||||
for i := 0; i < b.N; i++ {
|
||||
wg.Add(1)
|
||||
go func(id int) {
|
||||
defer wg.Done()
|
||||
b.Fatal("TestFailed") // want "call to .+B.+Fatal from a non-test goroutine"
|
||||
}(i)
|
||||
}
|
||||
}
|
||||
|
||||
func BenchmarkOKErrorf(b *testing.B) {
|
||||
var wg sync.WaitGroup
|
||||
defer wg.Wait()
|
||||
|
||||
for i := 0; i < b.N; i++ {
|
||||
wg.Add(1)
|
||||
go func(id int) {
|
||||
defer wg.Done()
|
||||
b.Errorf("TestFailed: %d", i)
|
||||
}(i)
|
||||
}
|
||||
}
|
||||
|
||||
func BenchmarkBadFatalGoGo(b *testing.B) {
|
||||
var wg sync.WaitGroup
|
||||
defer wg.Wait()
|
||||
|
||||
for i := 0; i < b.N; i++ {
|
||||
wg.Add(1)
|
||||
go func(id int) {
|
||||
go func() {
|
||||
defer wg.Done()
|
||||
b.Fatal("TestFailed") // want "call to .+B.+Fatal from a non-test goroutine"
|
||||
}()
|
||||
}(i)
|
||||
}
|
||||
|
||||
if false {
|
||||
defer b.Fatal("here")
|
||||
}
|
||||
|
||||
if true {
|
||||
go func() {
|
||||
b.Fatal("in here") // want "call to .+B.+Fatal from a non-test goroutine"
|
||||
}()
|
||||
}
|
||||
|
||||
func() {
|
||||
func() {
|
||||
func() {
|
||||
func() {
|
||||
go func() {
|
||||
b.Fatal("Here") // want "call to .+B.+Fatal from a non-test goroutine"
|
||||
}()
|
||||
}()
|
||||
}()
|
||||
}()
|
||||
}()
|
||||
|
||||
_ = 10 * 10
|
||||
_ = func() bool {
|
||||
go b.Fatal("Failed") // want "call to .+B.+Fatal from a non-test goroutine"
|
||||
return true
|
||||
}
|
||||
|
||||
defer func() {
|
||||
go b.Fatal("Here") // want "call to .+B.+Fatal from a non-test goroutine"
|
||||
}()
|
||||
}
|
||||
|
||||
func BenchmarkBadSkip(b *testing.B) {
|
||||
for i := 0; i < b.N; i++ {
|
||||
if i == 100 {
|
||||
go b.Skip("Skipping") // want "call to .+B.+Skip from a non-test goroutine"
|
||||
}
|
||||
if i == 22 {
|
||||
go func() {
|
||||
go func() {
|
||||
b.Skip("Skipping now") // want "call to .+B.+Skip from a non-test goroutine"
|
||||
}()
|
||||
}()
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func TestBadSkip(t *testing.T) {
|
||||
for i := 0; i < 1000; i++ {
|
||||
if i == 100 {
|
||||
go t.Skip("Skipping") // want "call to .+T.+Skip from a non-test goroutine"
|
||||
}
|
||||
if i == 22 {
|
||||
go func() {
|
||||
go func() {
|
||||
t.Skip("Skipping now") // want "call to .+T.+Skip from a non-test goroutine"
|
||||
}()
|
||||
}()
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func BenchmarkBadFailNow(b *testing.B) {
|
||||
for i := 0; i < b.N; i++ {
|
||||
if i == 100 {
|
||||
go b.FailNow() // want "call to .+B.+FailNow from a non-test goroutine"
|
||||
}
|
||||
if i == 22 {
|
||||
go func() {
|
||||
go func() {
|
||||
b.FailNow() // want "call to .+B.+FailNow from a non-test goroutine"
|
||||
}()
|
||||
}()
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func TestBadFailNow(t *testing.T) {
|
||||
for i := 0; i < 1000; i++ {
|
||||
if i == 100 {
|
||||
go t.FailNow() // want "call to .+T.+FailNow from a non-test goroutine"
|
||||
}
|
||||
if i == 22 {
|
||||
go func() {
|
||||
go func() {
|
||||
t.FailNow() // want "call to .+T.+FailNow from a non-test goroutine"
|
||||
}()
|
||||
}()
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func TestBadWithLoopCond(ty *testing.T) {
|
||||
var wg sync.WaitGroup
|
||||
defer wg.Wait()
|
||||
|
||||
for i := 0; i < 10; i++ {
|
||||
wg.Add(1)
|
||||
go func(id int) {
|
||||
defer ty.Fatalf("Why") // want "call to .+T.+Fatalf from a non-test goroutine"
|
||||
go func() {
|
||||
for j := 0; j < 2; ty.FailNow() { // want "call to .+T.+FailNow from"
|
||||
j++
|
||||
ty.Errorf("Done here")
|
||||
}
|
||||
}()
|
||||
}(i)
|
||||
}
|
||||
}
|
||||
|
||||
type customType int
|
||||
|
||||
func (ct *customType) Fatalf(fmtSpec string, args ...interface{}) {
|
||||
if fmtSpec == "" {
|
||||
panic("empty format specifier")
|
||||
}
|
||||
}
|
||||
|
||||
func (ct *customType) FailNow() {}
|
||||
func (ct *customType) Skip() {}
|
||||
|
||||
func TestWithLogFatalf(t *testing.T) {
|
||||
var wg sync.WaitGroup
|
||||
defer wg.Wait()
|
||||
|
||||
for i := 0; i < 10; i++ {
|
||||
wg.Add(1)
|
||||
go func(id int) {
|
||||
go func() {
|
||||
for j := 0; j < 2; j++ {
|
||||
log.Fatal("Done here")
|
||||
}
|
||||
}()
|
||||
}(i)
|
||||
}
|
||||
}
|
||||
|
||||
func TestWithCustomType(t *testing.T) {
|
||||
var wg sync.WaitGroup
|
||||
defer wg.Wait()
|
||||
|
||||
ct := new(customType)
|
||||
defer ct.FailNow()
|
||||
defer ct.Skip()
|
||||
|
||||
for i := 0; i < 10; i++ {
|
||||
wg.Add(1)
|
||||
go func(id int) {
|
||||
go func() {
|
||||
for j := 0; j < 2; j++ {
|
||||
ct.Fatalf("Done here: %d", i)
|
||||
}
|
||||
}()
|
||||
}(i)
|
||||
}
|
||||
}
|
||||
165
go/analysis/passes/testinggoroutine/testinggoroutine.go
Normal file
165
go/analysis/passes/testinggoroutine/testinggoroutine.go
Normal file
@ -0,0 +1,165 @@
|
||||
// Copyright 2020 The Go Authors. All rights reserved.
|
||||
// Use of this source code is governed by a BSD-style
|
||||
// license that can be found in the LICENSE file.
|
||||
|
||||
package testinggoroutine
|
||||
|
||||
import (
|
||||
"go/ast"
|
||||
"go/types"
|
||||
|
||||
"golang.org/x/tools/go/analysis"
|
||||
"golang.org/x/tools/go/analysis/passes/inspect"
|
||||
"golang.org/x/tools/go/ast/inspector"
|
||||
)
|
||||
|
||||
const Doc = `report calls to (*testing.T).Fatal from goroutines started by a test.
|
||||
|
||||
Functions that abruptly terminate a test, such as the Fatal, Fatalf, FailNow, and
|
||||
Skip{,f,Now} methods of *testing.T, must be called from the test goroutine itself.
|
||||
This checker detects calls to these functions that occur within a goroutine
|
||||
started by the test. For example:
|
||||
|
||||
func TestFoo(t *testing.T) {
|
||||
go func() {
|
||||
t.Fatal("oops") // error: (*T).Fatal called from non-test goroutine
|
||||
}()
|
||||
}
|
||||
`
|
||||
|
||||
var Analyzer = &analysis.Analyzer{
|
||||
Name: "testinggoroutine",
|
||||
Doc: Doc,
|
||||
Requires: []*analysis.Analyzer{inspect.Analyzer},
|
||||
Run: run,
|
||||
}
|
||||
|
||||
var forbidden = map[string]bool{
|
||||
"FailNow": true,
|
||||
"Fatal": true,
|
||||
"Fatalf": true,
|
||||
"Skip": true,
|
||||
"Skipf": true,
|
||||
"SkipNow": true,
|
||||
}
|
||||
|
||||
func run(pass *analysis.Pass) (interface{}, error) {
|
||||
inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
|
||||
|
||||
if !imports(pass.Pkg, "testing") {
|
||||
return nil, nil
|
||||
}
|
||||
|
||||
// Filter out anything that isn't a function declaration.
|
||||
onlyFuncs := []ast.Node{
|
||||
(*ast.FuncDecl)(nil),
|
||||
}
|
||||
|
||||
inspect.Nodes(onlyFuncs, func(node ast.Node, push bool) bool {
|
||||
fnDecl, ok := node.(*ast.FuncDecl)
|
||||
if !ok {
|
||||
return false
|
||||
}
|
||||
|
||||
if !hasBenchmarkOrTestParams(fnDecl) {
|
||||
return false
|
||||
}
|
||||
|
||||
// Now traverse the benchmark/test's body and check that none of the
|
||||
// forbidden methods are invoked in the goroutines within the body.
|
||||
ast.Inspect(fnDecl, func(n ast.Node) bool {
|
||||
goStmt, ok := n.(*ast.GoStmt)
|
||||
if !ok {
|
||||
return true
|
||||
}
|
||||
|
||||
checkGoStmt(pass, goStmt)
|
||||
|
||||
// No need to further traverse the GoStmt since right
|
||||
// above we manually traversed it in the ast.Inspect(goStmt, ...)
|
||||
return false
|
||||
})
|
||||
|
||||
return false
|
||||
})
|
||||
|
||||
return nil, nil
|
||||
}
|
||||
|
||||
func imports(pkg *types.Package, path string) bool {
|
||||
// TODO: (@odeke-em) perhaps move this function into a
|
||||
// a library since it is commonly used in x/tools/go/analysis.
|
||||
for _, imp := range pkg.Imports() {
|
||||
if imp.Path() == path {
|
||||
return true
|
||||
}
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
func hasBenchmarkOrTestParams(fnDecl *ast.FuncDecl) bool {
|
||||
// Check that the function's arguments include "*testing.T" or "*testing.B".
|
||||
params := fnDecl.Type.Params.List
|
||||
|
||||
for _, param := range params {
|
||||
if _, ok := typeIsTestingDotTOrB(param.Type); ok {
|
||||
return true
|
||||
}
|
||||
}
|
||||
|
||||
return false
|
||||
}
|
||||
|
||||
func typeIsTestingDotTOrB(expr ast.Expr) (string, bool) {
|
||||
starExpr, ok := expr.(*ast.StarExpr)
|
||||
if !ok {
|
||||
return "", false
|
||||
}
|
||||
selExpr, ok := starExpr.X.(*ast.SelectorExpr)
|
||||
if !ok {
|
||||
return "", false
|
||||
}
|
||||
|
||||
varPkg := selExpr.X.(*ast.Ident)
|
||||
if varPkg.Name != "testing" {
|
||||
return "", false
|
||||
}
|
||||
|
||||
varTypeName := selExpr.Sel.Name
|
||||
ok = varTypeName == "B" || varTypeName == "T"
|
||||
return varTypeName, ok
|
||||
}
|
||||
|
||||
// checkGoStmt traverses the goroutine and checks for the
|
||||
// use of the forbidden *testing.(B, T) methods.
|
||||
func checkGoStmt(pass *analysis.Pass, goStmt *ast.GoStmt) {
|
||||
// Otherwise examine the goroutine to check for the forbidden methods.
|
||||
ast.Inspect(goStmt, func(n ast.Node) bool {
|
||||
selExpr, ok := n.(*ast.SelectorExpr)
|
||||
if !ok {
|
||||
return true
|
||||
}
|
||||
|
||||
_, bad := forbidden[selExpr.Sel.Name]
|
||||
if !bad {
|
||||
return true
|
||||
}
|
||||
|
||||
// Now filter out false positives by the import-path/type.
|
||||
ident, ok := selExpr.X.(*ast.Ident)
|
||||
if !ok {
|
||||
return true
|
||||
}
|
||||
if ident.Obj == nil || ident.Obj.Decl == nil {
|
||||
return true
|
||||
}
|
||||
field, ok := ident.Obj.Decl.(*ast.Field)
|
||||
if !ok {
|
||||
return true
|
||||
}
|
||||
if typeName, ok := typeIsTestingDotTOrB(field.Type); ok {
|
||||
pass.ReportRangef(selExpr, "call to (*%s).%s from a non-test goroutine", typeName, selExpr.Sel)
|
||||
}
|
||||
return true
|
||||
})
|
||||
}
|
||||
17
go/analysis/passes/testinggoroutine/testinggoroutine_test.go
Normal file
17
go/analysis/passes/testinggoroutine/testinggoroutine_test.go
Normal file
@ -0,0 +1,17 @@
|
||||
// Copyright 2020 The Go Authors. All rights reserved.
|
||||
// Use of this source code is governed by a BSD-style
|
||||
// license that can be found in the LICENSE file.
|
||||
|
||||
package testinggoroutine_test
|
||||
|
||||
import (
|
||||
"testing"
|
||||
|
||||
"golang.org/x/tools/go/analysis/analysistest"
|
||||
"golang.org/x/tools/go/analysis/passes/testinggoroutine"
|
||||
)
|
||||
|
||||
func Test(t *testing.T) {
|
||||
testdata := analysistest.TestData()
|
||||
analysistest.Run(t, testdata, testinggoroutine.Analyzer, "a")
|
||||
}
|
||||
Loading…
Reference in New Issue
Block a user