From 1fba10c999613e8c04cef7f1331ff0051bfa7057 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Mart=C3=AD?= Date: Mon, 1 Jul 2019 18:30:01 +0200 Subject: [PATCH] cmd/go: fail if a test binary exits with no output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For example, if a test calls os.Exit(0), that could trick a 'go test' run into not running some of the other tests, and thinking that they all succeeded. This can easily go unnoticed and cause developers headaches. Add a simple sanity check as part of 'go test': if the test binary succeeds and doesn't print anything, we should error, as something clearly went very wrong. This is done by inspecting each of the stdout writes from the spawned process, since we don't want to read the entirety of the output into a buffer. We need to introduce a "buffered" bool var, as there's now an io.Writer layer between cmd.Stdout and &buf. A few TestMain funcs in the standard library needed fixing, as they returned without printing anything as a means to skip testing the entire package. For that purpose add testenv.MainMust, which prints a warning and prints SKIP, similar to when -run matches no tests. Finally, add tests for both os.Exit(0) and os.Exit(1), both as part of TestMain and as part of a single test, and test that the various stdout modes still do the right thing. Fixes #29062. Change-Id: Ic6f8ef3387dfc64e4cd3e8f903d7ca5f5f38d397 Reviewed-on: https://go-review.googlesource.com/c/go/+/184457 Run-TryBot: Daniel Martí TryBot-Result: Gobot Gobot Reviewed-by: Bryan C. Mills --- src/cmd/go/internal/test/test.go | 37 +++++- src/cmd/go/testdata/script/test_exit.txt | 152 +++++++++++++++++++++++ src/cmd/internal/goobj/goobj_test.go | 4 +- src/cmd/nm/nm_test.go | 4 +- src/cmd/objdump/objdump_test.go | 4 +- src/go/build/deps_test.go | 2 +- src/internal/testenv/testenv.go | 9 ++ 7 files changed, 200 insertions(+), 12 deletions(-) create mode 100644 src/cmd/go/testdata/script/test_exit.txt diff --git a/src/cmd/go/internal/test/test.go b/src/cmd/go/internal/test/test.go index fb011d4c03f18f..f4ce355189c0b1 100644 --- a/src/cmd/go/internal/test/test.go +++ b/src/cmd/go/internal/test/test.go @@ -1048,6 +1048,18 @@ func (lockedStdout) Write(b []byte) (int, error) { return os.Stdout.Write(b) } +type outputChecker struct { + w io.Writer + anyOutput bool +} + +func (o *outputChecker) Write(p []byte) (int, error) { + if !o.anyOutput && len(bytes.TrimSpace(p)) > 0 { + o.anyOutput = true + } + return o.w.Write(p) +} + // builderRunTest is the action for running a test binary. func (c *runCache) builderRunTest(b *work.Builder, a *work.Action) error { if a.Failed { @@ -1067,6 +1079,7 @@ func (c *runCache) builderRunTest(b *work.Builder, a *work.Action) error { } var buf bytes.Buffer + buffered := false if len(pkgArgs) == 0 || testBench { // Stream test output (no buffering) when no package has // been given on the command line (implicit current directory) @@ -1093,9 +1106,16 @@ func (c *runCache) builderRunTest(b *work.Builder, a *work.Action) error { stdout = io.MultiWriter(stdout, &buf) } else { stdout = &buf + buffered = true } } + // Keep track of whether we've seen any output at all. This is useful + // later, to avoid succeeding if the test binary did nothing or didn't + // reach the end of testing.M.Run. + outCheck := outputChecker{w: stdout} + stdout = &outCheck + if c.buf == nil { // We did not find a cached result using the link step action ID, // so we ran the link step. Try again now with the link output @@ -1109,7 +1129,7 @@ func (c *runCache) builderRunTest(b *work.Builder, a *work.Action) error { c.tryCacheWithID(b, a, a.Deps[0].BuildContentID()) } if c.buf != nil { - if stdout != &buf { + if !buffered { stdout.Write(c.buf.Bytes()) c.buf.Reset() } @@ -1207,6 +1227,19 @@ func (c *runCache) builderRunTest(b *work.Builder, a *work.Action) error { mergeCoverProfile(cmd.Stdout, a.Objdir+"_cover_.out") + if err == nil && !testList && !outCheck.anyOutput { + // If a test does os.Exit(0) by accident, 'go test' may succeed + // and it can take a while for a human to notice the package's + // tests didn't actually pass. + // + // If a test binary ran without error, it should have at least + // printed something, such as a PASS line. + // + // The only exceptions are when no tests have run, and the + // -test.list flag, which just prints the names of tests + // matching a pattern. + err = fmt.Errorf("test binary succeeded but did not print anything") + } if err == nil { norun := "" if !testShowPass && !testJSON { @@ -1227,7 +1260,7 @@ func (c *runCache) builderRunTest(b *work.Builder, a *work.Action) error { fmt.Fprintf(cmd.Stdout, "FAIL\t%s\t%s\n", a.Package.ImportPath, t) } - if cmd.Stdout != &buf { + if !buffered { buf.Reset() // cmd.Stdout was going to os.Stdout already } return nil diff --git a/src/cmd/go/testdata/script/test_exit.txt b/src/cmd/go/testdata/script/test_exit.txt new file mode 100644 index 00000000000000..2ab3d59b271032 --- /dev/null +++ b/src/cmd/go/testdata/script/test_exit.txt @@ -0,0 +1,152 @@ +env GO111MODULE=on + +# If a test exits with a zero status code, 'go test' prints its own error +# message and fails. +! go test ./zero +! stdout ^ok +! stdout 'exit status' +stdout 'did not print anything' +stdout ^FAIL + +# If a test exits with a non-zero status code, 'go test' fails normally. +! go test ./one +! stdout ^ok +stdout 'exit status' +! stdout 'did not print anything' +stdout ^FAIL + +# Ensure that other flags still do the right thing. +go test -list=. ./zero +stdout ExitZero + +! go test -bench=. ./zero +stdout 'did not print anything' + +# 'go test' with no args streams output without buffering. Ensure that it still +# catches a zero exit with missing output. +cd zero +! go test +stdout 'did not print anything' +cd ../normal +go test +stdout ^ok +cd .. + +# If a TestMain prints something and exits with a zero status code, 'go test' +# shouldn't complain about that. It's a common way to skip testing a package +# entirely. +go test ./main_zero_warning +! stdout 'skipping all tests' +stdout ^ok + +# With -v, we'll see the warning from TestMain. +go test -v ./main_zero_warning +stdout 'skipping all tests' +stdout ^ok + +# Listing all tests won't actually give a result if TestMain exits. That's okay, +# because this is how TestMain works. If we decide to support -list even when +# TestMain is used to skip entire packages, we can change this test case. +go test -list=. ./main_zero_warning +stdout 'skipping all tests' +! stdout TestNotListed + +# If a TestMain prints nothing and exits with a zero status code, 'go test' +# should fail. +! go test ./main_zero_nowarning +stdout 'did not print anything' + +# A test that simply prints "PASS" and exits with a zero status code shouldn't +# be OK, but we don't catch that at the moment. It's hard to tell if any test +# started but didn't finish without using -test.v. +go test ./fake_pass +stdout ^ok + +-- go.mod -- +module m + +-- ./normal/normal.go -- +package normal +-- ./normal/normal_test.go -- +package normal + +import "testing" + +func TestExitZero(t *testing.T) { +} + +-- ./zero/zero.go -- +package zero +-- ./zero/zero_test.go -- +package zero + +import ( + "os" + "testing" +) + +func TestExitZero(t *testing.T) { + os.Exit(0) +} + +-- ./one/one.go -- +package one +-- ./one/one_test.go -- +package one + +import ( + "os" + "testing" +) + +func TestExitOne(t *testing.T) { + os.Exit(1) +} + +-- ./main_zero_warning/zero.go -- +package zero +-- ./main_zero_warning/zero_test.go -- +package zero + +import ( + "fmt" + "os" + "testing" +) + +func TestMain(m *testing.M) { + fmt.Println("skipping all tests") + os.Exit(0) +} + +func TestNotListed(t *testing.T) {} + +-- ./main_zero_nowarning/zero.go -- +package zero +-- ./main_zero_nowarning/zero_test.go -- +package zero + +import ( + "os" + "testing" +) + +func TestMain(m *testing.M) { + os.Exit(0) +} + +-- ./fake_pass/fake.go -- +package fake +-- ./fake_pass/fake_test.go -- +package fake + +import ( + "fmt" + "os" + "testing" +) + +func TestFakePass(t *testing.T) { + fmt.Println("PASS") + os.Exit(0) +} diff --git a/src/cmd/internal/goobj/goobj_test.go b/src/cmd/internal/goobj/goobj_test.go index 4a4d35a413a533..4658e7d6716d89 100644 --- a/src/cmd/internal/goobj/goobj_test.go +++ b/src/cmd/internal/goobj/goobj_test.go @@ -29,9 +29,7 @@ var ( ) func TestMain(m *testing.M) { - if !testenv.HasGoBuild() { - return - } + testenv.MainMust(testenv.HasGoBuild) if err := buildGoobj(); err != nil { fmt.Println(err) diff --git a/src/cmd/nm/nm_test.go b/src/cmd/nm/nm_test.go index dcd9f360050ceb..018252793e015a 100644 --- a/src/cmd/nm/nm_test.go +++ b/src/cmd/nm/nm_test.go @@ -26,9 +26,7 @@ func TestMain(m *testing.M) { } func testMain(m *testing.M) int { - if !testenv.HasGoBuild() { - return 0 - } + testenv.MainMust(testenv.HasGoBuild) tmpDir, err := ioutil.TempDir("", "TestNM") if err != nil { diff --git a/src/cmd/objdump/objdump_test.go b/src/cmd/objdump/objdump_test.go index b24371ddea567b..e4ae9babcbb152 100644 --- a/src/cmd/objdump/objdump_test.go +++ b/src/cmd/objdump/objdump_test.go @@ -22,9 +22,7 @@ import ( var tmp, exe string // populated by buildObjdump func TestMain(m *testing.M) { - if !testenv.HasGoBuild() { - return - } + testenv.MainMust(testenv.HasGoBuild) var exitcode int if err := buildObjdump(); err == nil { diff --git a/src/go/build/deps_test.go b/src/go/build/deps_test.go index 6443094515da82..30edc38f480aed 100644 --- a/src/go/build/deps_test.go +++ b/src/go/build/deps_test.go @@ -202,7 +202,7 @@ var pkgDeps = map[string][]string{ "testing": {"L2", "flag", "fmt", "internal/race", "os", "runtime/debug", "runtime/pprof", "runtime/trace", "time"}, "testing/iotest": {"L2", "log"}, "testing/quick": {"L2", "flag", "fmt", "reflect", "time"}, - "internal/testenv": {"L2", "OS", "flag", "testing", "syscall", "internal/cfg"}, + "internal/testenv": {"L2", "OS", "flag", "fmt", "testing", "syscall", "internal/cfg"}, "internal/lazyregexp": {"L2", "OS", "regexp"}, "internal/lazytemplate": {"L2", "OS", "text/template"}, diff --git a/src/internal/testenv/testenv.go b/src/internal/testenv/testenv.go index b036aa6ebc78ce..5cb132760e5d24 100644 --- a/src/internal/testenv/testenv.go +++ b/src/internal/testenv/testenv.go @@ -13,6 +13,7 @@ package testenv import ( "errors" "flag" + "fmt" "internal/cfg" "os" "os/exec" @@ -32,6 +33,14 @@ func Builder() string { return os.Getenv("GO_BUILDER_NAME") } +func MainMust(cond func() bool) { + if !cond() { + fmt.Println("testenv: warning: can't run any tests") + fmt.Println("SKIP") + os.Exit(0) + } +} + // HasGoBuild reports whether the current system can build programs with ``go build'' // and then run them with os.StartProcess or exec.Command. func HasGoBuild() bool {