]> Cypherpunks repositories - gostls13.git/commitdiff
[release-branch.go1.15] cmd/test2json: do not emit a final Action if the result is...
authorBryan C. Mills <bcmills@google.com>
Fri, 14 Aug 2020 21:44:22 +0000 (17:44 -0400)
committerDmitri Shuralyov <dmitshur@golang.org>
Wed, 2 Sep 2020 15:02:34 +0000 (15:02 +0000)
If we are parsing a test output, and the test does not end in the
usual PASS or FAIL line (say, because it panicked), then we need the
exit status of the test binary in order to determine whether the test
passed or failed. If we don't have that status available, we shouldn't
guess arbitrarily — instead, we should omit the final "pass" or "fail"
action entirely.

(In practice, we nearly always DO have the final status, such as when
running 'go test' or 'go tool test2json some.exe'.)

Updates #40132
Fixes #40805

Change-Id: Iae482577361a6033395fe4a05d746b980e18c3de
Reviewed-on: https://go-review.googlesource.com/c/go/+/248624
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
(cherry picked from commit 1b86bdbdc3991c13c6ed156100a5f4918fdd9c6b)
Reviewed-on: https://go-review.googlesource.com/c/go/+/248725

src/cmd/go/internal/test/test.go
src/cmd/go/testdata/script/test_json_exit.txt [new file with mode: 0644]
src/cmd/internal/test2json/test2json.go
src/cmd/internal/test2json/testdata/benchshort.json
src/cmd/internal/test2json/testdata/empty.json
src/cmd/test2json/main.go

index 873a76aa38037e0d6cc5c00acfc12ddca1d0e2ff..77bfc11fe9a45dd64fd581216975debe8ca248a2 100644 (file)
@@ -1079,9 +1079,13 @@ func (c *runCache) builderRunTest(b *work.Builder, a *work.Action) error {
        }
 
        var stdout io.Writer = os.Stdout
+       var err error
        if testJSON {
                json := test2json.NewConverter(lockedStdout{}, a.Package.ImportPath, test2json.Timestamp)
-               defer json.Close()
+               defer func() {
+                       json.Exited(err)
+                       json.Close()
+               }()
                stdout = json
        }
 
@@ -1185,7 +1189,7 @@ func (c *runCache) builderRunTest(b *work.Builder, a *work.Action) error {
        }
 
        t0 := time.Now()
-       err := cmd.Start()
+       err = cmd.Start()
 
        // This is a last-ditch deadline to detect and
        // stop wedged test binaries, to keep the builders
diff --git a/src/cmd/go/testdata/script/test_json_exit.txt b/src/cmd/go/testdata/script/test_json_exit.txt
new file mode 100644 (file)
index 0000000..dc7ffb0
--- /dev/null
@@ -0,0 +1,102 @@
+[short] skip
+
+go test -c -o mainpanic.exe ./mainpanic &
+go test -c -o mainexit0.exe ./mainexit0 &
+go test -c -o testpanic.exe ./testpanic &
+go test -c -o testbgpanic.exe ./testbgpanic &
+wait
+
+# Test binaries that panic in TestMain should be marked as failing.
+
+! go test -json ./mainpanic
+stdout '"Action":"fail"'
+! stdout '"Action":"pass"'
+
+! go tool test2json ./mainpanic.exe
+stdout '"Action":"fail"'
+! stdout '"Action":"pass"'
+
+# Test binaries that exit with status 0 should be marked as passing.
+
+go test -json ./mainexit0
+stdout '"Action":"pass"'
+! stdout '"Action":"fail"'
+
+go tool test2json ./mainexit0.exe
+stdout '"Action":"pass"'
+! stdout '"Action":"fail"'
+
+# Test functions that panic should never be marked as passing
+# (https://golang.org/issue/40132).
+
+! go test -json ./testpanic
+stdout '"Action":"fail"'
+! stdout '"Action":"pass"'
+
+! go tool test2json ./testpanic.exe -test.v
+stdout '"Action":"fail"'
+! stdout '"Action":"pass"'
+
+! go tool test2json ./testpanic.exe
+stdout '"Action":"fail"'
+! stdout '"Action":"pass"'
+
+# Tests that panic in a background goroutine should be marked as failing.
+
+! go test -json ./testbgpanic
+stdout '"Action":"fail"'
+! stdout '"Action":"pass"'
+
+! go tool test2json ./testbgpanic.exe -test.v
+stdout '"Action":"fail"'
+! stdout '"Action":"pass"'
+
+! go tool test2json ./testbgpanic.exe
+stdout '"Action":"fail"'
+! stdout '"Action":"pass"'
+
+-- go.mod --
+module m
+go 1.14
+-- mainpanic/mainpanic_test.go --
+package mainpanic_test
+
+import "testing"
+
+func TestMain(m *testing.M) {
+       panic("haha no")
+}
+-- mainexit0/mainexit0_test.go --
+package mainexit0_test
+
+import (
+       "fmt"
+       "os"
+       "testing"
+)
+
+func TestMain(m *testing.M) {
+       fmt.Println("nothing to do")
+       os.Exit(0)
+}
+-- testpanic/testpanic_test.go --
+package testpanic_test
+
+import "testing"
+
+func TestPanic(*testing.T) {
+       panic("haha no")
+}
+-- testbgpanic/testbgpanic_test.go --
+package testbgpanic_test
+
+import "testing"
+
+func TestPanicInBackground(*testing.T) {
+       c := make(chan struct{})
+       go func() {
+               panic("haha no")
+               close(c)
+       }()
+       <-c
+}
index a01a8900e822c66bd062508f64e934b6a53940f4..4eb6dd4838b507057bca801aa4d3edbbc28026fe 100644 (file)
@@ -45,10 +45,10 @@ type textBytes []byte
 
 func (b textBytes) MarshalText() ([]byte, error) { return b, nil }
 
-// A converter holds the state of a test-to-JSON conversion.
+// A Converter holds the state of a test-to-JSON conversion.
 // It implements io.WriteCloser; the caller writes test output in,
 // and the converter writes JSON output to w.
-type converter struct {
+type Converter struct {
        w        io.Writer  // JSON output stream
        pkg      string     // package to name in events
        mode     Mode       // mode bits
@@ -100,9 +100,9 @@ var (
 //
 // The pkg string, if present, specifies the import path to
 // report in the JSON stream.
-func NewConverter(w io.Writer, pkg string, mode Mode) io.WriteCloser {
-       c := new(converter)
-       *c = converter{
+func NewConverter(w io.Writer, pkg string, mode Mode) *Converter {
+       c := new(Converter)
+       *c = Converter{
                w:     w,
                pkg:   pkg,
                mode:  mode,
@@ -122,11 +122,20 @@ func NewConverter(w io.Writer, pkg string, mode Mode) io.WriteCloser {
 }
 
 // Write writes the test input to the converter.
-func (c *converter) Write(b []byte) (int, error) {
+func (c *Converter) Write(b []byte) (int, error) {
        c.input.write(b)
        return len(b), nil
 }
 
+// Exited marks the test process as having exited with the given error.
+func (c *Converter) Exited(err error) {
+       if err == nil {
+               c.result = "pass"
+       } else {
+               c.result = "fail"
+       }
+}
+
 var (
        // printed by test on successful run.
        bigPass = []byte("PASS\n")
@@ -160,7 +169,7 @@ var (
 // handleInputLine handles a single whole test output line.
 // It must write the line to c.output but may choose to do so
 // before or after emitting other events.
-func (c *converter) handleInputLine(line []byte) {
+func (c *Converter) handleInputLine(line []byte) {
        // Final PASS or FAIL.
        if bytes.Equal(line, bigPass) || bytes.Equal(line, bigFail) || bytes.HasPrefix(line, bigFailErrorPrefix) {
                c.flushReport(0)
@@ -286,7 +295,7 @@ func (c *converter) handleInputLine(line []byte) {
 }
 
 // flushReport flushes all pending PASS/FAIL reports at levels >= depth.
-func (c *converter) flushReport(depth int) {
+func (c *Converter) flushReport(depth int) {
        c.testName = ""
        for len(c.report) > depth {
                e := c.report[len(c.report)-1]
@@ -298,23 +307,22 @@ func (c *converter) flushReport(depth int) {
 // Close marks the end of the go test output.
 // It flushes any pending input and then output (only partial lines at this point)
 // and then emits the final overall package-level pass/fail event.
-func (c *converter) Close() error {
+func (c *Converter) Close() error {
        c.input.flush()
        c.output.flush()
-       e := &event{Action: "pass"}
        if c.result != "" {
-               e.Action = c.result
-       }
-       if c.mode&Timestamp != 0 {
-               dt := time.Since(c.start).Round(1 * time.Millisecond).Seconds()
-               e.Elapsed = &dt
+               e := &event{Action: c.result}
+               if c.mode&Timestamp != 0 {
+                       dt := time.Since(c.start).Round(1 * time.Millisecond).Seconds()
+                       e.Elapsed = &dt
+               }
+               c.writeEvent(e)
        }
-       c.writeEvent(e)
        return nil
 }
 
 // writeOutputEvent writes a single output event with the given bytes.
-func (c *converter) writeOutputEvent(out []byte) {
+func (c *Converter) writeOutputEvent(out []byte) {
        c.writeEvent(&event{
                Action: "output",
                Output: (*textBytes)(&out),
@@ -323,7 +331,7 @@ func (c *converter) writeOutputEvent(out []byte) {
 
 // writeEvent writes a single event.
 // It adds the package, time (if requested), and test name (if needed).
-func (c *converter) writeEvent(e *event) {
+func (c *Converter) writeEvent(e *event) {
        e.Package = c.pkg
        if c.mode&Timestamp != 0 {
                t := time.Now()
index 28e287c848a678ba2c9f748a0f054b090ed1d78e..34b03b9362fa42a7502a33935744a10edb1525a5 100644 (file)
@@ -4,4 +4,3 @@
 {"Action":"output","Output":"# but to avoid questions of timing, we just use a file with no \\n at all.\n"}
 {"Action":"output","Output":"BenchmarkFoo   \t"}
 {"Action":"output","Output":"10000 early EOF"}
-{"Action":"pass"}
index 80b5217501de1a1db7cf1d7af2b41ed477125b71..e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 100644 (file)
@@ -1 +0,0 @@
-{"Action":"pass"}
index 0385d8f246c358a39185e3c8642c07ff2aa16184..57a874193e311cb19797dee89f16055fde81edeb 100644 (file)
@@ -118,12 +118,16 @@ func main() {
                w := &countWriter{0, c}
                cmd.Stdout = w
                cmd.Stderr = w
-               if err := cmd.Run(); err != nil {
+               err := cmd.Run()
+               if err != nil {
                        if w.n > 0 {
                                // Assume command printed why it failed.
                        } else {
                                fmt.Fprintf(c, "test2json: %v\n", err)
                        }
+               }
+               c.Exited(err)
+               if err != nil {
                        c.Close()
                        os.Exit(1)
                }