go: cmd/go: add per-test timeouts

Would there still be a 10m default -timeout?

I would prefer there not to be, since hitting that timeout without hitting a 1m per-test timeout seems like a signal that we have a giant test suite operating correctly, not that we have a hanging/broken test.

My only remaining worry is what I outline in the last three paragraphs in https://github.com/golang/go/issues/48157#issuecomment-912402979 - that setting absolute time.Duration timeouts for each test feels too rigid. I think built-in support for a “scale” or multiplier would be necessary, e.g. for slower machines such as CI. Otherwise we’ll either end up with realistic timeouts that cause sporadic failures on CI, or inflated timeouts that reduce flakes but take too long to make a test fail when locally iterating and testing.

A couple of comments later in the thread seem to agree that this problem warrants some attention. I personally think that some sort of multiplier seems like a sane solution, if you can roughly define what the “base” speed is - such as an average laptop from two years ago that’s idle. This solution seems to have been working well enough for build.golang.org, as I understand some builders use a multiplier given slower hardware.

I explained that in my first comment, which I linked (and presumably the up-voters of that comment agreed to some degree):

hitting that timeout without hitting a 1m per-test timeout seems like a signal that we have a giant test suite operating correctly, not that we have a hanging/broken test.

To expand slightly, there are two ways to encounter the default 10m timeout:

1. A test is stuck/deadlocked, and after 10m the test crashes. (The fix is to fix the test.)
2. The test suite is large and very slow and eventually starts taking >10m and hitting the timeout. (The fix is to raise -timeout and, ideally, speed up the tests.)

The default -timeout of 10m is useful in scenario 1 and not useful in scenario (2). With a default -testtimeout of 1m, scenario 1 is completely taken care of (in a better way – it fails after 1m instead of 10m), so the only effect of the default -timeout of 10m is to be an annoyance in scenario 2.

this is a separate setting and they shouldn’t interact with one another.

On the contrary, these are deeply intertwined and defining the interaction between timeout settings has been a primary topic of the original proposal and subsequent discussions.

Change https://go.dev/cl/526355 mentions this issue: testing: add individual timer for tests, benchmarks and fuzz targets

No change in consensus, so accepted. 🎉 This issue now tracks the work of implementing the proposal. — rsc for the proposal review group

What is the benefit of having SetTimeout(d time.Duration) as well as the -testtimeout flag? Would it not suffice to have the former, but not the latter? There are a couple of issues I see with -testtimeout.

Firstly is a basic usability concern. I would suggest that applying the same per-test timeout to all tests (being run) would be unhelpful a lot of the time. However I can much more readily think of situations where you’d want to run go test ./..., but apply specific timeouts to specific tests by adding calls to SetTimeout. But there are far fewer situations where it would make sense to apply to same per-test timeout to all tests - to do so would require you to select the longest-running test contained within the specified packages and set -testtimeout accordingly. And this requires knowledge of code base that the user who runs go test may not possess.

Secondly is a more nuanced concern about how -testtimeout fits into the existing environment. Running go test ./... -testtimeout=1m may have a significant effect on the outcome of a test - i.e. it may fail when previously it passed. And this is an important detail that is detached from the test itself. Currently, if we want a test to be executed in parallel (with other like-minded tests), we can call Parallel at the top of that test. There is no -parallel flag that causes all tests to run in parallel. Given that a timeout can have similarly outcome-altering effects on a test, would it not be more consistent, and more explicit, for per-test timeouts only be set inside the test itself?

@mvdan’s suggestion of a way to scale the value passed to SetTimeout seems to me a good way of making -testtimeout largely redundant, whilst also providing a mechanism to adjust the per-test timeout according to the environment in which the tests are being run:

Maybe we could also think about “testing timeout scales”; so I could run my tests in a way that all per-test timeouts are tripled if I know my machine is significantly slower than an average laptop (e.g. public CI). I think Go’s builders do something similar, where very slow computers such as small ARM/RISCV boards get many timeouts multiplied to avoid benign timeouts.

There is no safe way to attribute which goroutines need stopping (and then also no way to safe way to stop them).

If we had a testing.TB Context method then the tests and benchmarks could do cooperative graceful shutdown. (Examples are a bit trickier without further changes, but are also less likely to be problematic.)

So, as per my reading, -testtimeout or -timeout just set the per-test default timeout, so T.SetTimeout always takes precedence. Is that correct?

I think that’s the most intuitive to the test author. If someone wants to constrain the overall run-time of tests, they’d use the global -timeout, presumably.

1. If the -testtimeout flag is omitted but -timeout is specified explicitly, then that sets the default too.

I understand the reasoning behind this, but I’m also slightly uneasy about it. Many CIs currently use lines like go test -timeout=30m ./...; that means they would get zero net benefit from this proposal, unless they noticed they’d need to change to go test -timeout=30m -testtimeout=1m ./....

1. When a test timeout happens, the whole process still gets killed. There’s nothing we can really do about that.

Could you expand on the reason why we can’t do better?

As a separate point; would https://pkg.go.dev/testing#T.Deadline be updated to take into account the timeout of the running test too? I assume it would be, but just clarifying since this proposal doesn’t say.

Overall I really like this idea, for the reasons Bryan and Caleb mentioned too. I think my only main worry is that setting a per-test timeout with T.SetTimeout would be hard. Some machines are one or even two orders of magnitude faster than others in terms of CPU, disk, or network, especially when you take into account busy machines, like public shared CI runners.

This is not a problem for the -timeout or -testtimeout flags; they can be changed depending on the context. I can use -testtimeout=1m on my laptop, and -testtimeout=3m on the public CI, for example. However, T.SetTimeout is fairly static and can’t be adjusted depending on the environment, as far as I can see.

Maybe we could also think about “testing timeout scales”; so I could run my tests in a way that all per-test timeouts are tripled if I know my machine is significantly slower than an average laptop (e.g. public CI). I think Go’s builders do something similar, where very slow computers such as small ARM/RISCV boards get many timeouts multiplied to avoid benign timeouts.

This sounds great. Besides being an overall better experience when there’s a hanging test, it may allow for simpler test code in some situations. Sometimes I’ve written tests which exercise concurrent code which, if it fails, is expected to hang/deadlock; then I spawn my own timer to fail the test if it hasn’t completed after, say, 5s. Depending on whether the kill-the-whole-test behavior is acceptable, I could change these to simply use t.SetTimeout(5 * time.Second) instead.

When a test timeout happens, the whole process still gets killed. There’s nothing we can really do about that. But the failure does say which test function timed out.

I suspect this would help (at least somewhat) with the use-cases motivating #39038, too.

As a separate point; would https://pkg.go.dev/testing#T.Deadline be updated to take into account the timeout of the running test too? I assume it would be, but just clarifying since this proposal doesn’t say.

I wonder if this question got lost in the conversation. I was looking at the API again, and it says:

Deadline reports the time at which the test binary will have exceeded the timeout specified by the -timeout flag.

My intuition is that we should change the docs and behavior to account for per-test timeouts, but it’s worth noting that the current docs specify that it only looks at the timeout for the whole test binary.

Based on the discussion above, this proposal seems like a likely accept. — rsc for the proposal review group

@mvdan There is already an absolute duration in -timeout=, right? It seems like maybe we should have an automatic timeout scale (essentially elevating what cmd/dist does), but that could be done in a separate, orthogonal proposal. And it might not be worth doing until we have more information, since tests can of course check for environment variables and scale on their own, and people can write helpers that do that.

@joe-mann to me, most of the value of this proposal comes from -testtimeout.

The way I would use it, it’s mostly about improving the handling of deadlocked tests. For example, I interact with a lot of test suites which can take several minutes to run on heavily loaded CI workers but each individual test takes a few seconds at most. It’s not like I would find the longest-running test and set -testtimeout to slightly higher than that; instead, I would want -testtimeout to be high enough that no reasonable test could possibly hit it without a bug (whatever that time scale happens to be for the domain). For my test suites, the default value of 1m sounds pretty good.

Then, when a test deadlocks, go test fails faster (up to 9m sooner than it would’ve otherwise, assuming the default -timeout 10m, or even more if you’ve set -timeout higher). And for really large test suites, you don’t have to go through the dance of (1) start bumping up against the 10m default timeout and get some spurious failures, (2) look into whether they are “natural” timeouts or deadlocks, and (3) raise the -timeout. (That’s assuming we delete the 10m default -timeout as I brought up in https://github.com/golang/go/issues/48157#issuecomment-911946245.)

I think SetTimeout is useful for certain tests that are specifically about checking that a particular thing doesn’t hang. But I think I would use SetTimeout occasionally whereas -testtimeout would help me every day.

Making the default overall test binary timeout “off” sounds fine to me. We just need to make sure the new 1m per-test timeout applies to all the kinds of tests: tests, examples, fuzzing, and benchmarking.

If we’re answering open questions on this proposal, can I get confirmation that we’re going to remove the default -timeout? My previous inquiries got no replies. The CL that’s out doesn’t look like it changes the default 10m -timeout but I really think it should.

Change https://go.dev/cl/445597 mentions this issue: internal/testenv: remove RunWithTimout

@rsc I guess I don’t have a problem with people implementing their own scaling, but it would be a shame for each project to reinvent a slightly different wheel with different standards on what the “base timeout scale” is. I am hoping we can at least get ahead of defining what the scale of the default timeout should be. If some projects use “enough even for the slowest machine”, and others use “matching a modern fast machine”, then adding built-in scaling at a later time won’t work with the mismatching baselines.