Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: testing: reconsider adding Context method to testing.T #36532

Open
rogpeppe opened this issue Jan 13, 2020 · 10 comments
Open

proposal: testing: reconsider adding Context method to testing.T #36532

rogpeppe opened this issue Jan 13, 2020 · 10 comments
Labels
Projects
Milestone

Comments

@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Jan 13, 2020

There have been various previous proposals (#16221 #18182 #18199 #18368) to add a Context method to testing.T. #16221 was even accepted and implemented but then reverted.

By my understanding, the principal argument against adding a Context method was that Context provides a way to tell things to shut down, but no way to wait for things to actually finish, so adding this functionality doesn't actually provide a good way to wait for tests to gracefully shut down, because they might fail and resources might continue to be used even after the testing package has marked the tests as successful.

For example @bcmills wrote:

A Context that is canceled when the test fails would be fine, but trying to use it in isolation to signal the return of the Test function seems like a mistake. Context cancellation is asynchronous, so nothing guarantees that the goroutines started as part of the test have actually returned when the test ends, which means they are likely to interfere with subsequent tests.

Similarly from @niemeyer here:

Such a flagging mechanism that by definition will only be useful when things will continue running in the background past the test completion does feel like a wart to me. We should be discouraging people from doing that in the first place, because arbitrary background logic delayed for arbitrary periods of time while other things are being tested is a common source of testing pain, breaking in curious and hard to reproduce ways.

However, @dsnet, who reverted the code, suggested that the decision wasn't necessarily final:

I am personally still in support adding t.Context in conjunction with a wait mechanism as this will counter the current detriment of adding t.Context only. However, I don't believe that we should rush on more API design and we can revisit this for Go 1.9.

I propose that the recently added T.Cleanup method can now provide the wait mechanism that's needed - the other half of Context.Done. We can define the semantics such that the the testing context is cancelled just before invoking the Cleanup-registered functions. That way, it's easy to both listen for the test being done, and also to wait for asynchronous operations to finish when it is.

By hooking into the Cleanup mechanism, we don't presuppose any particular kind of waiting - this can hook into whatever mechanism your infrastructure provides for graceful shutdown.

A possible API description:

// Context returns a context that's cancelled just before
// Cleanup-registered functions are called.
//
// This allows Cleanup-registered functions to wait for any resources
// that are listening on Context.Done before the test completes.
func (t *T) Context() context.Context

Example usage:

func TestFoo(t *testing.T) {
	ctx := t.Context()
	var wg sync.WaitGroup
	t.Cleanup(wg.Wait)
	wg.Add(1)
	go func() {
		defer wg.Done()
		doSomething(ctx)
	}()
}
@gopherbot gopherbot added this to the Proposal milestone Jan 13, 2020
@gopherbot gopherbot added the Proposal label Jan 13, 2020
@seh
Copy link
Contributor

@seh seh commented Jan 13, 2020

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

@rogpeppe
Copy link
Contributor Author

@rogpeppe rogpeppe commented Jan 13, 2020

@seh yup, fixed, thanks!

@ianlancetaylor ianlancetaylor changed the title proposal: reconsider adding Context method to testing.T proposal: testing: reconsider adding Context method to testing.T Jan 13, 2020
@rogpeppe
Copy link
Contributor Author

@rogpeppe rogpeppe commented Jan 17, 2020

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
}
@bcmills
Copy link
Member

@bcmills bcmills commented Jan 21, 2020

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
	})
}
@bcmills
Copy link
Member

@bcmills bcmills commented Jan 22, 2020

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.

@rogpeppe
Copy link
Contributor Author

@rogpeppe rogpeppe commented Jan 22, 2020

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?

@bcmills
Copy link
Member

@bcmills bcmills commented Jan 22, 2020

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.

@rogpeppe
Copy link
Contributor Author

@rogpeppe rogpeppe commented Jan 22, 2020

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?

@bcmills
Copy link
Member

@bcmills bcmills commented Jan 23, 2020

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.

@carnott-snap
Copy link

@carnott-snap carnott-snap commented Jul 28, 2020

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

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Jul 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Incoming
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.