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: add (*T).Deadline #28135

Closed
bcmills opened this issue Oct 10, 2018 · 13 comments
Closed

testing: add (*T).Deadline #28135

bcmills opened this issue Oct 10, 2018 · 13 comments
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted
Milestone

Comments

@bcmills
Copy link
Contributor

bcmills commented Oct 10, 2018

Background

#16221 added a func (*T) Context() context.Context to the testing package, which was backed out due to concerns from @dsnet, @neild, @niemeyer, me, and perhaps others as well (#18199).

CL 134395 (errgroup propagation of Goexit, among other things) mitigates those concerns somewhat: it gives test authors a straightforward way to start a collection of goroutines and wait for them to finish, and cancels them if any of the goroutines calls t.FailNow or t.SkipNow (#15758 notwithstanding).

That leaves one use-case unaddressed: as @neild noted in the golang-dev thread and @dsnet noted in #18199 (comment), if a test times out (e.g. because of a deadlock in the package under test), it is often useful to shut it down cleanly and emit useful logs rather than wait for the testing package to dump all of the running goroutines.

@bradfitz suggested that test authors can add their own derived context, but a test in general has no idea how much time it has left: if we want to check the -test.timeout flag, we need to be able to subtract off the amount of time spent so far, and that implies the use of TestMain and the boilerplate that goes with it.


Proposal

I propose that we add back the (*testing.T).Context method, with the same signature as before but the following semantics:

  • When a test binary times out:
    • First, all tests that are still running are marked as having failed due to timing out.
    • Then, the contexts provided to tests are marked as Done, with Err returning context.DeadlineExceeded.
    • Finally, the test binary waits for some “reasonable” (but unspecified) grace period elapses, or until all tests that called their Context method (and their subtests) have returned, whichever occurs first.
    • If any tests are still running at that point, the test binary writes out the logs for any completed tests and then panics as before.
  • When a test is marked as Failed or Skipped:
    • The context provided to that test (if any) is marked as Done, with Err returning context.Canceled.
  • When a test function exits normally:
    • The context provided to that test is not marked as Done. That discourages the use of (*T).Context for normal cleanup of “background” goroutines, which should be accomplished by some other means (sync.WaitGroup, errgroup, tomb, or whatever alternative the test author prefers.)
@bcmills bcmills added Proposal NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Oct 10, 2018
@bcmills bcmills added this to the Proposal milestone Oct 10, 2018
@neild
Copy link
Contributor

neild commented Oct 10, 2018

Another option would be to add a t.Deadline and leave context creation up to the test.

@bcmills
Copy link
Contributor Author

bcmills commented Oct 10, 2018

t.Deadline would be somewhat more annoying to use, but acceptable for my use-cases.

@deanveloper
Copy link

The way I see it, current testing is very simple and I like it that way. Adding a t.Deadline I think is a good idea, however. But contexts are big constructs that can make things complicated quick, especially when looking at other people's code. Probably better to just have a t.Deadline() since that is much simpler.

@rsc
Copy link
Contributor

rsc commented Oct 17, 2018

The last attempt at adding Context was trying to signal "this test is done, all you helpers can go away".
The new attempt is trying to signal - using exactly the same mechanism! - "this test is about to be killed, you might want to die with a more useful message".

I don't see why we should prefer one over the other. They both seem very indirect. The fact that there were these two very different plausible uses suggests that having plain t.Context will be confusing to people expecting one or the other (or a third equally plausible meaning).

If the goal is to expose timeout information, let's focus on an API that does that well, instead of shoehorning it into context.

@andybons
Copy link
Member

@rsc are you OK with t.Deadline?

Per @golang/proposal-review

@Capstan
Copy link

Capstan commented Dec 11, 2018

This appears to be related, if not a resurrection of #18368 which was FrozenDueToAge. I'm adding this comment here to act as a trail of breadcrumbs, hoping that the link will show up in the older proposal.

@rsc
Copy link
Contributor

rsc commented Dec 12, 2018

Sure, let's try adding t.Deadline for Go 1.13.

@rsc rsc changed the title proposal: testing: add (*T).Context, canceled only on timeout, failure, or skip proposal: testing: add (*T).Deadline Dec 12, 2018
@rsc rsc modified the milestones: Proposal, Go1.13 Dec 12, 2018
@rsc rsc added NeedsFix The path to resolution is known, but the work has not been done. early-in-cycle A change that should be done early in the 3 month dev cycle. labels Dec 12, 2018
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 12, 2018
@ianlancetaylor ianlancetaylor changed the title proposal: testing: add (*T).Deadline testing: add (*T).Deadline Dec 12, 2018
@nhooyr
Copy link
Contributor

nhooyr commented Feb 6, 2019

The last attempt at adding Context was trying to signal "this test is done, all you helpers can go away".
The new attempt is trying to signal - using exactly the same mechanism! - "this test is about to be killed, you might want to die with a more useful message".

I don't see why we should prefer one over the other. They both seem very indirect. The fact that there were these two very different plausible uses suggests that having plain t.Context will be confusing to people expecting one or the other (or a third equally plausible meaning).

We should prefer the second over the first because t.Error/t.Log cannot be called after the test finishes so it makes more sense that the context being cancelled doesn't mean the test is done, but that it needs to finish. In this respect, the first option encourages incorrect behaviour where background goroutines may use t.Error/t.Log after the test has completed as @bcmills noted and I don't think there is a good third plausible meaning.

If the goal is to expose timeout information, let's focus on an API that does that well, instead of shoehorning it into context.

Using context.Context makes cancellation across Go very consistent which is really nice and easy. You can pass the context to any function that may take a while and you know it will try to exit once the context is cancelled. You can also just select on a Done() channel versus creating this channel again with time.After.

Using context.Context also enables the use of the value map within context.Context which could be very useful.

It also fits in really well with the Stdlib's adoption of context.Context. As a user I'd be very surprised to see most of the stdlib using context.Context to express deadlines and cancellation and then see a Deadline method on *testing.T.

@nhooyr
Copy link
Contributor

nhooyr commented Mar 5, 2019

@rsc

In #18368 (comment) you closed the issue because

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.

It isn't a testing specific problem yes, but because of t.Fatal and t.FailNow are so easy to abuse when dealing with multiple goroutines, it merits a testing specific solution.

Related #24680 and #15758

I guess if #15758 is done, then a testing specific solution doesn't matter.

@dmitshur
Copy link
Contributor

dmitshur commented Oct 7, 2019

I want to share a potentially relevant experience report.

I found context deadlines/timeouts are a great fit for strict cancellation purposes, but it's trickier to use them for "putting a bound on time to compute some result".

Specifically, when there's only one explicit time provided, it's not very obvious whether the "grace period" should be on the caller or callee side.

When it's on the caller side (i.e., the testing package), the problem is the caller doesn't have context to know what is "reasonable time" to wait, so it can only pick a very generic amount of time. At the same time, the callee (i.e., a user-written TestFoo(t *testing.T)) doesn't know what the unspecified grace period of caller is, so to be safe, it may need to add a grace period buffer on its own side too. At that point, both callee and caller are sharing the responsibility of figuring out the total grace period time, which is suboptimal.

As a result, I found it worked better to make it completely a responsibility of the callee to finish and return before the deadline. The downside of this is that it complicates all callee code.

@bcmills
Copy link
Contributor Author

bcmills commented Oct 7, 2019

The need for a grace period — and the fact that the appropriate duration for it depends on the details of the test — seems like a great reason to favor (*T).Deadline over (*T).Context.

@bcmills bcmills modified the milestones: Backlog, Go1.14 Oct 23, 2019
@gopherbot
Copy link

Change https://golang.org/cl/202758 mentions this issue: testing: provide (*T).Deadline to report when the test will have exceeded its timout

@dmitshur
Copy link
Contributor

This issue is currently labeled as early-in-cycle for Go 1.15. That time is now, so friendly ping. If it no longer needs to be done early in cycle, that label can be removed.

@golang golang locked and limited conversation to collaborators Feb 20, 2021
@rsc rsc unassigned bcmills Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted
Projects
None yet
Development

No branches or pull requests

9 participants