Replace Never & Eventually functions with synchronous versions #1657
Conversation
And if it does flake, make it do so quickly rather than deadlocking.
| collect := new(CollectT) | ||
|
|
||
| wait := make(chan struct{}) | ||
| go func() { |
There was a problem hiding this comment.
What does this goroutine gain us? We're waiting on it synchronously immediately after, so it's equivalent to just calling the condition inline here (without a goroutine)
There was a problem hiding this comment.
CollectT.FailNow will call runtime.GoExit. If we didn't use a new goroutine for each callback then the user calling FailNow would stop the whole test. This is the only reason we can't do this without any additional goroutines at all.
There was a problem hiding this comment.
It looks like @dolmen doesn't like CollectT, and I can't say I disagree with him.
So we actually could make this work with no additional goroutines. The choice is to either use func() bool or use an interface with an un-exported implementation. I find that every time I use Eventually I want to use assertions inside the condition function, so the latter is my preference. But we can panic an un-exported value rather than calling runtime.Goexit. All panics from the condition function should be handled regardless.
There was a problem hiding this comment.
I'm not sure I totally follow. It sounds like an unexported implementation makes sense, but could you sketch out what that code would look like?
assert/assertions.go
Outdated
|
|
||
| <-ticker | ||
|
|
||
| if time.Now().After(deadline) { |
There was a problem hiding this comment.
I wonder if we should still support failing the test immediately when deadline elapses, even if we're in the middle of calling the condition. We'd effectively just keep the goroutine above that I commented on and select over the done channel + the timeout channel. Maybe this is what was meant by leaking a goroutine though.
If we don't do that, I question the value of the API accepting a duration as a timeout because the duration isn't super closely tied with the maximum time this function might take. Maybe it would be better to just accept a maximum number of tries instead.
There was a problem hiding this comment.
I wonder if we should still support failing the test immediately when
deadlineelapses, even if we're in the middle of calling the condition. We'd effectively just keep the goroutine above that I commented on andselectover the done channel + the timeout channel. Maybe this is what was meant by leaking a goroutine though.
This makes the callback asynchronous (sometimes) and puts us back in the situation of having to allow it to potentially leak. It was always asking for trouble. I tried a similar fix in #1653 but I don't like it.
If we don't do that, I question the value of the API accepting a duration as a timeout because the duration isn't super closely tied with the maximum time this function might take. Maybe it would be better to just accept a maximum number of tries instead.
This is an excellent point. I hadn't considered that synchronous behaviour would suit a different function signature better. times int is far more natural than waitFor time.Duration. I've updated the PR.
Thanks for the review!
Summary
Eventually,EventuallyWithT, andNeverhave serious defects which require changes to their behaviour to rectify:Making them syncronous fixes every known problem with these functions.
Changes
EventuallyWithTcalledEventuallySync. Making the call toconditionsyncrhonous means we no longer have to decide what to do (or what not to do) when an asynchronous call to the user's condition function doesn't return before the end of the test. This solves the goroutine leak (assert: Eventually should not leak a goroutine #1611) but also forces the definition of a new function as there could be many badly written tests which would now deadlock if this was implimented in the existing functions. This also means evaluation of the test result happens between calls to theconditionfunction, making the assertion easier to use for the user and solving If condition passed to Never does not return before waitFor, then the Never assertion passes. #1654.Neverfor the same reasons. Implementing it asConsistentlyallows it to useCollectTtoo and delivers Feature: assert.Consistently #1087.conditionimmediately. This solves the timing race (Eventually can fail having never run condition #1652) where the select statement could be reached for the first time with both tick and waitFor channels ready, where the function would randomly return a verdict without actually testing the condition. This also delivers a popular request: Succeed quickly inEventually,EventuallyWithT#1424EventuallyandEventuallyWithTas deprecated with warnings about their faults and suggest the use ofEventuallySyncinstead.Neveras deprecated with warnings about its faults and suggest the use ofConsistentlyinstead.TestEventuallyTimeoutless likelt to fail, or fail more quickly.Motivation
Related issues
Eventually,EventuallyWithT#1424