fix: Remove use of require in require.Eventually in tests (coder#3110)

mafredri · johnstcn · web-flow · commit 51dd1fde3b1b · 2022-07-22T20:02:49.000+03:00
* 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.

* feat: Add ruleguard for require.Eventually

Co-authored-by: Cian Johnston &lt;cian@coder.com&gt;
diff --git 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
@@ -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
+		rawURL, err = cfg.URL().Read()
+		return err == nil && rawURL != ""
 	}, 15*time.Second, 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
@@ -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
@@ -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
@@ -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/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)
 
diff --git 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
@@ -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
@@ -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)
diff --git 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 {
 		$*_
