frankban / quicktest

Quick helpers for testing Go applications
MIT License
529 stars 27 forks source link

proposal: `Eventually` checker #121

Open rogpeppe opened 2 years ago

rogpeppe commented 2 years ago

Here's an idea for an API to make it easier to wait for a given condition to become true:

package quicktest

import "github.com/rogpeppe/retry"

// Eventually calls its provided function repeatedly,
// passing its returned value to the checker c until the checker
// succeeds.
//
// The retry argument governs how often the function is
// called; if it's nil, the default strategy is to use an exponential
// backoff that times out after about 5s.
//
// For example:
//
//  qt.Assert(t, func() int64 {
//      return atomic.LoadInt64(&foo)
//  }, qt.Eventually(qt.Equals, nil), int64(1234))
func Eventually(c Checker, retry *retry.Strategy) Checker

// EventuallyStable is like Eventually except that it also runs the checker
// for a time after the checker succeeds to make sure that it doesn't
// fail again. The stableRetry argument governs how that is done;
// the default is to run for about 100ms.
//
// In general stableRetry should complete in a much shorter
// period of time than retry because it will always be run to completion.
func EventuallyStable(c Checker, retry, stableRetry *retry.Strategy) Checker
mvdan commented 2 years ago

For both APIs, my mind immediately goes to: you mention the default limits (5s and 100ms), but not the initial frequency at which the tries are done. Does it start at every 1ms? 100ms?

// In general stableRetry should complete in a much shorter // period of time than retry because it will always be run to completion.

This sentence confused me at first; I wasn't sure if you meant that each check try should complete fast, or that the entire retry strategy should have a shorter total duration. I think you mean the latter; perhaps best to be explicit, like:

// In general, stableRetry should have a smaller maximum duration than retry // because it will always be run to completion.

mvdan commented 2 years ago

For anyone stumbling upon this, here's some context on the retry API: https://pkg.go.dev/github.com/rogpeppe/retry

frankban commented 2 years ago

This looks great!

It would be nice to try to improve how the strategy is provided.

One option is to make Eventually return a concrete type implementing Checker (rather than Checker itself), this concrete type having a WithStrategy(s retry.Strategy) Checker method. So, in most cases, we can just write

qt.Assert(f, qt.Eventually(qt.Equals), 42)
qt.Assert(f, qt.Eventually(qt.IsNil))

which reads well. And, when needed, a customized strategy can be provided:

qt.Assert(f, qt.Eventually(qt.Equals).WithStrategy(s), 42)

Similarly, WithStrategies(before, after *retry.Strategy) Checker could be used on EventuallyStable.

What do you think?