cmd/go: do not apply a kill timeout to 'go test' if -bench was set

[cespare]

Go version

go version go1.23.0 linux/amd64

Output of go env in your module/workspace:

n/a

What did you do?

The code doesn't really matter, but for the sake of illustration suppose we have this benchmark:

func Benchmark100MS(b *testing.B) {
	for i := 0; i < b.N; i++ {
		time.Sleep(100 * time.Millisecond)
	}
}

(You can test this with any benchmark.) Consider the following go test invocations:

1. go test -bench . -benchtime 70s -timeout 1s
2. go test -c -o x.test ./x/x_test.go && ./x.test -test.bench . -test.benchtime 70s -test.timeout 1s

What did you see happen?

The tests behave differently. When I run the test using go test, it kills the test after 61s:

$ go test -bench 100MS  -benchtime 70s -timeout 1s ./x/x_test.go
goos: linux
goarch: amd64
cpu: AMD Ryzen 9 3900X 12-Core Processor
Benchmark100MS-24       SIGQUIT: quit
PC=0x475921 m=0 sigcode=0

goroutine 0 gp=0x66bdc0 m=0 mp=0x66cc60 [idle]:
runtime.futex(0x66cda0, 0x80, 0x0, 0x0, 0x0, 0x0)
...
*** Test killed with quit: ran too long (1m1s).
exit status 2
FAIL    command-line-arguments  61.073s
FAIL

When I run the test binary, the -test.timeout flag has no effect and the benchmark runs to completion:

$ go test -c -o x.test ./x/x_test.go && time ./x.test -test.bench . -test.benchtime 70s -test.timeout 1s
goos: linux
goarch: amd64
cpu: AMD Ryzen 9 3900X 12-Core Processor
Benchmark100MS-24            836         100261554 ns/op
PASS
./x.test -test.bench . -test.benchtime 70s -test.timeout 1s  0.03s user 0.08s system 0% cpu 1:33.95 total

What did you expect to see?

I would expect the program to behave the same way whether invoked via go test or compiled and run directly.

What's going on here is a bit of a disagreement between the testing package and go test.

The testing package applies the timeout to tests, fuzz tests, and examples, but not benchmarks. This is very much intentional; the old issue #18845 flagged a regression in this behavior and it was fixed in 4707045. The regression test is still around: it checks that running a 1s benchmark using -timeout 750ms should pass.

go test sets its own timer to kill stuck tests. That timer is set unless -test.timeout is 0 (disabled) or if -test.fuzz is set. The go test timer is set to the value of the test timeout plus one minute, since it expects the test to time itself out normally.

So there is an inconsistency here.

I think we should resolve the inconsistency by changing go test not to time out test binaries if -test.bench was set, as it already does for -test.fuzz. The testing package clearly goes out of its way to avoid having -timeout apply to benchmarks; go test should respect that.

This certainly comes up in practice. I work on a pretty large Go codebase. The 10m default test timeout works fine for us; our tests are generally plenty fast. But when I run benchmarks across lots of packages (this is common as part of the sanity checks when doing Go upgrades, for example) I usually forget to set -timeout and then after 11m my benchmarks are killed. (The odd 11m number is the 10m default timeout value plus go test's extra minute.)

I noticed this when thinking about the related issue #48157 which is about adding per-test timeouts.

[gabyhelp]

Related Issues and Documentation

• cmd/go: fix docs for -test.timeout #4702 (closed)
• cmd/go: go test -timeout flag behavior doesn't match test binary -timeout flag behavior #20025 (closed)
• cmd/go: -test.timeout=0 does not cancel go tool's test timeout #18490 (closed)
• testing: -timeout affects benchmarks too (regression) #18845 (closed)
• cmd/go: add per-test timeouts #48157
• cmd/go: when -timeout is the default (10m), pass a nonzero -test.timeout to tests #28147 (closed)
• cmd/go: in 1.15: change in "go test" argument parsing #40763 (closed)
• cmd/go: unable to disable test timeout #14780 (closed)
• cmd/go: 'go test' should dynamically compute best timeout #12446
• testing: benchmark timeout when benchtime set too long #49973

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[ianlancetaylor]

CC @matloob @samthanawalla

[matloob]

This sounds reasonable to me.