tolerate run without pass/failed for benchmarks

[pohly]

Something in Go 1.20 changed so that there is a run event for benchmarks. As before, there is no pass or failed. This triggers the "test is running and thus must have failed" logic in gotestsum, which causes it to report the benchmark as failed.

As this is the behavior of Go (whether it's correct or not...), gotestsum should accept such output. To reduce the risk that genuine problems go undetected, such incomplete benchmarks get reported when a panic was detected (the original motivation for this workaround).

Related-to: #413 (comment)

[pohly]

Note that the default format is not informative enough when there was a panic: that panic output is not shown.

Other formats don't have that problem:

$ go run ./ --format=standard-verbose --raw-command cat testjson/testdata/input/go-1-20-panicked-benchmark.out
goos: linux
goarch: amd64
pkg: github.com/go-logr/logr/benchmark
cpu: Intel(R) Core(TM) i9-7980XE CPU @ 2.60GHz
=== RUN   BenchmarkDiscardLogInfoOneArg
BenchmarkDiscardLogInfoOneArg
BenchmarkDiscardLogInfoOneArg-36    	12695464	        91.33 ns/op
PASS
ok  	github.com/go-logr/logr/benchmark	1.265s
panic: fabricated panic
=== Failed
=== FAIL: github.com/go-logr/logr/benchmark BenchmarkDiscardLogInfoOneArg (unknown)
=== RUN   BenchmarkDiscardLogInfoOneArg
BenchmarkDiscardLogInfoOneArg
BenchmarkDiscardLogInfoOneArg-36    	12695464	        91.33 ns/op

DONE 1 tests, 1 failure in 0.001s

This is unrelated to this PR, I just noticed because the tests use the default format.

[pohly]

It's debatable whether this should check for a panic. I opted for staying closer to the current behavior unless a benchmark completed successfully.

Alternatively, the exit code could also be considered - but it is not always available?

[pohly]

Gentle ping... this is kind of urgent because of kubernetes/kubernetes#127245.

[dnephin]

Thank you for the PR! And sorry for the delay. I have been thinking about this change, and I don't think it's quite right.

The problem is that benchmarks don't send run events, but this fix is ignoring failures, which seems like the wrong fix for the problem.

I think #416 is the right fix for this. The problem is that we're adding an empty event into the running map, which then fails later on in this code you are changing. Instead of ignoring the failure, I think we need to fix the missing event.

[dnephin]

I added a comment to that issue with some other ideas about a fix. I like your suggestion of looking at the exit status to figure out if we might have failing tests with missing end events, but I'm not sure yet exactly what that could should look like.

As a workaround, is it possible to run the benchmarks without gotestsum, or do they get run together with tests?

[pohly]

I think #416 is the right fix for this.

I got odd results when I tried it - see #416 (comment).

As a workaround, is it possible to run the benchmarks without gotestsum, or do they get run together with tests?

The shell scripts which run the benchmarks don't know that they are running benchmarks 😢 It all depends on what's in the package and which parameters are passed through.

The workaround while we try to figure out a solution would be to revert to what Kubernetes was doing previously: run go test -json, capture output, then summarize with gobenchstat. It would still produce erroneous "Failed" output in the summary, but at least the jobs would be considered as passing again because the non-zero exit code of gobenchstat would get ignored.

[pohly]

@dnephin: ping?

Another alternative solution would be a command line option like --pass-through-exit-code: if true (not the default), the result of the go test invocation is used as the exit code of gotestsum. That doesn't address the confusion when the output of gotestsum reports FAIL, but at least CI success/failure results are the same with and without gotestsum.

This might help with flakes, too. As I learned, go test -json doesn't really cause the test binary to write JSON. There's still parsing of text output involved. If the test binary writes additional text randomly (logging...), then there is a risk that parsing randomly fails. I don't know how likely that is, but I suppose it cannot be ruled out.

[pohly]

Another alternative solution would be a command line option like --pass-through-exit-code:

That won't actually help. In contrast to what I expected, the exit code is zero already. It's the <failure message="Failed;" type=""> in the JUnit file which causes our CI to treat the job as failed, so we really have to focus on suppressing those.