From 16336135d6208e435b21e7d95e70a9e2ca1dcac2 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 4 Aug 2022 14:03:56 +0000 Subject: [PATCH 1/7] feat: add testutil.Eventually --- coderd/coderdtest/coderdtest.go | 16 +++---- coderd/coderdtest/coderdtest_test.go | 2 +- testutil/eventually.go | 64 +++++++++++++++++++++++++ testutil/eventually_test.go | 70 ++++++++++++++++++++++++++++ 4 files changed, 143 insertions(+), 9 deletions(-) create mode 100644 testutil/eventually.go create mode 100644 testutil/eventually_test.go diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index 6b0b2d425caaf..8502b038da66d 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -411,11 +411,11 @@ func AwaitTemplateVersionJob(t *testing.T, client *codersdk.Client, version uuid t.Logf("waiting for template version job %s", version) var templateVersion codersdk.TemplateVersion - require.Eventually(t, func() bool { + require.True(t, testutil.EventuallyShort(t, func() bool { var err error templateVersion, err = client.TemplateVersion(context.Background(), version) return assert.NoError(t, err) && templateVersion.Job.CompletedAt != nil - }, testutil.WaitShort, testutil.IntervalFast) + })) return templateVersion } @@ -425,11 +425,10 @@ func AwaitWorkspaceBuildJob(t *testing.T, client *codersdk.Client, build uuid.UU t.Logf("waiting for workspace build job %s", build) var workspaceBuild codersdk.WorkspaceBuild - require.Eventually(t, func() bool { - var err error - workspaceBuild, err = client.WorkspaceBuild(context.Background(), build) + require.True(t, testutil.EventuallyShort(t, func() bool { + workspaceBuild, err := client.WorkspaceBuild(context.Background(), build) return assert.NoError(t, err) && workspaceBuild.Job.CompletedAt != nil - }, testutil.WaitShort, testutil.IntervalFast) + })) return workspaceBuild } @@ -439,7 +438,7 @@ func AwaitWorkspaceAgents(t *testing.T, client *codersdk.Client, build uuid.UUID t.Logf("waiting for workspace agents (build %s)", build) var resources []codersdk.WorkspaceResource - require.Eventually(t, func() bool { + require.True(t, testutil.EventuallyShort(t, func() bool { var err error resources, err = client.WorkspaceResourcesByBuild(context.Background(), build) if !assert.NoError(t, err) { @@ -448,12 +447,13 @@ func AwaitWorkspaceAgents(t *testing.T, client *codersdk.Client, build uuid.UUID for _, resource := range resources { for _, agent := range resource.Agents { if agent.Status != codersdk.WorkspaceAgentConnected { + t.Logf("agent %s not connected yet", agent.Name) return false } } } return true - }, testutil.WaitLong, testutil.IntervalMedium) + })) return resources } diff --git a/coderd/coderdtest/coderdtest_test.go b/coderd/coderdtest/coderdtest_test.go index c9577e039210f..582d4188d6ba1 100644 --- a/coderd/coderdtest/coderdtest_test.go +++ b/coderd/coderdtest/coderdtest_test.go @@ -19,7 +19,7 @@ func TestNew(t *testing.T) { }) user := coderdtest.CreateFirstUser(t, client) version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) - coderdtest.AwaitTemplateVersionJob(t, client, version.ID) + _ = coderdtest.AwaitTemplateVersionJob(t, client, version.ID) template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) workspace := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) coderdtest.AwaitWorkspaceBuildJob(t, client, workspace.LatestBuild.ID) diff --git a/testutil/eventually.go b/testutil/eventually.go new file mode 100644 index 0000000000000..0d4d858dc0dd5 --- /dev/null +++ b/testutil/eventually.go @@ -0,0 +1,64 @@ +package testutil + +import ( + "context" + "testing" + "time" +) + +// Eventually is like require.Eventually except it takes a context. +// If ctx times out, the test will fail. +// ctx must have a deadline set or this will panic. +func Eventually(ctx context.Context, t testing.TB, condition func() bool, tick time.Duration) bool { + t.Helper() + + if _, ok := ctx.Deadline(); !ok { + panic("developer error: must set deadline on ctx") + } + + ch := make(chan bool, 1) + ticker := time.NewTicker(tick) + defer ticker.Stop() + for tick := ticker.C; ; { + select { + case <-ctx.Done(): + t.Errorf("Await timed out") + return false + case <-tick: + tick = nil + go func() { ch <- condition() }() + case v := <-ch: + if v { + return true + } + tick = ticker.C + } + } +} + +// EventuallyShort is a convenience function that runs Eventually with +// IntervalFast and times out after WaitShort. +func EventuallyShort(t testing.TB, condition func() bool) bool { + //nolint: gocritic + ctx, cancel := context.WithTimeout(context.Background(), WaitShort) + defer cancel() + return Eventually(ctx, t, condition, IntervalFast) +} + +// EventuallyMedium is a convenience function that runs Eventually with +// IntervalMedium and times out after WaitMedium. +func EventuallyMedium(t testing.TB, condition func() bool) bool { + //nolint: gocritic + ctx, cancel := context.WithTimeout(context.Background(), WaitMedium) + defer cancel() + return Eventually(ctx, t, condition, IntervalMedium) +} + +// EventuallyLong is a convenience function that runs Eventually with +// IntervalSlow and times out after WaitLong. +func EventuallyLong(t testing.TB, condition func() bool) bool { + //nolint: gocritic + ctx, cancel := context.WithTimeout(context.Background(), WaitLong) + defer cancel() + return Eventually(ctx, t, condition, IntervalSlow) +} diff --git a/testutil/eventually_test.go b/testutil/eventually_test.go new file mode 100644 index 0000000000000..83c439a165a99 --- /dev/null +++ b/testutil/eventually_test.go @@ -0,0 +1,70 @@ +package testutil_test + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "go.uber.org/goleak" + + "github.com/coder/coder/testutil" +) + +func TestMain(m *testing.M) { + goleak.VerifyTestMain(m) +} + +func TestEventually(t *testing.T) { + t.Parallel() + t.Run("OK", func(t *testing.T) { + t.Parallel() + state := 0 + condition := func() bool { + defer func() { + state++ + }() + return state > 2 + } + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() + assert.True(t, testutil.Eventually(ctx, t, condition, testutil.IntervalFast)) + }) + + t.Run("Timeout", func(t *testing.T) { + t.Parallel() + condition := func() bool { + return false + } + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() + mockT := new(testing.T) + assert.False(t, testutil.Eventually(ctx, mockT, condition, testutil.IntervalFast)) + assert.True(t, mockT.Failed()) + }) + + t.Run("Panic", func(t *testing.T) { + t.Parallel() + + panicky := func() { + mockT := new(testing.T) + condition := func() bool { return true } + assert.False(t, testutil.Eventually(context.Background(), mockT, condition, testutil.IntervalFast)) + } + assert.Panics(t, panicky) + }) + + t.Run("Short", func(t *testing.T) { + t.Parallel() + assert.True(t, testutil.EventuallyShort(t, func() bool { return true })) + }) + + t.Run("Medium", func(t *testing.T) { + t.Parallel() + assert.True(t, testutil.EventuallyMedium(t, func() bool { return true })) + }) + + t.Run("Long", func(t *testing.T) { + t.Parallel() + assert.True(t, testutil.EventuallyLong(t, func() bool { return true })) + }) +} From d5f209bd10b9f9a06a92288106616d3c64869eae Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 5 Aug 2022 12:48:37 +0100 Subject: [PATCH 2/7] Update coderd/coderdtest/coderdtest.go --- coderd/coderdtest/coderdtest.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index 8502b038da66d..71c37f683e1f5 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -438,7 +438,7 @@ func AwaitWorkspaceAgents(t *testing.T, client *codersdk.Client, build uuid.UUID t.Logf("waiting for workspace agents (build %s)", build) var resources []codersdk.WorkspaceResource - require.True(t, testutil.EventuallyShort(t, func() bool { + require.True(t, testutil.EventuallyLong(t, func() bool { var err error resources, err = client.WorkspaceResourcesByBuild(context.Background(), build) if !assert.NoError(t, err) { From 32b578ec71a7ffed39bae581aedf06dbcecd5338 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 5 Aug 2022 13:08:23 +0000 Subject: [PATCH 3/7] fix ruleguard linter --- scripts/rules.go | 2 +- testutil/eventually.go | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/scripts/rules.go b/scripts/rules.go index d77289388ba1c..c34ae6ed3bff3 100644 --- a/scripts/rules.go +++ b/scripts/rules.go @@ -91,7 +91,7 @@ func useStandardTimeoutsAndDelaysInTests(m dsl.Matcher) { m.Import("github.com/coder/coder/testutil") m.Match(`context.WithTimeout($ctx, $duration)`). - Where(m.File().Imports("testing") && !m["duration"].Text.Matches("^testutil\\.")). + Where(m.File().Imports("testing") && !m.File().PkgPath.Matches("testutil$") && !m["duration"].Text.Matches("^testutil\\.")). At(m["duration"]). Report("Do not use magic numbers in test timeouts and delays. Use the standard testutil.Wait* or testutil.Interval* constants instead.") diff --git a/testutil/eventually.go b/testutil/eventually.go index 0d4d858dc0dd5..79ee3fa37dff9 100644 --- a/testutil/eventually.go +++ b/testutil/eventually.go @@ -39,7 +39,6 @@ func Eventually(ctx context.Context, t testing.TB, condition func() bool, tick t // EventuallyShort is a convenience function that runs Eventually with // IntervalFast and times out after WaitShort. func EventuallyShort(t testing.TB, condition func() bool) bool { - //nolint: gocritic ctx, cancel := context.WithTimeout(context.Background(), WaitShort) defer cancel() return Eventually(ctx, t, condition, IntervalFast) @@ -48,7 +47,6 @@ func EventuallyShort(t testing.TB, condition func() bool) bool { // EventuallyMedium is a convenience function that runs Eventually with // IntervalMedium and times out after WaitMedium. func EventuallyMedium(t testing.TB, condition func() bool) bool { - //nolint: gocritic ctx, cancel := context.WithTimeout(context.Background(), WaitMedium) defer cancel() return Eventually(ctx, t, condition, IntervalMedium) @@ -57,7 +55,6 @@ func EventuallyMedium(t testing.TB, condition func() bool) bool { // EventuallyLong is a convenience function that runs Eventually with // IntervalSlow and times out after WaitLong. func EventuallyLong(t testing.TB, condition func() bool) bool { - //nolint: gocritic ctx, cancel := context.WithTimeout(context.Background(), WaitLong) defer cancel() return Eventually(ctx, t, condition, IntervalSlow) From 1dca2c3aef70536db3e86a43f9d49025fb74a317 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 5 Aug 2022 13:36:18 +0000 Subject: [PATCH 4/7] address PR comments --- coderd/coderdtest/coderdtest.go | 12 ++++++------ testutil/eventually.go | 32 ++++++++++++++++++++++---------- testutil/eventually_test.go | 18 +++++++++--------- 3 files changed, 37 insertions(+), 25 deletions(-) diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index 71c37f683e1f5..c858d52a68dd3 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -411,9 +411,9 @@ func AwaitTemplateVersionJob(t *testing.T, client *codersdk.Client, version uuid t.Logf("waiting for template version job %s", version) var templateVersion codersdk.TemplateVersion - require.True(t, testutil.EventuallyShort(t, func() bool { + require.True(t, testutil.EventuallyShort(t, func(ctx context.Context) bool { var err error - templateVersion, err = client.TemplateVersion(context.Background(), version) + templateVersion, err = client.TemplateVersion(ctx, version) return assert.NoError(t, err) && templateVersion.Job.CompletedAt != nil })) return templateVersion @@ -425,8 +425,8 @@ func AwaitWorkspaceBuildJob(t *testing.T, client *codersdk.Client, build uuid.UU t.Logf("waiting for workspace build job %s", build) var workspaceBuild codersdk.WorkspaceBuild - require.True(t, testutil.EventuallyShort(t, func() bool { - workspaceBuild, err := client.WorkspaceBuild(context.Background(), build) + require.True(t, testutil.EventuallyShort(t, func(ctx context.Context) bool { + workspaceBuild, err := client.WorkspaceBuild(ctx, build) return assert.NoError(t, err) && workspaceBuild.Job.CompletedAt != nil })) return workspaceBuild @@ -438,9 +438,9 @@ func AwaitWorkspaceAgents(t *testing.T, client *codersdk.Client, build uuid.UUID t.Logf("waiting for workspace agents (build %s)", build) var resources []codersdk.WorkspaceResource - require.True(t, testutil.EventuallyLong(t, func() bool { + require.True(t, testutil.EventuallyLong(t, func(ctx context.Context) bool { var err error - resources, err = client.WorkspaceResourcesByBuild(context.Background(), build) + resources, err = client.WorkspaceResourcesByBuild(ctx, build) if !assert.NoError(t, err) { return false } diff --git a/testutil/eventually.go b/testutil/eventually.go index 79ee3fa37dff9..c4f6f55aeaf67 100644 --- a/testutil/eventually.go +++ b/testutil/eventually.go @@ -4,16 +4,27 @@ import ( "context" "testing" "time" + + "github.com/stretchr/testify/assert" ) -// Eventually is like require.Eventually except it takes a context. -// If ctx times out, the test will fail. -// ctx must have a deadline set or this will panic. -func Eventually(ctx context.Context, t testing.TB, condition func() bool, tick time.Duration) bool { +// Eventually is like require.Eventually except it allows passing +// a context into the condition. It is safe to use with `require.*`. +// +// If ctx times out, the test will fail, but not immediately. +// It is the caller's responsibility to exit early if required. +// +// It is the caller's responsibility to ensure that ctx has a +// deadline or timeout set. Eventually will panic if this is not +// the case in order to avoid potentially waiting forever. +// +// condition is not run in a goroutine; use the provided +// context argument for cancellation if required. +func Eventually(ctx context.Context, t testing.TB, condition func(context.Context) bool, tick time.Duration) bool { t.Helper() if _, ok := ctx.Deadline(); !ok { - panic("developer error: must set deadline on ctx") + panic("developer error: must set deadline or timeout on ctx") } ch := make(chan bool, 1) @@ -22,13 +33,14 @@ func Eventually(ctx context.Context, t testing.TB, condition func() bool, tick t for tick := ticker.C; ; { select { case <-ctx.Done(): - t.Errorf("Await timed out") + assert.NoError(t, ctx.Err(), "Eventually timed out") return false case <-tick: tick = nil - go func() { ch <- condition() }() + ch <- condition(ctx) case v := <-ch: if v { + close(ch) return true } tick = ticker.C @@ -38,7 +50,7 @@ func Eventually(ctx context.Context, t testing.TB, condition func() bool, tick t // EventuallyShort is a convenience function that runs Eventually with // IntervalFast and times out after WaitShort. -func EventuallyShort(t testing.TB, condition func() bool) bool { +func EventuallyShort(t testing.TB, condition func(context.Context) bool) bool { ctx, cancel := context.WithTimeout(context.Background(), WaitShort) defer cancel() return Eventually(ctx, t, condition, IntervalFast) @@ -46,7 +58,7 @@ func EventuallyShort(t testing.TB, condition func() bool) bool { // EventuallyMedium is a convenience function that runs Eventually with // IntervalMedium and times out after WaitMedium. -func EventuallyMedium(t testing.TB, condition func() bool) bool { +func EventuallyMedium(t testing.TB, condition func(context.Context) bool) bool { ctx, cancel := context.WithTimeout(context.Background(), WaitMedium) defer cancel() return Eventually(ctx, t, condition, IntervalMedium) @@ -54,7 +66,7 @@ func EventuallyMedium(t testing.TB, condition func() bool) bool { // EventuallyLong is a convenience function that runs Eventually with // IntervalSlow and times out after WaitLong. -func EventuallyLong(t testing.TB, condition func() bool) bool { +func EventuallyLong(t testing.TB, condition func(context.Context) bool) bool { ctx, cancel := context.WithTimeout(context.Background(), WaitLong) defer cancel() return Eventually(ctx, t, condition, IntervalSlow) diff --git a/testutil/eventually_test.go b/testutil/eventually_test.go index 83c439a165a99..2ba4b134a91c4 100644 --- a/testutil/eventually_test.go +++ b/testutil/eventually_test.go @@ -19,7 +19,7 @@ func TestEventually(t *testing.T) { t.Run("OK", func(t *testing.T) { t.Parallel() state := 0 - condition := func() bool { + condition := func(_ context.Context) bool { defer func() { state++ }() @@ -27,18 +27,18 @@ func TestEventually(t *testing.T) { } ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) defer cancel() - assert.True(t, testutil.Eventually(ctx, t, condition, testutil.IntervalFast)) + testutil.Eventually(ctx, t, condition, testutil.IntervalFast) }) t.Run("Timeout", func(t *testing.T) { t.Parallel() - condition := func() bool { + condition := func(_ context.Context) bool { return false } ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) defer cancel() mockT := new(testing.T) - assert.False(t, testutil.Eventually(ctx, mockT, condition, testutil.IntervalFast)) + testutil.Eventually(ctx, mockT, condition, testutil.IntervalFast) assert.True(t, mockT.Failed()) }) @@ -47,24 +47,24 @@ func TestEventually(t *testing.T) { panicky := func() { mockT := new(testing.T) - condition := func() bool { return true } - assert.False(t, testutil.Eventually(context.Background(), mockT, condition, testutil.IntervalFast)) + condition := func(_ context.Context) bool { return true } + testutil.Eventually(context.Background(), mockT, condition, testutil.IntervalFast) } assert.Panics(t, panicky) }) t.Run("Short", func(t *testing.T) { t.Parallel() - assert.True(t, testutil.EventuallyShort(t, func() bool { return true })) + testutil.EventuallyShort(t, func(_ context.Context) bool { return true }) }) t.Run("Medium", func(t *testing.T) { t.Parallel() - assert.True(t, testutil.EventuallyMedium(t, func() bool { return true })) + testutil.EventuallyMedium(t, func(_ context.Context) bool { return true }) }) t.Run("Long", func(t *testing.T) { t.Parallel() - assert.True(t, testutil.EventuallyLong(t, func() bool { return true })) + testutil.EventuallyLong(t, func(_ context.Context) bool { return true }) }) } From 5eb64ec1e8f066b8078f3ee7d1cc9f70bd5deb93 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 5 Aug 2022 13:43:32 +0000 Subject: [PATCH 5/7] remove unused channel --- testutil/eventually.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/testutil/eventually.go b/testutil/eventually.go index c4f6f55aeaf67..a70697022e789 100644 --- a/testutil/eventually.go +++ b/testutil/eventually.go @@ -27,7 +27,6 @@ func Eventually(ctx context.Context, t testing.TB, condition func(context.Contex panic("developer error: must set deadline or timeout on ctx") } - ch := make(chan bool, 1) ticker := time.NewTicker(tick) defer ticker.Stop() for tick := ticker.C; ; { @@ -36,11 +35,7 @@ func Eventually(ctx context.Context, t testing.TB, condition func(context.Contex assert.NoError(t, ctx.Err(), "Eventually timed out") return false case <-tick: - tick = nil - ch <- condition(ctx) - case v := <-ch: - if v { - close(ch) + if condition(ctx) { return true } tick = ticker.C From 41f678b786b9a73314792ef7f99e98c14bc7a1ef Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 5 Aug 2022 15:04:34 +0100 Subject: [PATCH 6/7] Update testutil/eventually.go Co-authored-by: Mathias Fredriksson --- testutil/eventually.go | 1 - 1 file changed, 1 deletion(-) diff --git a/testutil/eventually.go b/testutil/eventually.go index a70697022e789..e92929512ff10 100644 --- a/testutil/eventually.go +++ b/testutil/eventually.go @@ -38,7 +38,6 @@ func Eventually(ctx context.Context, t testing.TB, condition func(context.Contex if condition(ctx) { return true } - tick = ticker.C } } } From 759380266e1d653f5a3742ec5038fefeb73fbf4e Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 5 Aug 2022 14:44:49 +0000 Subject: [PATCH 7/7] check ctx after tick --- testutil/eventually.go | 1 + 1 file changed, 1 insertion(+) diff --git a/testutil/eventually.go b/testutil/eventually.go index e92929512ff10..d4f482abf1608 100644 --- a/testutil/eventually.go +++ b/testutil/eventually.go @@ -35,6 +35,7 @@ func Eventually(ctx context.Context, t testing.TB, condition func(context.Contex assert.NoError(t, ctx.Err(), "Eventually timed out") return false case <-tick: + assert.NoError(t, ctx.Err(), "Eventually timed out") if condition(ctx) { return true }