cmd/go: go test hides the exit status of the test binary

[dnephin]

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

$ go version
go1.16.2 linux/amd6

Does this issue reproduce with the latest release?

Yes

What did you do?

When a test binary fails with a panic it exits with code 2 (like all other Go binaries). go test, however, hides that exit code and always exits with code 1. The only way I could get an exit code other than 0 or 1 from go test was to cause a build error. Common causes of panics are:

1. the test itself panics because of some bug
2. the -test.timeout is reached, and causes a panic.

In both of these cases the output from the compiled test binary will only contain the tests that have run before the panic. There is no output for the tests that were not run.

Any program which runs go test -json and wants to attempt to re-run failed tests (ex: gotestsum --rerun-fails) needs to be able to differentiate these two cases:

1. all tests ran, but some tests failed - exit code 1
2. some tests did not run, likely because of a panic - (currently exit code 1)

It seems like it is not currently possible to detect these two cases, which makes it impossible to safely rerun individual failed tests. If the program running tests attempts to re-run tests after a panic, it will only know about the tests that ran, not any of the tests that were missed.

What did you expect to see?

I would like to make go test always use the exit code of the compiled test binary. That way if the test binary panics, go test will exit code 2. If there is some reason not to use exit code 2 for this purpose, any exit code other than 1 would also work.

[dnephin]

Technically it may be possible to scan all of the test output and look for a panic: prefix in any of the lines, however that seems both expensive and error prone. I'm hoping that using the exit code is an easier and safer option.

[gopherbot]

Change https://golang.org/cl/309250 mentions this issue: cmd/go: use exit code from compiled test binary on error

[bcmills]

I would like to make go test always use the exit code of the compiled test binary.

I don't think that's coherent: in general go test may invoke multiple binaries, so what would it do if they return different exit codes?

[bcmills]

It seems like it is not currently possible to detect [tests omitted due to an earlier panic], which makes it impossible to safely rerun individual failed tests.

That seems like a reasonable concern, but I think we should address it more directly. Perhaps we could have go test inject an extra TestEvent at the end of the output in case of a panic?

Or, perhaps the behavior noted in #38382 is actually the solution here: at the moment, it seems that you can detect an incomplete test run by the presence of a Test in the JSON output that has an event with "Action":"run" but no corresponding event with "Action":"pass" or "Action":"fail". If a test case starts but does not either pass or fail, then the test run is necessarily incomplete and tests after that one may be missing.

Although, that still leaves the problem of detecting programs that panic (or invoke os.Exit(1)) before running tests, in between tests, or after all tests have completed but before TestMain returns. But perhaps that could be detected by running go test -list . and comparing that list of tests against the tests that appear in the go test -json output.

[dnephin]

@bcmills thank you for the quick reply.

I don't think that's coherent: in general go test may invoke multiple binaries, so what would it do if they return different exit codes?

There is already a clear pattern for handling this in the Go toolchain (and other software):

1. What happens if one binary fails some tests and others do not. go test uses exit code 1 (instead of 0)
2. What happens if one binary fails to compile, and another has failing tests. go test uses exit code 2 (instead of 0 or 1)
3. The implementation of SetExitStatus already encodes this pattern. Any "more severe" problem (higher value exit code) takes priority over lower values.

That seems quite coherent to me. The user is requesting one thing "run all of these tests", if that request is not satisfied (all tests are not run), then indicating it with the exit code seems appropriate. It doesn't matter that some packages did run all tests.

That seems like a reasonable concern, but I think we should address it more directly. Perhaps we could have go test inject an extra TestEvent at the end of the output in case of a panic?

I considered that, but it seems like doing so would be a lot more involved, could be significantly more error prone (because there are many cases to handle), and may not actually provide any more value than the simple solution.

Or, perhaps the behavior noted in #38382 is actually the solution here: at the moment, it seems that you can detect an incomplete test run by the presence of a Test in the JSON output that has an event with "Action":"run" but no corresponding event with "Action":"pass" or "Action":"fail"

#38382 describes an abnormal case. Most of the time, when a test panics, it does include an "Action":"Fail" event. Changing that would be a breaking change. For example:

{"Action":"output","Test":"TestPanics","Output":"--- FAIL: TestPanics (0.00s)\n"}
{"Action":"output","Test":"TestPanics","Output":"panic: this is a panic [recovered]\n"}
{"Action":"output","Test":"TestPanics","Output":"\tpanic: this is a panic\n"}
...
{"Action":"fail","Test":"TestPanics"}
{"Action":"output","Output":"FAIL\tgotest.tools/gotestsum/testjson/internal/frenzy\t0.003s\n"}
{"Action":"fail"}

This seems like the correct behaviour. It is the package that is incomplete, not the test.

that still leaves the problem of detecting programs that panic (or invoke os.Exit(1)) before running tests, in between tests, or after all tests have completed but before TestMain returns. But perhaps that could be detected by running go test -list . and comparing that list of tests against the tests that appear in the go test -json output.

How is that a better solution than simply not hiding the relevant exit code? That seems to be a lot more involved and error prone. There are many things that can change the list of tests, environment variables, build tags, or even command line flags. go list is unlikely to be able to reproduce the correct list of tests.

---

On a separate note, could you comment on why the current behaviour is correct? Why should go test hide the actual error code of the test binary?