Did you intend for your sample TestFoo function to include a call to WaitGroup.Done?

@seh yup, fixed, thanks!

I'm having second thoughts about this proposal. Given that it's so simple to define your own test-context function, does T.Context actually justify its place?

I believe the function below does almost exactly the same thing as the T.Context method proposed above (the difference being that it returns a new context value every time, but that shouldn't make any significant semantic difference beyond extra memory use):

func testContext(t *testing.T) context.Context {
	ctx, cancel := context.WithCancel(context.Background())
	t.Cleanup(cancel)
	return ctx
}

I think the changes to golang.org/x/sync/errgroup proposed in CL 134395 are probably cleaner, and wouldn't require any changes to the standard-library testing package.

In particular, that change would allow you to do something like:

func TestFoo(t *testing.T) {
	g, ctx := errgroup.New(context.Background())
	t.Cleanup(g.Stop)
	g.Go(func() error {
		defer wg.Done()
		doSomething(ctx)
		return nil
	})
}

Thinking about this some more: I think a (*T).Context method would still be too confusing.

Some users would likely assume that it is (only) cancelled if and when the test is marked as failed. Others might assume that it is cancelled at start of the Cleanup phase, before the cleanup functions are invoked. Yet others might assume that it is cancelled after all of the Cleanup functions have completed.

Since there is no single “intuitive” interpretation, we should avoid the ambiguous name. Perhaps we could find a less ambiguous name, but given that the explicit code isn't much longer I'm not sure that's worth the API weight.

After some discussion with @mvdan, I realised that my original proposal here was not sufficient for his use case (he wants to start tearing things down immediately there's a test error).

I don't agree with tearing everything down on any call to Fail, but ISTM that there's an alternative: provide a context that's canceled whenever FailNow is called and we document that it's OK to call FailNow in a goroutine.

Calling FailNow in a goroutine still won't tear down the entire test (it can't, of course) but at least then there's a mechanism whereby that can happen. And people really like calling FailNow in goroutines, and there's no really reason AFAICS why it needs to be disallowed. We could even document that it calls runtime.GoExit under the hood.

wdyt?

The approach in that errgroup CL still seems more general and a bit cleaner to me.

We don't need to couple cancellation to FailNow; we only need to have a mechanism to tie the call to runtime.Goexit to the cancellation of a corresponding Context, and tying goroutines to higher-level tasks is literally the only purpose of errgroup.Group, so that seems like a natural place for it.

Note that one of the changes in that errgroup CL specifically adds handling and propagation for runtime.Goexit: a runtime.Goexit in any of the associated goroutines cancels the associated Context, and also propagates to any pending or subsequent Wait calls. So one of the effects of that CL is to enable errgroup to tear down (concurrent portions of) a test in the natural way on failure.

I like that thought. The only issue is that you'd have to be careful to add the corresponding code to every goroutine, I guess. That might turn out to be a pain (and it's error-prone).

We'd still need to drop the "FailNow must be called from the goroutine running the test or benchmark function, not from other goroutines created during the test." sentence from the docs. Does that seem reasonable to you?

We'd still need to drop the "FailNow must be called from the goroutine running the test or benchmark function, not from other goroutines created during the test." sentence from the docs. Does that seem reasonable to you?

That seems reasonable. I would probably replace that sentence with a more explicit one, rather than dropping it outright. Perhaps something like:

FailNow marks the function as having failed and stops its execution the execution of the current goroutine by calling runtime.Goexit (which then runs all of its deferred calls in the current goroutine). Execution will continue at the next test or benchmark. FailNow must be called from the goroutine running the test or benchmark function, not from other goroutines created during the test. Calling FailNow does not stop those other goroutines. FailNow stops only the calling goroutine, not other goroutines associated with or created during the test.

And in the comment for type T:

A test ends when its Test function returns  or calls any of the methods FailNow, Fatal, Fatalf, SkipNow, Skip, or Skipf. Those methods, as well as the Parallel method, must be called only from the goroutine running the Test function. or its goroutine terminates due to a panic or a call to runtime.Goexit. The methods FailNow, Fatal, Fatalf, SkipNow, Skip, and Skipf terminate the calling goroutine, so a call to any of those methods from the goroutine running the Test function ends the test.

And for (*T).Parallel:

Parallel signals that this test is to be run in parallel with (and only with) other parallel tests. When a test is run multiple times due to use of -test.count or -test.cpu, multiple instances of a single test never run in parallel with each other. To ensure correct scheduling, all goroutines associated with the test must block until Parallel returns.

I am interested in continuing this discussion, can we add this to the proposal rotation, or was there something blocking the discussion?

What I've done for couple of projects was to introduce the following function:

const (
  // Arbitrary amount of time to let tests exit cleanly before main process terminates.
  timeoutGracePeriod = 10 * time.Second
)

// contextWithDeadline returns context with will timeout before t.Deadline().
func contextWithDeadline(t *testing.T) context.Context {
  t.Helper()

  deadline, ok := t.Deadline()
  if !ok {
    return context.Background()
  }

  ctx, cancel := context.WithDeadline(context.Background(), deadline.Truncate(timeoutGracePeriod))

  t.Cleanup(cancel)

  return ctx
}

This allows to properly respect -timeout flag in go test. Having a function like this in standard library would be indeed useful, perhaps in a form of:

gracePeriod := time.Second
ctx := t.ContextWithDeadline(gracePeriod)

We could also validate that gracePeriod > now - deadline or alternatively use context.WithTimeout.

I am strongly in favor of adding this.

It seems like every single test that I write begins with:

func TestXxx(t *testing.T) {
	ctx, cancel := context.WithCancel(context.Background())
	defer cancel()

I have also found that tests which use contexts and don't begin with that preamble usually leak resources or contain other context related bugs. I think making this functionality built-in would help encourage better testing.