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: clarify Eventually documentation #1534

Closed
wants to merge 1 commit into from

Conversation

st3penta
Copy link

Summary

The documentation of the Eventually function is a bit ambiguous, not specifying what happens when the execution of the condition function takes longer than one tick.

Changes

Clarify the Eventually function description by describing the behavior of the condition function execution in the above scenario.

Related issues

Closes #1443

@st3penta st3penta force-pushed the assert-clarify-docs branch from 360f1d6 to 890a441 Compare February 15, 2024 22:19
@arjunmahishi arjunmahishi added documentation assert.Eventually About assert.Eventually/EventuallyWithT labels Feb 16, 2024
Copy link
Collaborator

@arjunmahishi arjunmahishi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this comment is answering a very specific question about the implementation. If the user is not concerned with this detail, it might confuse them.

IMO, we can leave the doc comment as it was before and add comments within the function that describe the implementation details.


Perhaps here we can describe that "the ticker is paused while the condition function is being evaluated. If the condition function yields false it is resumed again"

testify/assert/assertions.go

Lines 1880 to 1888 in 890a441

case <-tick:
tick = nil
go func() { ch <- condition() }()
case v := <-ch:
if v {
return true
}
tick = ticker.C
}

@st3penta
Copy link
Author

Actually i found that there are 3 occurrences of this same ticker logic: here, here, and here. Should we replicate the same comment in all those 3 points?

I liked the idea of moving the explanation closer to the implementation, but actually the code itself is pretty simple, so maybe the comment isn't really necessary, what do you think?

@arjunmahishi
Copy link
Collaborator

but actually the code itself is pretty simple

Agreed. We can skip it then.

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 documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify documentation of assert.Eventually/assert.EventuallyWithT
2 participants