proposal: testing: add T.Context

[cstockton]

Both #16221, #18199 and the mailing list seems to be mostly bikeshedding in my opinion. I don't think a synchronization primitive needs to be added here, having the context is more than enough for every use case I can imagine. The context is needed to solve REAL problems with writing code, a synchronization primitive is a mere convenience that isn't useful for the problem imposed by testing. The problem is:

Write a function that must walk through pipelines of various highly concurrent systems (already use context.Context for signaling while crossing system boundaries, strengthening the need for this IMO) and may exit at any point of execution without a signal.

The only solution that makes sense to that problem is to add a signal, so lets do that please. If the change was only adjusted to cancel soon as Fatal was called, before goruntime.Goexit() it would be perfect. It removes a lot of the mental drain of writing complicated concurrent tests and allows a tighter integration between tests and your application code. As it stands now I have to make large amounts of test support structures to initialize subsystems in various ways as different tests scenarios require different subsystems to be initialized and propagate errors to the testing goroutine. This would be so much nicer if my subsystems could share tight testing integrations such as:

type SysOneTester struct {
  *testing.T
  pkg.SystemOne
}

func (..) Start(ctx context.Context) {
  if systemone.Condition() != ExpectedState { t.Fatal(`systemone has failed`) }
  // start this system, and it's dependencies
}
func TestSystemsWhenThisSystemsStateIs(t *testing.T) {
   s1, s2, s3 := SysOneTester{t, pkg.New()},SysTwoTester{t, pkg2.New()},SysFiveTester{t, pkg5.New()}
   ctx, g := errgroup.WithContext(t.Context())
   g.Go(s1)
   ...
   g.Go(s3)
   if err := g.Wait(); err != nil {
     t.Fatal(err)
   }
}

When you start multiplying having to do the above times many N, it's very difficult to integrate testing's behavior of immediately exiting when Fatal is called. It requires careful use to not leave dead goroutines running or have deadlocks all together. When I am writing unittests I am already having to think about the set of complicated systems I am testing, having to worry about writing a second application for my application to test it is counter productive.

[dsnet]

Your title is a little misleading since it isn't adding the same T.Context back in as before. I'm not exactly sure what you are proposing, but it is distinctly different in behavior from the prior implementation that was reverted.

I don't really understand your example code. What is Start a method on? When is it invoked? What is pkg? It seems s1 is of type struct, yet it is being passed into g.Go, which usually expects a function. What's going on? There are too many unexplained things going on here for a proper analysis of the benefits/detriments of the proposed change.

[cstockton]

The way it was implemented before was a bug, canceling the context after runtime.Goexit doesn't make sense for reasons I've already outlined in the other issue. As far as not understanding my proposal, what is not clear? Add back context. Fix the bug with cancellation.

Ignore the code, focus on the fact that the Context is being embraced as a way to cross API boundaries.
-> Testing crosses API boundaries.

Go advocates sharing by communicating.
-> runtime.Goexit doesn't communicate.

[cstockton]

I have no further information to provide so feel free to close this @dsnet if adding t.Context() back is not open for consideration. Thanks.

[dsnet]

t.Context (or its moral equivalent) will probably be included in Go 1.9. However, it's resurrection will probably not have the same semantics as what you are pushing for here, which is different from other how other people expect it to work.

[cstockton]

@dsnet Yea it's completely unreasonable of me to expect that when my caller terminates my currently executing goroutine that the context would be done. It's much simpler to reason about and no traps for developers to fall into while attempting to cleanup and exit in defers. Thanks.

[cstockton]

@dsnet what is the moral equivalent to Context for Go 1.9? I see testing/quick changes but nothing related to testing, if your more correct solution didn't make it this release what was the blocker? If time was the issue can you share your design proposal and maybe I can lend a hand so it makes it next release?

[ianlancetaylor]

I think this got dropped. Sorry. Thanks for reopening the issue.

[cstockton]

@ianlancetaylor No problem at all, I'll give dsnet time to let me know how he would like to proceed, thanks.

[dsnet]

IIUC, you are proposing to add a T.Context that returns a context that is canceled only if t.Fatal is called?

As it stands today, t.Fatal can only be called by the main Test goroutine. The documentation explicitly says that "FailNow must be called from the goroutine running the test or benchmark function, not from other goroutines created during the test." Given this restriction, the behavior you are suggesting is trivially implemented yourself with two lines:

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

    go helperTestFunction(ctx, t) // Context is canceled if the main goroutine fails
    t.FailNow() // Causes Test to return and cancels the context
}

Are you suggesting that we should relax the restriction on FailNow so that it can be called from other goroutines also?

If that were the case, I can understand why context becomes more useful because now you would need to plumb the cancel callback function to various goroutines that could also call FailNow.

[cstockton]

@dsnet Hello- I was asking what the solution that you were planning for 1.9 was in place of Context(). I thought perhaps you were proposing something like errgroup baked in which would be very nice, I use that library in most projects. I.E.:

T.Go(func() error)

// Maybe also useful depending on if T.Context() method is added as well.
//   - If T.Context() is not added I think a func with below signature would be
//     needed.
//   - If T.Context() is not added, than I think it's a lot more useful to have an
//     additional method of signature func(func(context.Context) error) live
//     on some combination of T.Go or T.GoContext identifiers.
T.GoContext(func(ctx context.Context) error)

// Wait will call Error with the trigger error if any of the goroutines
// started by Go return a non-nil error.
T.Wait() 

// WaitFatal is like Wait but calls Fatal instead of Error.
T.WaitFatal()

If that was canceled then I would say having Context() available would be nice, yes. It saves two lines in a large majority of tests I write since context is used so widely. I added that IF Context() was added- I felt you had less room to shoot yourself in the foot while writing tests if cancel was called directly before Goexit().

While I think having just a Context() is useful I agree that allowing concurrent calls to Fatal as well would make for a even better testing experience, would just need to define how concurrent failure affects future calls to methods on T.

I would be happy to implement any of the options here if the requirements could be settled on.

[leighmcculloch]

In the simple use case @dsnet included above #18368 (comment), where it's just a saving of two lines, the main benefit is not only the line saving but encouraging us Go developers to do the correct thing in our tests by providing a Context that cancels when the test is complete.

I might be a poor example, but I'm writing a tonne of tests using context.Background() for the first time and I overlooked that I should be canceling them if the test fails early.

However, I can see that this might border on test framework-like functionality that could start to pile up in the testing package.

[dsnet]

Providing only a mechanism for cancelation (i.e., "please stop") without a mechanism for completion (i.e., "did you stop?") is incomplete. You are not allowed to call testing.T.Error after the main Test function has returned. Cancelation without completion only encourages incorrect behavior.

[cstockton]

Providing only a mechanism for cancelation (i.e., "please stop") without a mechanism for completion (i.e., "did you stop?") is incomplete. You are not allowed to call testing.T.Error after the main Test function has returned. Cancelation without completion only encourages incorrect behavior.

I do believe it would be better to provide both, but I don't understand how it encourages incorrect behavior. People already have no choice but to start multiple goroutines in tests today, having to provide their own cancellation mechanisms for a rather unforgiving API (calls Go exit, concurrent safe on some methods). Providing a signal for when a test is done removes half of an entire problem space you mention above, leaving developers a more simple task of "did you stop?". Which is as simple as a WaitGroup and something everyone is familiar with.

@dsnet Since you continue to be the most vocal person against this feature since you reverted it 2 releases ago, stating you had a better solution in mind can you share that with me please? I provided a response above as food for thought, offered to do the work and that offer still stands. I'm not going to write a patch to have you block it though, so waiting for your feedback.

[neild]

What's the problem you're trying to solve?

A problem (maybe not the one you're trying to solve) which the above discussion suggests to me is that it's too easy to make mistakes when starting goroutines in tests. You aren't supposed to call T.Fatal from outside the main test goroutine, but nothing prevents you from doing so and it may even work. You aren't supposed to call T.* after the test function has returned, but it's quite easy to do so by accident.

Perhaps it's worth adding additional infrastructure to the testing package to avoid these problems. Maybe something like the following:

// Go starts a new goroutine. The test or benchmark will not
// complete until this goroutine returns.
func (TB) Go(f func())

// Context returns a context which is canceled after the
// TestXxx/BenchmarkXxx function returns or FailNow is called.
func (TB) Context() context.Context

// FailNow marks the function as having failed, cancels the
// function Context, and stops the execution of the current
// goroutine. Execution will continue at the next test or
// benchmark. Calling FailNow does not stop any other goroutines.
func (TB) FailNow()

It would be simple to provide this functionality in a separate package, which might argue against the need for including it in the testing package. On the other hand, the demonstrated ease of getting concurrency wrong in tests argues for it. I don't have a strong conclusion myself.

[dsnet]

I provided a response above as food for thought, offered to do the work and that offer still stands. I'm not going to write a patch to have you block it though, so waiting for your feedback.

I apologize, I did not see your response. I was responding to the comment immediately prior to mine.

Both yours and the one that @neild just posted look very similar. The API that Damien proposed is one that I proposed some time ago (not on this thread... there have been too many on this) and the one that I support.

[dsnet]

I'm marking this as a proposal since new API is being added (or altered). As Damien mentioned, it's also possible to experiment with those by wrapping *testing.T.

I doubt this will happen in Go1.10 since the freeze has happened. I apologize that nothing happened this cycle.

[cstockton]

@dsnet Completely fine I know you all are busy, if you get a chance could you give me the proposal issue number or just the username of the proposal? I've been searching issues for a bit now but it's not turning up, if you can't find it quickly don't bother- I will start with What @neild and I posted. Thanks.

[neild]

Coincidentally, I am right now looking at a test which would have benefited from this. General summary:

func TestStreamAbort(t *testing.T) {
  stream := newStream()
  defer stream.Close()
  go func() {
    for i := 0; i < 100; i++ {
      // The 50th message will abort the stream.
      if err := stream.Send(Message{Abort: i == 50}); err != nil {
        t.Fatal(err)
      }
    }
  }()
  // Read messages from the stream, t.Fatal if the stream doesn't close after the 50th.
}

Lots of problems: t.Fatal in a child goroutine, the child goroutine might outlive the test, t.Fatal might be called after the test completes. This currently works enough of the time for nobody to notice it (although I bet the test is flaky).

Changing the go to T.Go as described above would make this test correct with no other changes.

[dsnet]

#18022 was a proposal for adding WaitGroup.Go(), where it would take in an argument-free and return-free function, but there was push-back against that it only allows func():

As far as the proposal goes, I don't like how it limits the func signature to func().

Personally, I'm not bothered by that, since I expected people to write closured functions anyways. Thus, I like the simplicity of: func (TB) Go(f func())

[rsc]

My objection to wg.Go in #18022 (comment) applies even more to t.Go. This is not a testing-specific problem (making sure goroutines are done before returning from a function). It's not obvious why it merits a testing-specific solution. Let's gather more information about the general problem and look for general solutions, maybe with an eye toward Go 2.

The original comment above assumes some little-c context that I am missing today. It's unclear exactly what the problem to solve is. If the problem is making sure goroutines exit, then the above paragraph applies. If it's something else, then probably we should open a different issue with a clear problem statement.