Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Succeed quickly in Eventually, EventuallyWithT #1424

Open
cszczepaniak opened this issue Jul 13, 2023 · 8 comments · May be fixed by #1657
Open

Succeed quickly in Eventually, EventuallyWithT #1424

cszczepaniak opened this issue Jul 13, 2023 · 8 comments · May be fixed by #1657
Labels
assert.Eventually About assert.Eventually/EventuallyWithT enhancement pkg-assert Change related to package testify/assert under consideration

Comments

@cszczepaniak
Copy link

cszczepaniak commented Jul 13, 2023

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
Copy link
Collaborator

dolmen commented Jul 25, 2023

I think this is a good behavior change.

We must involve people who designed it.

@cszczepaniak
Copy link
Author

This would be addressed by the proposal in #1439 if that were to move forward.

@dolmen dolmen added the assert.Eventually About assert.Eventually/EventuallyWithT label Jul 31, 2023
@pgimalac
Copy link

pgimalac commented Jun 6, 2024

👋 Hey @dolmen @cszczepaniak, any update on this ?
Would you consider a PR fixing this specifically ? (without necessarily going with the bigger #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
Copy link
Author

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

@pgimalac
Copy link

pgimalac commented Jun 8, 2024

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

@cszczepaniak
Copy link
Author

@dolmen @pgimalac #1427 is now rebased and cleaned up. Ready for review again!

@dolmen
Copy link
Collaborator

dolmen commented Jun 13, 2024

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

@cszczepaniak
Copy link
Author

cszczepaniak commented Jun 17, 2024

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert.Eventually About assert.Eventually/EventuallyWithT enhancement pkg-assert Change related to package testify/assert under consideration
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants