From b0043098deef66ee518e3c37852800cf82e785ac Mon Sep 17 00:00:00 2001 From: Nick Irvine <115657443+nfi-hashicorp@users.noreply.github.com> Date: Mon, 23 Jan 2023 17:07:55 -0800 Subject: [PATCH 1/3] chore: document and unit test sdk/testutil/retry --- sdk/testutil/retry/retry.go | 53 +++++++++++++++++++++++++++++--- sdk/testutil/retry/retry_test.go | 37 ++++++++++++++++++++++ 2 files changed, 86 insertions(+), 4 deletions(-) diff --git a/sdk/testutil/retry/retry.go b/sdk/testutil/retry/retry.go index 6e3ab3d466e8..cf7481aa2a26 100644 --- a/sdk/testutil/retry/retry.go +++ b/sdk/testutil/retry/retry.go @@ -5,10 +5,17 @@ // func TestX(t *testing.T) { // retry.Run(t, func(r *retry.R) { // if err := foo(); err != nil { -// r.Fatal("f: ", err) +// r.Errorf("foo: %s", err) +// return // } // }) // } +// +// Run uses the DefaultFailer, which is a Timer with a Timeout of 7s, +// and a Wait of 25ms. To customize, use RunWith. +// +// WARNING: unlike *testing.T, *retry.R#Fatal and FailNow *do not* +// fail the test function entirely, only the current run the retry func package retry import ( @@ -31,8 +38,16 @@ type Failer interface { } // R provides context for the retryer. +// +// Logs from Logf, (Error|Fatal)(f) are gathered in an internal buffer +// and printed only if the retryer fails. Printed logs are deduped and +// prefixed with source code line numbers type R struct { - fail bool + // fail is set by FailNow and (Fatal|Error)(f). It indicates the pass + // did not succeed, and should be retried + fail bool + // done is set by Stop. It indicates the entire run was a failure, + // and triggers t.FailNow() done bool output []string } @@ -43,33 +58,58 @@ func (r *R) Logf(format string, args ...interface{}) { func (r *R) Helper() {} +// runFailed is a sentinel value to indicate that the func itself +// didn't panic, rather that `FailNow` was called. +// +// TODO: this value isn't a unique sentinel value; it compares equal to +// any other `struct{}{}`. var runFailed = struct{}{} +// FailNow stops run execution. It is roughly equivalent to: +// +// r.Error("") +// return +// +// inside the function being run. func (r *R) FailNow() { r.fail = true panic(runFailed) } +// Fatal is equivalent to r.Logf(args) followed by r.FailNow(), i.e. the run +// function should be exited. Retries on the next run are allowed. Fatal is +// equivalent to +// +// r.Error(args) +// return +// +// inside the function being run. func (r *R) Fatal(args ...interface{}) { r.log(fmt.Sprint(args...)) r.FailNow() } +// Fatalf is like Fatal but allows a format string func (r *R) Fatalf(format string, args ...interface{}) { r.log(fmt.Sprintf(format, args...)) r.FailNow() } +// Error indicates the current run encountered an error and should be retried. +// It *does not* stop execution of the rest of the run function. func (r *R) Error(args ...interface{}) { r.log(fmt.Sprint(args...)) r.fail = true } +// Errorf is like Error but allows a format string func (r *R) Errorf(format string, args ...interface{}) { r.log(fmt.Sprintf(format, args...)) r.fail = true } +// If err is non-nil, equivalent to r.Fatal(err.Error()) followed by +// r.FailNow(). Otherwise a no-op. func (r *R) Check(err error) { if err != nil { r.log(err.Error()) @@ -81,7 +121,8 @@ func (r *R) log(s string) { r.output = append(r.output, decorate(s)) } -// Stop retrying, and fail the test with the specified error. +// Stop retrying, and fail the test, logging the specified error. +// Does not stop execution, so return should be called after. func (r *R) Stop(err error) { r.log(err.Error()) r.done = true @@ -142,6 +183,8 @@ func run(r Retryer, t Failer, f func(r *R)) { } for r.Continue() { + // run f(rr), but if recover yields a runFailed value, we know + // FailNow was called. func() { defer func() { if p := recover(); p != nil && p != runFailed { @@ -163,7 +206,8 @@ func run(r Retryer, t Failer, f func(r *R)) { fail() } -// DefaultFailer provides default retry.Run() behavior for unit tests. +// DefaultFailer provides default retry.Run() behavior for unit tests, namely +// 7s timeout with a wait of 25ms func DefaultFailer() *Timer { return &Timer{Timeout: 7 * time.Second, Wait: 25 * time.Millisecond} } @@ -213,6 +257,7 @@ type Timer struct { Wait time.Duration // stop is the timeout deadline. + // TODO: Next()? // Set on the first invocation of Next(). stop time.Time } diff --git a/sdk/testutil/retry/retry_test.go b/sdk/testutil/retry/retry_test.go index 31923a0bfb22..cfca5e98c544 100644 --- a/sdk/testutil/retry/retry_test.go +++ b/sdk/testutil/retry/retry_test.go @@ -5,6 +5,7 @@ import ( "testing" "time" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -41,6 +42,40 @@ func TestRetryer(t *testing.T) { } } +func TestBasic(t *testing.T) { + t.Run("Error allows retry", func(t *testing.T) { + i := 0 + Run(t, func(r *R) { + i++ + t.Logf("i: %d; r: %#v", i, r) + if i == 1 { + r.Errorf("Errorf, i: %d", i) + return + } + }) + assert.Equal(t, i, 2) + }) + + t.Run("Fatal returns from func, but does not fail test", func(t *testing.T) { + i := 0 + gotHere := false + ft := &fakeT{} + Run(ft, func(r *R) { + i++ + t.Logf("i: %d; r: %#v", i, r) + if i == 1 { + r.Fatalf("Fatalf, i: %d", i) + gotHere = true + } + }) + + assert.False(t, gotHere) + assert.Equal(t, i, 2) + // surprisingly, r.FailNow() *does not* trigger ft.FailNow()! + assert.Equal(t, ft.fails, 0) + }) +} + func TestRunWith(t *testing.T) { t.Run("calls FailNow after exceeding retries", func(t *testing.T) { ft := &fakeT{} @@ -65,12 +100,14 @@ func TestRunWith(t *testing.T) { r.Fatalf("not yet") }) + // TODO: these should all be assert require.Equal(t, 2, iter) require.Equal(t, 1, ft.fails) require.Len(t, ft.out, 1) require.Contains(t, ft.out[0], "not yet\n") require.Contains(t, ft.out[0], "do not proceed\n") }) + } type fakeT struct { From 8e2136126f21ed9995e54a484d59bfe2fc35fe68 Mon Sep 17 00:00:00 2001 From: Nick Irvine <115657443+nfi-hashicorp@users.noreply.github.com> Date: Mon, 23 Jan 2023 17:09:59 -0800 Subject: [PATCH 2/3] add test for known bug with runFailed [should fail tests] --- sdk/testutil/retry/retry_test.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/sdk/testutil/retry/retry_test.go b/sdk/testutil/retry/retry_test.go index cfca5e98c544..66e5bb3f28d8 100644 --- a/sdk/testutil/retry/retry_test.go +++ b/sdk/testutil/retry/retry_test.go @@ -108,6 +108,21 @@ func TestRunWith(t *testing.T) { require.Contains(t, ft.out[0], "do not proceed\n") }) + t.Run("Func being run can panic with struct{}{}", func(t *testing.T) { + gotPanic := false + func() { + defer func() { + if p := recover(); p != nil { + gotPanic = true + } + }() + Run(t, func(r *R) { + panic(struct{}{}) + }) + }() + + assert.True(t, gotPanic) + }) } type fakeT struct { From f4112692446599f3ca1933f5fc61710259a043d5 Mon Sep 17 00:00:00 2001 From: Nick Irvine <115657443+nfi-hashicorp@users.noreply.github.com> Date: Mon, 23 Jan 2023 17:11:41 -0800 Subject: [PATCH 3/3] fix runFailed --- sdk/testutil/retry/retry.go | 9 +++------ sdk/testutil/retry/retry_test.go | 34 ++++++++++++++++---------------- 2 files changed, 20 insertions(+), 23 deletions(-) diff --git a/sdk/testutil/retry/retry.go b/sdk/testutil/retry/retry.go index cf7481aa2a26..4ee54ae83e2b 100644 --- a/sdk/testutil/retry/retry.go +++ b/sdk/testutil/retry/retry.go @@ -60,10 +60,7 @@ func (r *R) Helper() {} // runFailed is a sentinel value to indicate that the func itself // didn't panic, rather that `FailNow` was called. -// -// TODO: this value isn't a unique sentinel value; it compares equal to -// any other `struct{}{}`. -var runFailed = struct{}{} +type runFailed struct{} // FailNow stops run execution. It is roughly equivalent to: // @@ -73,7 +70,7 @@ var runFailed = struct{}{} // inside the function being run. func (r *R) FailNow() { r.fail = true - panic(runFailed) + panic(runFailed{}) } // Fatal is equivalent to r.Logf(args) followed by r.FailNow(), i.e. the run @@ -187,7 +184,7 @@ func run(r Retryer, t Failer, f func(r *R)) { // FailNow was called. func() { defer func() { - if p := recover(); p != nil && p != runFailed { + if p := recover(); p != nil && p != (runFailed{}) { panic(p) } }() diff --git a/sdk/testutil/retry/retry_test.go b/sdk/testutil/retry/retry_test.go index 66e5bb3f28d8..ae1c81f6be4a 100644 --- a/sdk/testutil/retry/retry_test.go +++ b/sdk/testutil/retry/retry_test.go @@ -42,7 +42,7 @@ func TestRetryer(t *testing.T) { } } -func TestBasic(t *testing.T) { +func TestBasics(t *testing.T) { t.Run("Error allows retry", func(t *testing.T) { i := 0 Run(t, func(r *R) { @@ -74,6 +74,22 @@ func TestBasic(t *testing.T) { // surprisingly, r.FailNow() *does not* trigger ft.FailNow()! assert.Equal(t, ft.fails, 0) }) + + t.Run("Func being run can panic with struct{}{}", func(t *testing.T) { + gotPanic := false + func() { + defer func() { + if p := recover(); p != nil { + gotPanic = true + } + }() + Run(t, func(r *R) { + panic(struct{}{}) + }) + }() + + assert.True(t, gotPanic) + }) } func TestRunWith(t *testing.T) { @@ -107,22 +123,6 @@ func TestRunWith(t *testing.T) { require.Contains(t, ft.out[0], "not yet\n") require.Contains(t, ft.out[0], "do not proceed\n") }) - - t.Run("Func being run can panic with struct{}{}", func(t *testing.T) { - gotPanic := false - func() { - defer func() { - if p := recover(); p != nil { - gotPanic = true - } - }() - Run(t, func(r *R) { - panic(struct{}{}) - }) - }() - - assert.True(t, gotPanic) - }) } type fakeT struct {