Repository: beatlabs/patron
Pull request #185 (2020-03-16 14:34:18)

Use delay in the Retry pattern
## Which problem is this PR solving?

Solves #184 - Use delay in the `Retry` pattern

## Short description of the changes

`delay` was originally passed to the `Retry` struct but was not handled. This PR adds a `time.Sleep` between each retries.

The PR also transformed tests into table-driven tests to avoid c/p.


codecov[bot]: # [Codecov](https://codecov.io/gh/beatlabs/patron/pull/185?src=pr&el=h1) Report
> Merging [#185](https://codecov.io/gh/beatlabs/patron/pull/185?src=pr&el=desc) into [master](https://codecov.io/gh/beatlabs/patron/commit/7c27b37aba9dc190efbf0c6a4c56660eea826980&el=desc) will **increase** coverage by `0.01%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/beatlabs/patron/pull/185/graphs/tree.svg?width=650&height=150&src=pr&token=sxY15rXW1X)](https://codecov.io/gh/beatlabs/patron/pull/185?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #185      +/-   ##
==========================================
+ Coverage   84.40%   84.41%   +0.01%     
==========================================
  Files          51       51              
  Lines        3046     3048       +2     
==========================================
+ Hits         2571     2573       +2     
  Misses        397      397              
  Partials       78       78              
```


| [Impacted Files](https://codecov.io/gh/beatlabs/patron/pull/185?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [reliability/retry/retry.go](https://codecov.io/gh/beatlabs/patron/pull/185/diff?src=pr&el=tree#diff-cmVsaWFiaWxpdHkvcmV0cnkvcmV0cnkuZ28=) | `100.00% <100.00%> (ø)` | |
| [trace/sql/sql.go](https://codecov.io/gh/beatlabs/patron/pull/185/diff?src=pr&el=tree#diff-dHJhY2Uvc3FsL3NxbC5nbw==) | `86.25% <100.00%> (ø)` | |
| [async/kafka/group/group.go](https://codecov.io/gh/beatlabs/patron/pull/185/diff?src=pr&el=tree#diff-YXN5bmMva2Fma2EvZ3JvdXAvZ3JvdXAuZ28=) | `83.13% <0.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/beatlabs/patron/pull/185?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/beatlabs/patron/pull/185?src=pr&el=footer). Last update [69990e1...35e4289](https://codecov.io/gh/beatlabs/patron/pull/185?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/reliability/retry/retry.go b/reliability/retry/retry.go
index a708b54d0..2bd30a0d5 100644
--- a/reliability/retry/retry.go
+++ b/reliability/retry/retry.go
@@ -37,6 +37,10 @@ func (r Retry) Execute(act Action) (interface{}, error) {
 		if current == 0 {
 			break
 		}
+
+		if r.delay > 0 {
+			time.Sleep(r.delay)
+		}
 	}
 	return nil, err
 }


Test patch:
diff --git a/reliability/retry/retry_test.go b/reliability/retry/retry_test.go
index 60ab21c22..b84d2ef5e 100644
--- a/reliability/retry/retry_test.go
+++ b/reliability/retry/retry_test.go
@@ -6,6 +6,12 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+var (
+	errTest    = errors.New("test error")
+	testResult = "test result"
 )
 
 func TestNew(t *testing.T) {
@@ -35,28 +41,68 @@ func TestNew(t *testing.T) {
 	}
 }
 
-func TestRetry_Execute_Success(t *testing.T) {
-	r, err := New(3, 10*time.Millisecond)
-	assert.NoError(t, err)
-	act := mockAction{errors: 1}
-	res, err := r.Execute(func() (interface{}, error) {
-		return act.Execute()
-	})
-	assert.NoError(t, err)
-	assert.NotNil(t, res)
-	assert.Equal(t, 2, act.executions)
-}
+func Test_Retry_Execute(t *testing.T) {
+	testCases := map[string]struct {
+		retries            int
+		delay              time.Duration
+		action             mockAction
+		expectedExecutions int
+		expectErr          bool
+	}{
+		"instant success": {
+			retries:            3,
+			action:             mockAction{errors: 0},
+			expectedExecutions: 1,
+		},
+		"instant success with delay": {
+			retries:            3,
+			delay:              500 * time.Millisecond,
+			action:             mockAction{errors: 0},
+			expectedExecutions: 1,
+		},
+		"success without delay after an error": {
+			retries:            3,
+			action:             mockAction{errors: 1},
+			expectedExecutions: 2,
+		},
+		"success with delay after an error": {
+			retries:            3,
+			delay:              500 * time.Millisecond,
+			action:             mockAction{errors: 1},
+			expectedExecutions: 2,
+		},
+		"error": {
+			retries:            3,
+			action:             mockAction{errors: 3},
+			expectedExecutions: 3,
+			expectErr:          true,
+		},
+	}
+	for name, tC := range testCases {
+		t.Run(name, func(t *testing.T) {
+			r, err := New(tC.retries, tC.delay)
+			require.NoError(t, err)
+
+			start := time.Now()
+			res, err := r.Execute(func() (interface{}, error) {
+				return tC.action.Execute()
+			})
+			elapsed := time.Since(start)
 
-func TestRetry_Execute_Failed(t *testing.T) {
-	r, err := New(3, 10*time.Millisecond)
-	assert.NoError(t, err)
-	act := mockAction{errors: 3}
-	res, err := r.Execute(func() (interface{}, error) {
-		return act.Execute()
-	})
-	assert.Error(t, err)
-	assert.Nil(t, res)
-	assert.Equal(t, 3, act.executions)
+			if tC.expectErr {
+				assert.Equal(t, err, errTest)
+				assert.Nil(t, res)
+			} else {
+				assert.NoError(t, err)
+				assert.Equal(t, testResult, res)
+			}
+
+			assert.Equal(t, tC.expectedExecutions, tC.action.executions)
+
+			// Assert that the total time taken takes into account the delay between retries
+			assert.True(t, elapsed > tC.delay*time.Duration(tC.expectedExecutions-1))
+		})
+	}
 }
 
 type mockAction struct {
@@ -70,14 +116,14 @@ func (ma *mockAction) Execute() (string, error) {
 		ma.executions++
 	}()
 	if ma.errors > 0 {
-		return "", errors.New("TEST")
+		return "", errTest
 	}
-	return "TEST", nil
+	return testResult, nil
 }
 
 var err error
 
-func BenchmarkCircuitBreaker_Execute(b *testing.B) {
+func BenchmarkRetry_Execute(b *testing.B) {
 	var r *Retry
 	r, err = New(3, 0)
 	b.ReportAllocs()