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

assert: check early in Eventually and EventuallyWithT #1427

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

cszczepaniak
Copy link

@cszczepaniak cszczepaniak commented Jul 15, 2023

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

@dolmen dolmen added enhancement pkg-assert Change related to package testify/assert labels Jul 25, 2023
@dolmen
Copy link
Collaborator

dolmen commented Jul 25, 2023

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

cszczepaniak commented Jul 25, 2023

@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?

assert/assertions.go Outdated Show resolved Hide resolved
@cszczepaniak
Copy link
Author

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

@dolmen
Copy link
Collaborator

dolmen commented Jul 30, 2023

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?

@cszczepaniak
Copy link
Author

@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

@dolmen dolmen added the assert.Eventually About assert.Eventually/EventuallyWithT label Jul 31, 2023
@dolmen dolmen changed the title Return early in Eventually and EventuallyWithT assert: early in Eventually and EventuallyWithT Oct 16, 2023
@dolmen dolmen changed the title assert: early in Eventually and EventuallyWithT assert: check early in Eventually and EventuallyWithT Oct 30, 2023
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants