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

Add an EventuallyChecker #141

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

Conversation

TeodorPt
Copy link

This PR proposes one form of an EventuallyChecker, which allows asserting eventually (i.e. retry for a given period of time until the assertion holds true) and also for stability (i.e. retry for a given period of time during which the assertion should still hold true).

The EventuallyChecker implements the Checker interface so it can be used like any regular checker. It expects as argument a function with no parameters and one return value.

See issue #121.

@TeodorPt
Copy link
Author

@rogpeppe ptal when you have the chance, this is an implementation proposal for the Eventually checker you proposed

Copy link
Owner

@frankban frankban left a comment

Choose a reason for hiding this comment

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

This looks awesome, thanks!
I have some questions and comments.

eventually.go Outdated Show resolved Hide resolved
eventually.go Show resolved Hide resolved
eventually.go Outdated
break
}
if !i.Next(nil) {
note("got", got)
Copy link
Owner

Choose a reason for hiding this comment

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

Can't we trust that the underlying checker already notes the got argument?

Copy link
Author

Choose a reason for hiding this comment

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

The problem is that the got argument is not noted by the checkers, it's appended to the output by the caller in writeError according to the parameter passed to the Eventually checker, which is a function. So the output will look like this:

got: func () int {...}
want: 42

We need overwrite that and that's why I added it as a note and then changed the logic slightly in writeError to skip any provided argument if they are already in the notes.

eventually.go Outdated Show resolved Hide resolved
eventually.go Outdated Show resolved Hide resolved
eventually.go Outdated Show resolved Hide resolved
eventually.go Show resolved Hide resolved
eventually.go Outdated Show resolved Hide resolved
@@ -90,9 +90,12 @@ func writeError(w io.Writer, err error, p reportParams) {
}

// Write notes if present.
noteKeys := map[string]bool{}
Copy link
Owner

Choose a reason for hiding this comment

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

Not sure this is required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants