Helper for limiting a test's duration?

[glenjamin]

I've run across a few cases recently where I've wanted to set a deadline on a test running. Generally when doing potentially complicated things that could deadlock, and as mistake could accidentally leave the test hanging.

Would you be interesting in accepting a new function/package for this functionality?

There's some related disucssion in https://github.com/golang/go/issues/10529 where it was deemed not suitable for core.

[glenjamin]

There's some subtleties I missed in this the first time around, but here's what I've ended up with

I'm on the fence a bit about whether it makes sense to include a context. I could imagine cases where you wouldn't want one, and also cases where you might want a custom one with extra stuff injected.

func testWithTimeout(t *testing.T, timeout time.Duration, f func(ctx context.Context, t *testing.T)) {
	ctx, cancel := context.WithTimeout(context.Background(), timeout)
	defer cancel()
	finished := make(chan bool)
	go t.Run("withTimeout", func(t *testing.T) {
		defer cancel()
		f(ctx, t)
		finished <- true
	})
	select {
	case <-finished:
	case <-ctx.Done():
		if errors.Is(ctx.Err(), context.DeadlineExceeded) {
			t.Fatalf("test timed out after %s", timeout)
		}
	}
}

[dnephin]

Hey Glen! Thanks for opening this issue, I think it's an interesting idea.

I've worked with a few tests that have this problem. Often I see them either use a context with a timeout directly to limit runtime, or use a select, similar to the one in your example, with a time.Timer to set the deadline.

I think my main concern with adding something like this, for code that doesn't itself support cancellation, would be that it actually leaves the goroutine running after the timeout. When all the code is inline in the test that's a bit easier to see. I wonder if a helper like this should actually panic to stop the tests from running (similar to how the -test.timeout flag works). That way you don't end up with a bunch of leaked goroutines still running in the background as other tests run.

Is it safe to call t.FailNow in f ? Generally t.FailNow can only be called from the main test goroutine (because it uses https://pkg.go.dev/runtime#Goexit and that only stops the current goroutine). But I'm not sure what the rules are if t.Run itself is called in a separate goroutine. At the very least, any calls to t.Log (and t.Error, etc) from f would panic if the goroutine outlives the test function.

I would probably want to avoid passing in a t to f and maybe instead have it return an error, although I imagine that's a very different pattern. That's more about limiting the time of a specific operation, rather than the test as a whole. In some ways this is similar to the design of gotest.tools/v3/poll.WaitOn. Because it doesn't use testing.T it encourages only retrying a very specific operation. Recently I've seen a lot of code that uses testutil/retry, which is essentially the exact same thing as poll.WaitOn, but because it accepts a testing.T it is much easier for someone to wrap a large block of code with a retry. I've found that retries around larger blocks of code end up being harder to debug when they fail.

In short, I like the idea, but I'm not sure exactly what the right interface or behaviour should be for a generic helper.

[glenjamin]

Hello! Yeah, I'm on the same page as you here - the need is there but I'm not quite sure about the exact approach.

Is it safe to call t.FailNow in f ?

Yep, this was what led me to run the function as a sub-test, without that FaiNow and co don't work.

I think my main concern with adding something like this, for code that doesn't itself support cancellation, would be that it actually leaves the goroutine running after the timeout.

Good point, the test in question had non-timeout channel reads in the test body, so had this exact issue.

It worked out ok for me since the test would either succeed in time, or block forever - so wrapping the whole test in this helper was preferable to cluttering the test body.

I suspect this helper would be less useful as protection against a test which takes a bit too long, but still completes - I'd have to test that flow a bit more

[dnephin]

It looks like this proposal https://github.com/golang/go/issues/48157 was accepted. So maybe -testtimeout will be making it into the stdlib in the next release or two.