From 9cb5990ac716a1b58ddebc4bee500d56a15e8384 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 22 Jul 2022 13:40:04 +0300 Subject: [PATCH 1/4] fix: Remove use of `require` in `require.Eventually` in tests Because require uses `t.FailNow()` and `require.Eventually` runs the function in a goroutine, which is not allowed. --- agent/agent_test.go | 4 +++- cli/resetpassword_test.go | 18 ++++++++---------- cli/server_test.go | 16 +++++++--------- coderd/coderd_test.go | 3 +-- coderd/coderdtest/coderdtest.go | 21 +++++++++++++++------ coderd/provisionerdaemons_test.go | 4 ++-- coderd/templateversions_test.go | 30 ++++++++++++++++++++---------- coderd/workspacebuilds_test.go | 3 +-- 8 files changed, 57 insertions(+), 42 deletions(-) diff --git a/agent/agent_test.go b/agent/agent_test.go index c10bfd3157af9..e8ebe93b9a4bf 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -224,7 +224,9 @@ func TestAgent(t *testing.T) { if runtime.GOOS == "windows" { // Windows uses UTF16! 🪟🪟🪟 content, _, err = transform.Bytes(unicode.UTF16(unicode.LittleEndian, unicode.UseBOM).NewDecoder(), content) - require.NoError(t, err) + if !assert.NoError(t, err) { + return false + } } gotContent = string(content) return true diff --git a/cli/resetpassword_test.go b/cli/resetpassword_test.go index 960cebe52bdd8..3fe0ab8551bbb 100644 --- a/cli/resetpassword_test.go +++ b/cli/resetpassword_test.go @@ -44,17 +44,15 @@ func TestResetPassword(t *testing.T) { err = serverCmd.ExecuteContext(ctx) assert.ErrorIs(t, err, context.Canceled) }() - var client *codersdk.Client + var rawURL string require.Eventually(t, func() bool { - rawURL, err := cfg.URL().Read() - if err != nil { - return false - } - accessURL, err := url.Parse(rawURL) - require.NoError(t, err) - client = codersdk.New(accessURL) - return true - }, 15*time.Second, 25*time.Millisecond) + rawURL, err = cfg.URL().Read() + return err == nil && rawURL != "" + }, 15*time.Minute, 25*time.Millisecond) + accessURL, err := url.Parse(rawURL) + require.NoError(t, err) + client := codersdk.New(accessURL) + _, err = client.CreateFirstUser(ctx, codersdk.CreateFirstUserRequest{ Email: email, Username: username, diff --git a/cli/server_test.go b/cli/server_test.go index 824f991aa436a..6d924088439e7 100644 --- a/cli/server_test.go +++ b/cli/server_test.go @@ -50,17 +50,15 @@ func TestServer(t *testing.T) { go func() { errC <- root.ExecuteContext(ctx) }() - var client *codersdk.Client + var rawURL string require.Eventually(t, func() bool { - rawURL, err := cfg.URL().Read() - if err != nil { - return false - } - accessURL, err := url.Parse(rawURL) - assert.NoError(t, err) - client = codersdk.New(accessURL) - return true + rawURL, err = cfg.URL().Read() + return err == nil && rawURL != "" }, time.Minute, 50*time.Millisecond) + accessURL, err := url.Parse(rawURL) + require.NoError(t, err) + client := codersdk.New(accessURL) + _, err = client.CreateFirstUser(ctx, codersdk.CreateFirstUserRequest{ Email: "some@one.com", Username: "example", diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index 448d6d04a3524..d7103d39f9d89 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -153,8 +153,7 @@ func TestAuthorizeAllEndpoints(t *testing.T) { // so we wait for it to occur. require.Eventually(t, func() bool { provisionerds, err := client.ProvisionerDaemons(ctx) - require.NoError(t, err) - return len(provisionerds) > 0 + return assert.NoError(t, err) && len(provisionerds) > 0 }, time.Second*10, time.Second) provisionerds, err := client.ProvisionerDaemons(ctx) diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index 98fd6d1a33b7a..eb5d089201a63 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -353,7 +353,8 @@ func CreateWorkspaceBuild( t *testing.T, client *codersdk.Client, workspace codersdk.Workspace, - transition database.WorkspaceTransition) codersdk.WorkspaceBuild { + transition database.WorkspaceTransition, +) codersdk.WorkspaceBuild { req := codersdk.CreateWorkspaceBuildRequest{ Transition: codersdk.WorkspaceTransition(transition), } @@ -397,36 +398,44 @@ func UpdateTemplateVersion(t *testing.T, client *codersdk.Client, organizationID // AwaitTemplateImportJob awaits for an import job to reach completed status. func AwaitTemplateVersionJob(t *testing.T, client *codersdk.Client, version uuid.UUID) codersdk.TemplateVersion { + t.Helper() + t.Logf("waiting for template version job %s", version) var templateVersion codersdk.TemplateVersion require.Eventually(t, func() bool { var err error templateVersion, err = client.TemplateVersion(context.Background(), version) - require.NoError(t, err) - return templateVersion.Job.CompletedAt != nil + return assert.NoError(t, err) && templateVersion.Job.CompletedAt != nil }, 5*time.Second, 25*time.Millisecond) return templateVersion } // AwaitWorkspaceBuildJob waits for a workspace provision job to reach completed status. func AwaitWorkspaceBuildJob(t *testing.T, client *codersdk.Client, build uuid.UUID) codersdk.WorkspaceBuild { + t.Helper() + + 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.NoError(t, err) - return workspaceBuild.Job.CompletedAt != nil + return assert.NoError(t, err) && workspaceBuild.Job.CompletedAt != nil }, 5*time.Second, 25*time.Millisecond) return workspaceBuild } // AwaitWorkspaceAgents waits for all resources with agents to be connected. func AwaitWorkspaceAgents(t *testing.T, client *codersdk.Client, build uuid.UUID) []codersdk.WorkspaceResource { + t.Helper() + + t.Logf("waiting for workspace agents (build %s)", build) var resources []codersdk.WorkspaceResource require.Eventually(t, func() bool { var err error resources, err = client.WorkspaceResourcesByBuild(context.Background(), build) - require.NoError(t, err) + if !assert.NoError(t, err) { + return false + } for _, resource := range resources { for _, agent := range resource.Agents { if agent.Status != codersdk.WorkspaceAgentConnected { diff --git a/coderd/provisionerdaemons_test.go b/coderd/provisionerdaemons_test.go index 7d1434ba9f24b..1a89bb4e701a8 100644 --- a/coderd/provisionerdaemons_test.go +++ b/coderd/provisionerdaemons_test.go @@ -7,6 +7,7 @@ import ( "testing" "time" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/coder/coder/coderd/coderdtest" @@ -39,8 +40,7 @@ func TestProvisionerDaemons(t *testing.T) { require.Eventually(t, func() bool { var err error version, err = client.TemplateVersion(context.Background(), version.ID) - require.NoError(t, err) - return version.Job.Error != "" + return assert.NoError(t, err) && version.Job.Error != "" }, 5*time.Second, 25*time.Millisecond) }) } diff --git a/coderd/templateversions_test.go b/coderd/templateversions_test.go index 8579b5e5daffb..a1b987bfcc7a2 100644 --- a/coderd/templateversions_test.go +++ b/coderd/templateversions_test.go @@ -115,7 +115,9 @@ func TestPatchCancelTemplateVersion(t *testing.T) { require.Eventually(t, func() bool { var err error version, err = client.TemplateVersion(context.Background(), version.ID) - require.NoError(t, err) + if !assert.NoError(t, err) { + return false + } t.Logf("Status: %s", version.Job.Status) return version.Job.Status == codersdk.ProvisionerJobRunning }, 5*time.Second, 25*time.Millisecond) @@ -148,7 +150,9 @@ func TestPatchCancelTemplateVersion(t *testing.T) { require.Eventually(t, func() bool { var err error version, err = client.TemplateVersion(context.Background(), version.ID) - require.NoError(t, err) + if !assert.NoError(t, err) { + return false + } t.Logf("Status: %s", version.Job.Status) return version.Job.Status == codersdk.ProvisionerJobRunning }, 5*time.Second, 25*time.Millisecond) @@ -536,9 +540,7 @@ func TestTemplateVersionDryRun(t *testing.T) { // Wait for the job to complete require.Eventually(t, func() bool { job, err := client.TemplateVersionDryRun(ctx, version.ID, job.ID) - assert.NoError(t, err) - - return job.Status == codersdk.ProvisionerJobSucceeded + return assert.NoError(t, err) && job.Status == codersdk.ProvisionerJobSucceeded }, 5*time.Second, 25*time.Millisecond) <-logsDone @@ -588,7 +590,8 @@ func TestTemplateVersionDryRun(t *testing.T) { { Type: &proto.Provision_Response_Log{ Log: &proto.Log{}, - }}, + }, + }, { Type: &proto.Provision_Response_Complete{ Complete: &proto.Provision_Complete{}, @@ -609,7 +612,9 @@ func TestTemplateVersionDryRun(t *testing.T) { require.Eventually(t, func() bool { job, err := client.TemplateVersionDryRun(context.Background(), version.ID, job.ID) - assert.NoError(t, err) + if !assert.NoError(t, err) { + return false + } t.Logf("Status: %s", job.Status) return job.Status == codersdk.ProvisionerJobPending @@ -620,7 +625,9 @@ func TestTemplateVersionDryRun(t *testing.T) { require.Eventually(t, func() bool { job, err := client.TemplateVersionDryRun(context.Background(), version.ID, job.ID) - assert.NoError(t, err) + if !assert.NoError(t, err) { + return false + } t.Logf("Status: %s", job.Status) return job.Status == codersdk.ProvisionerJobCanceling @@ -642,7 +649,9 @@ func TestTemplateVersionDryRun(t *testing.T) { require.Eventually(t, func() bool { job, err := client.TemplateVersionDryRun(context.Background(), version.ID, job.ID) - assert.NoError(t, err) + if !assert.NoError(t, err) { + return false + } t.Logf("Status: %s", job.Status) return job.Status == codersdk.ProvisionerJobSucceeded @@ -666,7 +675,8 @@ func TestTemplateVersionDryRun(t *testing.T) { { Type: &proto.Provision_Response_Log{ Log: &proto.Log{}, - }}, + }, + }, { Type: &proto.Provision_Response_Complete{ Complete: &proto.Provision_Complete{}, diff --git a/coderd/workspacebuilds_test.go b/coderd/workspacebuilds_test.go index c9f24d0442c14..9c4894ff704fb 100644 --- a/coderd/workspacebuilds_test.go +++ b/coderd/workspacebuilds_test.go @@ -221,8 +221,7 @@ func TestPatchCancelWorkspaceBuild(t *testing.T) { require.Eventually(t, func() bool { var err error build, err = client.WorkspaceBuild(context.Background(), workspace.LatestBuild.ID) - require.NoError(t, err) - return build.Job.Status == codersdk.ProvisionerJobRunning + return assert.NoError(t, err) && build.Job.Status == codersdk.ProvisionerJobRunning }, 5*time.Second, 25*time.Millisecond) err := client.CancelWorkspaceBuild(context.Background(), build.ID) require.NoError(t, err) From 93d4fb336c06fbfb821ad31175ab655b72607631 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 22 Jul 2022 14:28:16 +0300 Subject: [PATCH 2/4] Update cli/resetpassword_test.go Co-authored-by: Cian Johnston --- cli/resetpassword_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/resetpassword_test.go b/cli/resetpassword_test.go index 3fe0ab8551bbb..3bae7752e5a4f 100644 --- a/cli/resetpassword_test.go +++ b/cli/resetpassword_test.go @@ -48,7 +48,7 @@ func TestResetPassword(t *testing.T) { require.Eventually(t, func() bool { rawURL, err = cfg.URL().Read() return err == nil && rawURL != "" - }, 15*time.Minute, 25*time.Millisecond) + }, 15*time.Second, 25*time.Millisecond) accessURL, err := url.Parse(rawURL) require.NoError(t, err) client := codersdk.New(accessURL) From 9478f17b60455c99bce335457ffebd0324104540 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 22 Jul 2022 17:36:36 +0300 Subject: [PATCH 3/4] feat: Add ruleguard for require.Eventually --- scripts/rules.go | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/scripts/rules.go b/scripts/rules.go index 67aae45ddd086..16917101d16e9 100644 --- a/scripts/rules.go +++ b/scripts/rules.go @@ -59,6 +59,17 @@ func doNotCallTFailNowInsideGoroutine(m dsl.Matcher) { Where(m["require"].Text == "require"). Report("Do not call functions that may call t.FailNow in a goroutine, as this can cause data races (see testing.go:834)") + // require.Eventually runs the function in a goroutine. + m.Match(` + require.Eventually(t, func() bool { + $*_ + $require.$_($*_) + $*_ + }, $*_)`). + At(m["require"]). + Where(m["require"].Text == "require"). + Report("Do not call functions that may call t.FailNow in a goroutine, as this can cause data races (see testing.go:834)") + m.Match(` go func($*_){ $*_ @@ -90,10 +101,10 @@ func InTx(m dsl.Matcher) { At(m["f"]). Report("Do not use the database directly within the InTx closure. Use '$y' instead of '$x'.") - //When using a tx closure, ensure that if you pass the db to another - //function inside the closure, it is the tx. - //This will miss more complex cases such as passing the db as apart - //of another struct. + // When using a tx closure, ensure that if you pass the db to another + // function inside the closure, it is the tx. + // This will miss more complex cases such as passing the db as apart + // of another struct. m.Match(` $x.InTx(func($y database.Store) error { $*_ From f6167b6d585d51307694404981af0a1193265dc6 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 22 Jul 2022 17:38:18 +0300 Subject: [PATCH 4/4] fix: Issue caught by ruleguard --- coderd/devtunnel/tunnel_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/coderd/devtunnel/tunnel_test.go b/coderd/devtunnel/tunnel_test.go index a012922f801fd..47703cacf81de 100644 --- a/coderd/devtunnel/tunnel_test.go +++ b/coderd/devtunnel/tunnel_test.go @@ -78,7 +78,9 @@ func TestTunnel(t *testing.T) { require.Eventually(t, func() bool { res, err := fTunServer.requestHTTP() - require.NoError(t, err) + if !assert.NoError(t, err) { + return false + } defer res.Body.Close() _, _ = io.Copy(io.Discard, res.Body)