assert: check early in Eventually, EventuallyWithT, and Never #1427
Conversation
|
This PR changes more than what it claims. Please keep it focused on #1424 and move the changes related to the tick separately. Because it seems that runs of the condition will not overlap anymore. |
cszczepaniak
force-pushed
the
cs/quicker_eventually
branch
from
d4ebae4 to
dc3af0f
Compare
|
I intend to merge #1395 first. Could you help with your own review now, and we will work on your own changes after the merge? |
dolmen
changed the title
dolmen
changed the title
assert/assertions_test.go
Outdated
| condition := func(t *CollectT) { <-time.After(time.Millisecond) } | ||
|
|
||
| done := make(chan struct{}) | ||
| go func() { | ||
| defer close(done) | ||
| True(t, EventuallyWithT(mockT, condition, 1000*time.Millisecond, 100*time.Millisecond)) | ||
| }() | ||
|
|
||
| select { | ||
| case <-done: | ||
| case <-time.After(10 * time.Millisecond): | ||
| Fail(t, `condition not satisfied quickly enough`) | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
assert/assertions_test.go
Outdated
| condition := func() bool { <-time.After(time.Millisecond); return true } | ||
|
|
||
| done := make(chan struct{}) | ||
| go func() { | ||
| defer close(done) | ||
| True(t, Eventually(mockT, condition, 1000*time.Millisecond, 100*time.Millisecond)) | ||
| }() | ||
|
|
||
| select { | ||
| case <-done: | ||
| case <-time.After(10 * time.Millisecond): | ||
| Fail(t, `condition not satisfied quickly enough`) | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
cszczepaniak
force-pushed
the
cs/quicker_eventually
branch
from
dc3af0f to
7ec44f8
Compare
| // Check that a long running condition doesn't block Eventually. | ||
| // See issue 805 (and its long tail of following issues) | ||
| func TestEventuallyTimeout(t *testing.T) { |
There was a problem hiding this comment.
This test isn't new; I just moved it so that all of the Eventually* tests are grouped together rather than having the Never* tests in between them
cszczepaniak
changed the title
|
@cszczepaniak I just merged #1481 which uses |
|
@dolmen sounds good. I don't think that affects this PR, though. |
|
@cszczepaniak Please rebase as this requires to resolve merge conflicts. |
cszczepaniak
force-pushed
the
cs/quicker_eventually
branch
from
82097f1 to
bae586f
Compare
|
@dolmen Thanks, I've rebased now. |
|
👋 Hey @dolmen @cszczepaniak, any update on this PR ? |
|
👋 Hey @dolmen, any chance you could give this PR another review ? |
There was a problem hiding this comment.
I think the code in this PR is perfectly fine and implements the feature cleanly. I especially like that it removes the rather implicit assignment of ticker.C to tick in the for statement.
@dolmen's concern that this removes any overlapping runs of the condition isn't an issue. The condition already cannot overlap because of tick being set to null before calling the condition function.
Because of the way tickers work this cannot alter the timing of when the last condition function call happens so long as the condition function function always has a shorter duration than tick. If the condition function can take longer than tick then this may alter the timing. In all cases where the duration of the condition function is unrelated to when it is called this change can add at most 1 additional call of the condition function. It is possible that poorly written code with race conditions may now panic, deadlock, or any number of other things that make tests fail.
It's unlikely that even very fast asynchronous code will satisfy the new first call to the condition function if Eventually is called soon after the asynchronous code starts. But this means it's more likely that more condition functions will be exercised in their failing state. I don't think this will speed up many tests at all.
I think these factors all stand to improve the condition of code tested by Testify, but I'm sure there will be tests which previously passed which now fail in poorly written code-bases. We will need to be ready to field support requests.
I'm going to leave my approve but give other maintainers a couple of weeks to consider what I've written here before merging.
Summary
Addresses #1424.
EventuallyandEventuallyWithTcurrent must always wait at least the polling duration before they can succeed. This PR starts checking the condition immediately. The assertion still fails if the initial check of the condition takes longer than the configured timeout.Changes
Motivation
This will provide a small optimization to callers, because tests can complete more quickly than they did before if conditions are met already.
Related issues