testify icon indicating copy to clipboard operation
testify copied to clipboard

assert: check early in Eventually, EventuallyWithT, and Never

Open cszczepaniak opened this issue 1 year ago • 9 comments

Summary

Addresses #1424.

Eventually and EventuallyWithT current 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

cszczepaniak avatar Jul 15 '23 18:07 cszczepaniak

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.

dolmen avatar Jul 25 '23 01:07 dolmen

@dolmen What more does it change? The changes I made to the tick here are to facilitate addressing #1424. How would you do it differently?

cszczepaniak avatar Jul 25 '23 01:07 cszczepaniak

#1439 could also address this issue. @dolmen what are your thoughts on this PR's approach vs. addressing #1439 instead?

cszczepaniak avatar Jul 30 '23 17:07 cszczepaniak

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 avatar Jul 30 '23 22:07 dolmen

@dolmen After #1395, I think we should pursue either the approach in this PR or the approach outlined in #1439, but not both. I will wait until we have the input needed to decide on #1439

cszczepaniak avatar Jul 30 '23 22:07 cszczepaniak

@cszczepaniak I just merged #1481 which uses runtime.Goexit. The single goroutine approach is incompatible.

dolmen avatar Jun 13 '24 20:06 dolmen

@dolmen sounds good. I don't think that affects this PR, though.

cszczepaniak avatar Jun 14 '24 02:06 cszczepaniak

@cszczepaniak Please rebase as this requires to resolve merge conflicts.

dolmen avatar Jul 09 '24 08:07 dolmen

@dolmen Thanks, I've rebased now.

cszczepaniak avatar Jul 09 '24 13:07 cszczepaniak