fix(test): Remove uses of t.FailNow in goroutines (#1978)

It is documented as illegal to use t.FailNow in a goroutine.
Uses of require assertions call t.FailNow. This may have led to
inconsistent test results as some assertions were subject to
undefined behavior. Note that it is still allowed to call
assert assertions in goroutines.

See: https://pkg.go.dev/testing#T.FailNow

internal/cmd/commands/server/worker_shutdown_reload_test.go

@@ -98,37 +98,31 @@ func TestServer_ShutdownWorker(t *testing.T) {
 		UseDevAuthMethod:  true,
 		UseDevTarget:      true,
 	})
-	controllerDoneCh := make(chan struct{})
-	defer func() {
+	t.Cleanup(func() {
 		if controllerCmd.DevDatabaseCleanupFunc != nil {
 			require.NoError(controllerCmd.DevDatabaseCleanupFunc())
 		}
-	}()
+	})
 	controllerCmd.presetConfig = atomic.NewString(fmt.Sprintf(shutdownReloadControllerConfig, controllerCmd.DatabaseUrl, controllerKey, workerAuthKey))
 
+	// Use code channel so that we can use test assertions on the returned integer.
+	// It is illegal to call `t.FailNow()` from a goroutine.
+	// https://pkg.go.dev/testing#T.FailNow
+	controllerCodeChan := make(chan int)
 	go func() {
-		defer close(controllerDoneCh)
-		if code := controllerCmd.Run(nil); code != 0 {
-			output := controllerCmd.UI.(*cli.MockUi).ErrorWriter.String() + controllerCmd.UI.(*cli.MockUi).OutputWriter.String()
-			require.FailNow(output, "command exited with non-zero error code")
-		}
+		controllerCodeChan <- controllerCmd.Run(nil)
 	}()
 
 	waitCh(t, controllerCmd.startedCh)
 
 	// Start the worker
 	workerCmd := testServerCommand(t, testServerCommandOpts{})
-	workerDoneCh := make(chan struct{})
 	workerCmd.presetConfig = atomic.NewString(fmt.Sprintf(shutdownReloadWorkerConfig, workerAuthKey))
 
+	workerCodeChan := make(chan int)
 	go func() {
-		defer close(workerDoneCh)
-		if code := workerCmd.Run(nil); code != 0 {
-			output := workerCmd.UI.(*cli.MockUi).ErrorWriter.String() + workerCmd.UI.(*cli.MockUi).OutputWriter.String()
-			require.FailNow(output, "command exited with non-zero error code")
-		}
+		workerCodeChan <- workerCmd.Run(nil)
 	}()
-
 	waitCh(t, workerCmd.startedCh)
 
 	// Set up the target
@@ -159,7 +153,10 @@ func TestServer_ShutdownWorker(t *testing.T) {
 
 	// Now, shut the worker down.
 	close(workerCmd.ShutdownCh)
-	waitCh(t, workerDoneCh)
+	if <-workerCodeChan != 0 {
+		output := workerCmd.UI.(*cli.MockUi).ErrorWriter.String() + workerCmd.UI.(*cli.MockUi).OutputWriter.String()
+		require.FailNow(output, "command exited with non-zero error code")
+	}
 
 	// Connection should fail, and the session should be closed on the DB.
 	sConn.TestSendRecvFail(t)
@@ -167,7 +164,10 @@ func TestServer_ShutdownWorker(t *testing.T) {
 
 	// We're done! Shutdown the controller, and that's it.
 	close(controllerCmd.ShutdownCh)
-	waitCh(t, controllerDoneCh)
+	if <-controllerCodeChan != 0 {
+		output := controllerCmd.UI.(*cli.MockUi).ErrorWriter.String() + controllerCmd.UI.(*cli.MockUi).OutputWriter.String()
+		require.FailNow(output, "command exited with non-zero error code")
+	}
 }
 
 // largely copied from controller/testing.go

internal/cmd/ops/server_test.go

@@ -411,10 +411,17 @@ func TestShutdown(t *testing.T) {
 				require.NoError(t, err)
 
 				s1 := &http.Server{}
+				// Use error channel so that we can use test assertions on the returned error.
+				// It is illegal to call `t.FailNow()` from a goroutine.
+				// https://pkg.go.dev/testing#T.FailNow
+				errChan := make(chan error)
 				go func() {
-					require.ErrorIs(t, s1.Serve(l1), http.ErrServerClosed)
+					errChan <- s1.Serve(l1)
 				}()
-
+				t.Cleanup(func() {
+					// Will block until we stopped serving
+					require.ErrorIs(t, <-errChan, http.ErrServerClosed)
+				})
 				err = s1.Shutdown(context.Background())
 				require.NoError(t, err)
 
@@ -445,17 +452,28 @@ func TestShutdown(t *testing.T) {
 				require.NoError(t, err)
 
 				s1 := &http.Server{}
+				// Use error channel so that we can use test assertions on the returned error.
+				// It is illegal to call `t.FailNow()` from a goroutine.
+				// https://pkg.go.dev/testing#T.FailNow
+				s1ErrChan := make(chan error)
 				go func() {
-					require.ErrorIs(t, s1.Serve(l1), http.ErrServerClosed)
+					s1ErrChan <- s1.Serve(l1)
 				}()
+				t.Cleanup(func() {
+					require.ErrorIs(t, <-s1ErrChan, http.ErrServerClosed)
+				})
 
 				l2, err := net.Listen("tcp", "127.0.0.1:0")
 				require.NoError(t, err)
 
 				s2 := &http.Server{}
+				s2ErrChan := make(chan error)
 				go func() {
-					require.ErrorIs(t, s2.Serve(l2), http.ErrServerClosed)
+					s2ErrChan <- s2.Serve(l2)
 				}()
+				t.Cleanup(func() {
+					require.ErrorIs(t, <-s2ErrChan, http.ErrServerClosed)
+				})
 
 				return &Server{
 					bundles: []*opsBundle{