Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

testing: odd interaction between -json flag and TestMain #31969

Closed
eliben opened this issue May 10, 2019 · 10 comments
Closed

testing: odd interaction between -json flag and TestMain #31969

eliben opened this issue May 10, 2019 · 10 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@eliben
Copy link
Member

eliben commented May 10, 2019

What version of Go are you using (go version)?

$ go version
go version go1.12 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/usr/local/google/home/eliben/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/usr/local/google/home/eliben/go"
GOPROXY="https://proxy.golang.org"
GORACE=""
GOROOT="/usr/lib/google-golang"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/google-golang/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build529164426=/tmp/go-build -gno-record-gcc-switches"

What did you do?

package main

import (
	"fmt"
	"os"
	"testing"
)

func TestJoe(t *testing.T) {
	fmt.Println("from joe")
}

func TestMain(m *testing.M) {
	if len(os.Getenv("XYZ")) > 0 {
		return
	}
	fmt.Println("before run")
	s := m.Run()
	fmt.Println("after run")
	os.Exit(s)
}

What did you expect to see?

When running go test with this file, the test passes. It also passes with go test -json. However, if I tickle the early return in TestMain:

$ XYZ=2 go test -json
{"Time":"2019-05-10T09:33:09.534255206-07:00","Action":"output","Package":"_testing/testmain","Output":"ok  \t_testing/testmain\t0.019s\n"}
{"Time":"2019-05-10T09:33:09.534473515-07:00","Action":"fail","Package":"_testing/testmain","Elapsed":0.019}

Note the fail status in the last line. The exit code of this go test -json run is 0, even though it (wrongly) reported failure. Regularly running go test here passes, as expected.

@andybons
Copy link
Member

@mpvl @josharian

@andybons andybons added this to the Unplanned milestone May 13, 2019
@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 13, 2019
@hanleym
Copy link

hanleym commented May 14, 2019

This seems to happen when TestMain returns without m.Run() being called; regardless of os.Exit().

@ryanbrainard
Copy link

I'm experiencing this same behavior with test2json. I'm assuming they share the same code path, so that makes sense.

To add to what is above, it does not matter if you return from TestMain() or os.Exit(0) from TestMain(), they both exhibit this behavior.

Here is my repro for test2json:

package dummy

import (
	"os"
	"testing"
)

func TestMain(m *testing.M) {
	os.Exit(0)
	// os.Exit(m.Run())
}

func TestDummy(t *testing.T) {
}

Run tests:

$ go test -c -o test-exec
$ go tool test2json -t ./test-exec
{"Time":"2019-08-27T10:10:53.931641+09:00","Action":"fail","Elapsed":0.005}

Toggling the commented lines in TestMain() results in:

$ go test -c -o test-exec         
$ go tool test2json -t ./test-exec
{"Time":"2019-08-27T10:11:51.007087+09:00","Action":"output","Output":"PASS\n"}
{"Time":"2019-08-27T10:11:51.007417+09:00","Action":"pass","Elapsed":0.006}

On a related note, when there are no tests and m.Run() is called, the test2json shows "passed" even though the docs state "skip - the test was skipped or the package contained no tests"

@gopherbot
Copy link

Change https://golang.org/cl/192104 mentions this issue: test2json: default to "pass" when the test doesn't report failures

@eliben
Copy link
Member Author

eliben commented Aug 28, 2019

I believe I understand what's going on. The test commands uses a converter from test2json to filter its stdout. test2json looks for PASS lines to mark a "pass" and FAIL lines to mark a "fail", but if no such lines are encountered in the stdout of the running test, it currently defaults to "fail" -- see

e := &event{Action: "fail"}

I sent https://golang.org/cl/192104 to change this to "pass", and all tests pass except one where the current behavior is explicitly assumed.

Trying to think of any bad side effects of this... In general, the behavior of test2json is hereby aligned to go test, which will take a "no tests run" as a passing event.

Note: this change makes the behavior similar to running in a directory with no tests, where test2json currently reports a "pass".

@mvdan
Copy link
Member

mvdan commented Aug 31, 2019

I'm not positive that this is the right fix. In #29062, the fix is to make go test fail if the test binary didn't print PASS, so I think we should apply the same fix to test2json.

@eliben
Copy link
Member Author

eliben commented Aug 31, 2019

@mvdan the main complaint in this issue was that -json has different semantics from no-json, and the CL aligns them.

If the original semantics of no-json are reversed, then the CL should be reverted.

I'll comment on #29062

tomocy pushed a commit to tomocy/go that referenced this issue Sep 1, 2019
When a test has a TestMain that doesn't run any tests (doesn't invoke
m.Run), `go test` passes, but `go test -json` reports a "fail" event
though the exit code is still 0.

This CL fixes test2json to behave similarly to `go test` in such cases -
no output from the test is taken as "pass" by default, not as "fail".

Fixes golang#31969

Change-Id: I1829d40fc30dc2879e73974fac416f6a34212ccd
Reviewed-on: https://go-review.googlesource.com/c/go/+/192104
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
t4n6a1ka pushed a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
When a test has a TestMain that doesn't run any tests (doesn't invoke
m.Run), `go test` passes, but `go test -json` reports a "fail" event
though the exit code is still 0.

This CL fixes test2json to behave similarly to `go test` in such cases -
no output from the test is taken as "pass" by default, not as "fail".

Fixes golang#31969

Change-Id: I1829d40fc30dc2879e73974fac416f6a34212ccd
Reviewed-on: https://go-review.googlesource.com/c/go/+/192104
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@mvdan
Copy link
Member

mvdan commented Sep 9, 2019

Yup. My suggestion would be to reopen this issue to revert its CL, once the other issue is fixed.

@gopherbot
Copy link

Change https://golang.org/cl/222243 mentions this issue: cmd/go: make go test -json report failures for panicking/exiting tests

gopherbot pushed a commit that referenced this issue Mar 6, 2020
'go test -json' should report that a test failed if the test binary
did not exit normally with status 0. This covers panics, non-zero
exits, and abnormal terminations.

These tests don't print a final result when run with -test.v (which is
used by 'go test -json'). The final result should be "PASS" or "FAIL"
on a line by itself. 'go test' prints "FAIL" in this case, but
includes error information.

test2json was changed in CL 192104 to report that a test passed if it
does not report a final status. This caused 'go test -json' to report
that a test passed after a panic or non-zero exit.

With this change, test2json treats "FAIL" with error information the
same as "FAIL" on a line by itself. This is intended to be a minimal
fix for backporting, but it will likely be replaced by a complete
solution for #29062.

Fixes #37555
Updates #29062
Updates #31969

Change-Id: Icb67bcd36bed97e6a8d51f4d14bf71f73c83ac3d
Reviewed-on: https://go-review.googlesource.com/c/go/+/222243
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@gopherbot
Copy link

Change https://golang.org/cl/222658 mentions this issue: [release-branch.go1.14] cmd/go: make go test -json report failures for panicking/exiting tests

gopherbot pushed a commit that referenced this issue Mar 9, 2020
…r panicking/exiting tests

'go test -json' should report that a test failed if the test binary
did not exit normally with status 0. This covers panics, non-zero
exits, and abnormal terminations.

These tests don't print a final result when run with -test.v (which is
used by 'go test -json'). The final result should be "PASS" or "FAIL"
on a line by itself. 'go test' prints "FAIL" in this case, but
includes error information.

test2json was changed in CL 192104 to report that a test passed if it
does not report a final status. This caused 'go test -json' to report
that a test passed after a panic or non-zero exit.

With this change, test2json treats "FAIL" with error information the
same as "FAIL" on a line by itself. This is intended to be a minimal
fix for backporting, but it will likely be replaced by a complete
solution for #29062.

Fixes #37671
Updates #37555
Updates #29062
Updates #31969

Change-Id: Icb67bcd36bed97e6a8d51f4d14bf71f73c83ac3d
Reviewed-on: https://go-review.googlesource.com/c/go/+/222243
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
(cherry picked from commit 5ea58c6)
Reviewed-on: https://go-review.googlesource.com/c/go/+/222658
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@golang golang locked and limited conversation to collaborators Mar 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants