From 53017a39ae3660ac73c3a5aa30e4af6fd0ed8b6c Mon Sep 17 00:00:00 2001 From: Emmanuel T Odeke Date: Wed, 1 Jan 2020 01:03:11 -0800 Subject: [PATCH] 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 TryBot-Result: Gobot Gobot Reviewed-by: Alan Donovan --- .../testinggoroutine/testdata/src/a/a.go | 261 ++++++++++++++++++ .../testinggoroutine/testinggoroutine.go | 165 +++++++++++ .../testinggoroutine/testinggoroutine_test.go | 17 ++ 3 files changed, 443 insertions(+) create mode 100644 go/analysis/passes/testinggoroutine/testdata/src/a/a.go create mode 100644 go/analysis/passes/testinggoroutine/testinggoroutine.go create mode 100644 go/analysis/passes/testinggoroutine/testinggoroutine_test.go diff --git a/go/analysis/passes/testinggoroutine/testdata/src/a/a.go b/go/analysis/passes/testinggoroutine/testdata/src/a/a.go new file mode 100644 index 00000000000..5fe90417c3b --- /dev/null +++ b/go/analysis/passes/testinggoroutine/testdata/src/a/a.go @@ -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) + } +} diff --git a/go/analysis/passes/testinggoroutine/testinggoroutine.go b/go/analysis/passes/testinggoroutine/testinggoroutine.go new file mode 100644 index 00000000000..2f6604db17f --- /dev/null +++ b/go/analysis/passes/testinggoroutine/testinggoroutine.go @@ -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 + }) +} diff --git a/go/analysis/passes/testinggoroutine/testinggoroutine_test.go b/go/analysis/passes/testinggoroutine/testinggoroutine_test.go new file mode 100644 index 00000000000..1a590263a14 --- /dev/null +++ b/go/analysis/passes/testinggoroutine/testinggoroutine_test.go @@ -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") +}