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

testing: T.Context usability concerns #18199

Closed
quentinmit opened this issue Dec 5, 2016 · 15 comments

Comments

Projects
None yet
8 participants
@quentinmit
Copy link
Contributor

commented Dec 5, 2016

https://groups.google.com/d/topic/golang-dev/rS6psxIf17Q/discussion

Decide on whether to keep/change/remove the API before the 1.8 release.

@quentinmit quentinmit added this to the Go1.8 milestone Dec 5, 2016

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 5, 2016

Issue #18182 was another person asking for this last night.

Their motivation: #18182 (comment)

@dsnet

This comment has been minimized.

Copy link
Member

commented Dec 5, 2016

#16221 is the proposal that added T.Context

@niemeyer

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2016

Copying here the point made in golang-dev, for clarity:

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.

As people pointed out, it's not a mere case of putting logic locally. The context, by definition, will only trigger code to start aborting after it's too late. An aborting yet running goroutine can do whatever it wants in order to attempt to stop cleanly.

In my own testing, services generally have a Stop method because of that. Internally, they leverage a tomb to flag the stopping and block until everything is done cleaning themselves up. Stop also usually returns an error, so one can also check that the stop was itself clean.

@dsnet

This comment has been minimized.

Copy link
Member

commented Dec 5, 2016

Summary of my thoughts.

We should either:

  • Add both (a mechanism to signal for termination AND a mechanism to wait for all to be terminated)
  • Not add anything new at all

I understand most uses of Context don't care about waiting for termination, but I believe it is important enough in a significant number of testing purposes.

Some arguments against the status quo (i.e., only adding Context):

  • The same argument for adding Context for convenience can also be made for adding some wait mechanism. Adding Context saves 2 lines of code. Adding some mechanism for waiting saves another 2 lines. It's a slippery slope. Where do we stop?
  • Adding Context alone makes it more confusing how to add your own wait mechanism since there is no way to insert a post-cancelation wait. That is, the testing framework will only call cancel when the Test function returns, but using your own WaitGroup.Wait will prevent the Test function from even returning.
  • You could resolve the above issue by just not using T.Context and roll with your own Context and WaitGroup, but now you have to deal with remembering whether you should be using your own ctx variable that you pass around or T.Context.
  • We already have a problem with many tests calling T's method when the Test function returns. The lack of a built-in waiting mechanism encourages people to do this incorrectly. Imagine the following:
    go func() {
        select {
        ...
        case <-t.Context().Done(): // Closed when Test returns
            if err := something.Close(); err != nil {
                t.Errorf("unexpected Close error: %v", err) // This is problematic!
            }
        }
    }()

If we go with adding a wait mechanism, I don't feel strongly whether it's by exposing a WaitGroup, or by adding a Go method as @bcmills suggested in #16221, or something else.

@bcmills

This comment has been minimized.

Copy link
Member

commented Dec 5, 2016

There is one benefit of adding Context that does not apply to a mechanism for waiting for cleanup: it is difficult to construct a Context which is canceled when the test has failed but the test function is still running (e.g. when t.Failed() transitions to true), but it's fairly easy to defer func() { cancel(); wg.Wait() }().

So a minimal fix might be a Context which is canceled only on failure (and which otherwise explicitly leaks goroutines), or perhaps a Context which is canceled on failure and explicitly panics if its methods are called after the test function has returned. That would address the "finding out when the test has failed" case but leave the "cleaning up the test" boilerplate in user code (where it is today).

@dsnet

This comment has been minimized.

Copy link
Member

commented Dec 5, 2016

I think canceling the Context upon first failure is inconsistent with how tests work. When t.Error is called, it is normal to continue moving on and perform other tests. If the first t.Error caused the context to be canceled, then that would affect the results of following tests within the same Test function. I'm okay with cancelation occuring when t.Fatal is called, but that is included in the current behavior.

The fact that people have differing expectations of when T.Context gets canceled is an argument against adding it. At least with rolling with your own context, the expectations are clear. So far there have been at least 3 expressed ways people expect it gets canceled:

  • (current behavior) Only when the Test ends (either the Test returns or t.Fatal is called)
  • Only when the Test fails
  • Only when the Test times out (however that is defined)
  • Some combination of the above...
@bcmills

This comment has been minimized.

Copy link
Member

commented Dec 5, 2016

When t.Error is called, it is normal to continue moving on and perform other tests.

Good point. Still, especially given the ability to create subtests I think there are a number of situations in which "cancel on first failure" really is what you want. But perhaps that argues for something more like errgroup, combining "cancel on first failure", "ignore subsequent errors", and "wait for cleanup" into a single API.

@dsnet

This comment has been minimized.

Copy link
Member

commented Dec 6, 2016

Another data point. There are use-cases where we want to add graceful timeout to tests. The current implementation of t.Context provides no way for the user to manually cancel the context other than returning from the Test function.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 6, 2016

If you want a timeout, you can make a derived context from t.Context(). Again, this isn't supposed to be the end-all Context for all imaginable use cases. It's supposed to be a convenience thing.

And having a context available would've helped a CL I reviewed yesterday (https://go-review.googlesource.com/c/33972/1/http2/transport_test.go) if the context were available. But it was a few lines of code without t.Context, so we survived.

But obvious you dislike it, so feel free to send a CL removing it and the documentation from go1.8.html.

Perfect is the enemy of good.

@dsnet

This comment has been minimized.

Copy link
Member

commented Dec 8, 2016

I'm uploading a CL to revert this change. There are a fair number of people who have reservations about this.

The main argument for adding t.Context seems to be for convenience (saving ~2 lines) and for consistency with the rest of standard library (which is moving towards adding Context where it makes sense). However, the benefit of convenience and consistency is offset by increased risk of users calling t.Error after the Test has returned. Overall, I don't believe a 2-line convenience is worth the real risk of improper t.Error usage (which is already a fairly common problem in tests).

To be fair, I originally voted in favor of this change in #16221, but changed my position on this after trying to use it on several unit tests.

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.

@gopherbot

This comment has been minimized.

Copy link

commented Dec 8, 2016

CL https://golang.org/cl/34141 mentions this issue.

@rogpeppe

This comment has been minimized.

Copy link
Contributor

commented Dec 8, 2016

One possibility might be to add:

// Go starts a new goroutine running f. The passed context is canceled
// when the test completes. The next test will not be run until
// f returns - the test will fail if f takes too long to return after
// the test has completed.
func (t *T) Go(f func(context.Context))

That way the test can at least wait for the goroutine to finish
before ending the test.

@gopherbot gopherbot closed this in 4bf7d1e Dec 9, 2016

@gopherbot

This comment has been minimized.

Copy link

commented Dec 9, 2016

CL https://golang.org/cl/34242 mentions this issue.

gopherbot pushed a commit that referenced this issue Dec 9, 2016

api: remove testing Context accessors from go1.8.txt
Fixes the build.

Updates #18199

Change-Id: Ibf029ba9f9293d1f3d49c1c8773fc262159a5d5b
Reviewed-on: https://go-review.googlesource.com/34242
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot

This comment has been minimized.

Copy link

commented Dec 10, 2016

CL https://golang.org/cl/34274 mentions this issue.

gopherbot pushed a commit to golang/tools that referenced this issue Dec 10, 2016

go/loader: fix broken tests after context removal from testing package
Updates golang/go#11811
Updates golang/go#18199

Change-Id: I2ce4615653034563a64b9c126651d2a6ce2aef50
Reviewed-on: https://go-review.googlesource.com/34274
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
@glasser

This comment has been minimized.

Copy link
Contributor

commented Dec 16, 2016

If the idea of T.Context() is ever revisited, I'd suggest considering also including the test name with WithValue. This would mean that if you're using a logging mechanism other than T.Log with parallel tests (and test -v), you'd have an easy way of labeling logs with the test name.

@golang golang locked and limited conversation to collaborators Dec 16, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.