search

stretchr / testify

A toolkit with common assertions and mocks that plays nicely with the standard library
MIT License
22.5k stars 1.56k forks source link

Succeed quickly in `Eventually`, `EventuallyWithT` #1424

Open cszczepaniak opened 11 months ago

cszczepaniak commented 11 months ago

assert.Eventually does not check initially whether the condition is satisfied. This means it must wait a minimum of the tick duration before checking the condition for the first time. It would be a nice optimization to early-out if the condition is met already when the function is called.

dolmen commented 11 months ago

I think this is a good behavior change.

We must involve people who designed it.

cszczepaniak commented 11 months ago

This would be addressed by the proposal in https://github.com/stretchr/testify/issues/1439 if that were to move forward.

pgimalac commented 4 weeks ago

👋 Hey @dolmen @cszczepaniak, any update on this ? Would you consider a PR fixing this specifically ? (without necessarily going with the bigger https://github.com/stretchr/testify/issues/1439)

I work on a project where Eventually is used pretty extensively in e2e tests where the tick is often in the order of several seconds, so this would be a great improvement duration-wise !

cszczepaniak commented 3 weeks ago

I would still be willing to make this change if everybody is in agreement.

pgimalac commented 3 weeks ago

Works for me, as long as this is fixed ! I didn't notice you already had a PR opened by the way 😄

cszczepaniak commented 3 weeks ago

@dolmen @pgimalac https://github.com/stretchr/testify/pull/1427 is now rebased and cleaned up. Ready for review again!

dolmen commented 3 weeks ago

@cszczepaniak I just merged #1481 which uses runtime.Goexit if FailNow is called from the condition. The single goroutine approach seems incompatible.

cszczepaniak commented 2 weeks ago

@dolmen commented on the PR as well, but checking the condition first doesn't require the single goroutine change (which is #1439) so I still think addressing this issue is worthwhile, but possibly we should just close #1439