From 14765b872058da589e8dcdf46a18b18913a34aa9 Mon Sep 17 00:00:00 2001 From: Trevor Suarez Date: Sat, 31 Jul 2021 19:38:45 -0600 Subject: [PATCH 1/5] Adding a test for expected attempt numbers --- retry_test.go | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/retry_test.go b/retry_test.go index 8340a15..b3b886b 100644 --- a/retry_test.go +++ b/retry_test.go @@ -17,6 +17,43 @@ func TestRetry(t *testing.T) { } } +func TestRetryAttemptNumberIsAccurate(t *testing.T) { + var strategyAttemptNumber uint + var actionAttemptNumber uint + + strategy := func(attempt uint) bool { + strategyAttemptNumber = attempt + + return true + } + + action := func(attempt uint) error { + actionAttemptNumber = attempt + + return nil + } + + err := Retry(action, strategy) + + if err != nil { + t.Error("expected a nil error") + } + + if strategyAttemptNumber != 0 { + t.Errorf( + "expected strategy to receive 0, received %v instead", + strategyAttemptNumber, + ) + } + + if actionAttemptNumber != 1 { + t.Errorf( + "expected action to receive 1, received %v instead", + actionAttemptNumber, + ) + } +} + func TestRetryRetriesUntilNoErrorReturned(t *testing.T) { const errorUntilAttemptNumber = 5 From 6d0dd13076e614e2d33446960ecb95bf079e6ba4 Mon Sep 17 00:00:00 2001 From: Trevor Suarez Date: Sat, 31 Jul 2021 19:41:46 -0600 Subject: [PATCH 2/5] Fixing attempt number to start at 1 for actions It'll still start at 0 for strategies. --- retry.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/retry.go b/retry.go index 57aed6c..307cf53 100644 --- a/retry.go +++ b/retry.go @@ -17,7 +17,7 @@ func Retry(action Action, strategies ...strategy.Strategy) error { var err error for attempt := uint(0); (attempt == 0 || err != nil) && shouldAttempt(attempt, strategies...); attempt++ { - err = action(attempt) + err = action(attempt + 1) } return err From 552c087eff57c3f753d9a4a63e90967ed3ef22c3 Mon Sep 17 00:00:00 2001 From: Trevor Suarez Date: Sat, 31 Jul 2021 19:47:00 -0600 Subject: [PATCH 3/5] Improving documentation on strategy attempts --- strategy/strategy.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/strategy/strategy.go b/strategy/strategy.go index 0a5148c..52dda3f 100644 --- a/strategy/strategy.go +++ b/strategy/strategy.go @@ -15,9 +15,9 @@ import ( // allows for the next attempt to be made. Returning `false` halts the retrying // process and returns the last error returned by the called Action. // -// The strategy will be passed an "attempt" number on each successive retry +// The strategy will be passed an "attempt" number before each successive retry // iteration, starting with a `0` value before the first attempt is actually -// made. This allows for a pre-action delay, etc. +// made. This allows for a pre-action, such as a delay, etc. type Strategy func(attempt uint) bool // Limit creates a Strategy that limits the number of attempts that Retry will From 55b1410629a4a150ccdf5deabb51b0afd85593d5 Mon Sep 17 00:00:00 2001 From: Trevor Suarez Date: Sat, 31 Jul 2021 20:22:31 -0600 Subject: [PATCH 4/5] Fixing the limit test and improving an example Just now realizing that the limit strategy has had an off-by-one error for 5+ years now... Not sure what I was thinking here: 678601cc58fa358336a46de5c4c6305ab7875ca6 --- example_test.go | 9 +++++++++ strategy/strategy_test.go | 10 ++++++---- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/example_test.go b/example_test.go index 2071d58..b34ba1b 100644 --- a/example_test.go +++ b/example_test.go @@ -69,6 +69,8 @@ func Example_httpGetWithStrategies() { func Example_withBackoffJitter() { action := func(attempt uint) error { + fmt.Println("attempt", attempt) + return errors.New("something happened") } @@ -83,4 +85,11 @@ func Example_withBackoffJitter() { jitter.Deviation(random, 0.5), ), ) + + // Output: + // attempt 1 + // attempt 2 + // attempt 3 + // attempt 4 + // attempt 5 } diff --git a/strategy/strategy_test.go b/strategy/strategy_test.go index 8c525b6..0c13bd7 100644 --- a/strategy/strategy_test.go +++ b/strategy/strategy_test.go @@ -10,23 +10,25 @@ import ( const timeMarginOfError = time.Millisecond func TestLimit(t *testing.T) { + // Strategy attempts are 0-based. + // Treat this functionally as n+1. const attemptLimit = 3 strategy := Limit(attemptLimit) - if !strategy(1) { + if !strategy(0) { t.Error("strategy expected to return true") } - if !strategy(2) { + if !strategy(1) { t.Error("strategy expected to return true") } - if !strategy(3) { + if !strategy(2) { t.Error("strategy expected to return true") } - if strategy(4) { + if strategy(3) { t.Error("strategy expected to return false") } } From 90c002ca88d4f3b00572f03aa29f6c967115b2a4 Mon Sep 17 00:00:00 2001 From: Trevor Suarez Date: Sat, 31 Jul 2021 20:25:05 -0600 Subject: [PATCH 5/5] Fixing a bug with the `Limit` strategy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit (It's had an off-by-one error for 5+ years now...) déjà vu: 678601cc58fa358336a46de5c4c6305ab7875ca6 --- strategy/strategy.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/strategy/strategy.go b/strategy/strategy.go index 52dda3f..8d2f0ec 100644 --- a/strategy/strategy.go +++ b/strategy/strategy.go @@ -24,7 +24,7 @@ type Strategy func(attempt uint) bool // make. func Limit(attemptLimit uint) Strategy { return func(attempt uint) bool { - return (attempt <= attemptLimit) + return (attempt < attemptLimit) } }