]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/test2json: do not emit a final Action if the result is not known
authorBryan C. Mills <bcmills@google.com>
Fri, 14 Aug 2020 21:44:22 +0000 (17:44 -0400)
committerBryan C. Mills <bcmills@google.com>
Mon, 17 Aug 2020 19:43:21 +0000 (19:43 +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'.)

Fixes #40132

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>
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 9c120e08dccced8b4084c7a2959222a47a7f42d7..9cef8cf89c08b5cedd9407dada11aef5968f8b4a 100644 (file)
@@ -1098,9 +1098,13 @@ func (c *runCache) builderRunTest(b *work.Builder, ctx context.Context, a *work.
        }
 
        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
        }
 
@@ -1204,7 +1208,7 @@ func (c *runCache) builderRunTest(b *work.Builder, ctx context.Context, a *work.
        }
 
        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)
                }