Replace Never & Eventually functions with synchronous versions#1657
Replace Never & Eventually functions with synchronous versions#1657brackendawson wants to merge 3 commits into
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?
|
|
||
| <-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!
|
The issues described recently in #1396 lead me to believe the collect object and the condition function need more work to support the test being failed immediately from the condition function. Either of these options need to be used:
|
|
Do we still need this now #1427 was merged? I'm simply reviewing issues linked to this recently merged PR, and I wonder if this one is still needed. |
|
In the case where the condition function does not return before the time, Eventually can still leak a goroutine after the end of the test. Making execution of the condition function synchronous fixes this. But this PR needs some work. |
|
Got it, thanks Could you rebase and fix the conflicts? |
dolmen
added
pkg-assert
## What changed? Introduces `common/testing/await`, a polling-based test helper that replaces testify's `Eventually` / `EventuallyWithT`. ## Why? From the [package doc](https://github.com/temporalio/temporal/blob/stephanos/all-require/common/testing/await/await.go): ``` Improvements over testify's eventually functions: - Misuse detection: accidentally using the real *testing.T (e.g. s.T() or suite assertion methods) instead of the callback's collect T is a common mistake. This package detects it and fails with a clear message. - Safer bool predicates: unlike testify's Eventually, [RequireTrue] only accepts func() bool, so returning false is the sole retry signal. If the predicate accidentally marks the real test failed, it reports that immediately instead of polling until timeout. - Timeout-aware callbacks: callbacks receive a context derived from the parent context and canceled when the await timeout or test deadline is reached, so RPCs and blocking waits can exit instead of continuing after the retry window has expired. - Panic propagation: if the condition panics (e.g. nil dereference), the panic is propagated immediately rather than being silently swallowed or retried until timeout. See https:github.com/stretchr/testify/issues/1810 - Bounded goroutine lifetime: each attempt completes before the next starts, avoiding the overlapping-attempt data races and "panic: Fail in goroutine after Test has completed" crashes seen with testify's Eventually. See https:github.com/stretchr/testify/issues/1611 - Deadlock detection: a condition that ignores t.Context() is abandoned after a grace period, producing a clear "does it honor t.Context()?" failure instead of hanging until go test -timeout. - Condition always runs: testify's Eventually can fail without ever running the condition due to a timer/ticker race with short timeouts. This package runs the condition immediately on the first iteration. See stretchr/testify#1652 ``` The upstream fix ([testify#1657](stretchr/testify#1657)) aims to address several of these but has been open since Oct 2024 without merging. ## How did you test it? - [ ] built - [ ] run locally and tested manually - [x] covered by existing tests - [x] added new unit test(s) - [ ] added new functional test(s) ## Potential risks The biggest downside is that it adds more code that we own. But apart from any unknown bugs, this package should rarely/never change.
4 participants
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 Never: unexpectedly succeeds if timeout reached before condition returns #1654.Neverfor the same reasons. Implementing it asConsistentlyallows it to useCollectTtoo and delivers Feature: assert.Consistently #1087.conditionimmediately. This solves the timing race (assert.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