Repository navigation
testing: add (*T).Deadline #28135
Description
Activity
Another option would be to add a t.Deadline and leave context creation up to the test.
t.Deadlinewould be somewhat more annoying to use, but acceptable for my use-cases.The way I see it, current testing is very simple and I like it that way. Adding a
t.DeadlineI 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 at.Deadline()since that is much simpler.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.
Reacted by Karol MarcjanSure, let's try adding t.Deadline for Go 1.13.
- changed the title
[-]proposal: testing: add (*T).Context, canceled only on timeout, failure, or skip[/-][+]proposal: testing: add (*T).Deadline[/+]on Dec 12, 2018 7 remaining items
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
testingpackage), 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-writtenTestFoo(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.
Reacted by Bryan C. MillsThe 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).Deadlineover(*T).Context.Reacted by Jehiah CzebotarChange https://golang.org/cl/202758 mentions this issue:
testing: provide (*T).Deadline to report when the test will have exceeded its timoutThis 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.
- locked and limited conversation to collaborators
on Feb 20, 2021
Background
#16221 added a
func (*T) Context() context.Contextto the testing package, which was backed out due to concerns from @dsnet, @neild, @niemeyer, me, and perhaps others as well (#18199).CL 134395 (
errgrouppropagation ofGoexit, 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 callst.FailNowort.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
testingpackage 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.timeoutflag, we need to be able to subtract off the amount of time spent so far, and that implies the use ofTestMainand the boilerplate that goes with it.Proposal
I propose that we add back the
(*testing.T).Contextmethod, with the same signature as before but the following semantics:Done, withErrreturningcontext.DeadlineExceeded.Contextmethod (and their subtests) have returned, whichever occurs first.FailedorSkipped:Done, withErrreturningcontext.Canceled.Done. That discourages the use of(*T).Contextfor normal cleanup of “background” goroutines, which should be accomplished by some other means (sync.WaitGroup,errgroup,tomb, or whatever alternative the test author prefers.)