test(all): Improve and fix subtests with parallell/nonparallel parents

coder · mafredri · Apr 17, 2023 · Apr 17, 2023 · Apr 17, 2023 · Apr 17, 2023
commit ec0cc6b6bc210d88caf26a552f59e25c2f215981
diff --git a/agent/reaper/reaper_test.go b/agent/reaper/reaper_test.go
@@ -30,42 +30,42 @@ func TestReap(t *testing.T) {
 	// OK checks that's the reaper is successfully reaping
 	// exited processes and passing the PIDs through the shared
 	// channel.
+}
 
-	//nolint:paralleltest // Signal handling.
-	t.Run("OK", func(t *testing.T) {
-		pids := make(reap.PidCh, 1)
-		err := reaper.ForkReap(
-			reaper.WithPIDCallback(pids),
-			// Provide some argument that immediately exits.
-			reaper.WithExecArgs("/bin/sh", "-c", "exit 0"),
-		)
-		require.NoError(t, err)
+//nolint:paralleltest // Signal handling.
+func TestReap_OK(t *testing.T) {
+	pids := make(reap.PidCh, 1)
+	err := reaper.ForkReap(
+		reaper.WithPIDCallback(pids),
+		// Provide some argument that immediately exits.
+		reaper.WithExecArgs("/bin/sh", "-c", "exit 0"),
+	)
+	require.NoError(t, err)
 
-		cmd := exec.Command("tail", "-f", "/dev/null")
-		err = cmd.Start()
-		require.NoError(t, err)
+	cmd := exec.Command("tail", "-f", "/dev/null")
+	err = cmd.Start()
+	require.NoError(t, err)
 
-		cmd2 := exec.Command("tail", "-f", "/dev/null")
-		err = cmd2.Start()
-		require.NoError(t, err)
+	cmd2 := exec.Command("tail", "-f", "/dev/null")
+	err = cmd2.Start()
+	require.NoError(t, err)
 
-		err = cmd.Process.Kill()
-		require.NoError(t, err)
+	err = cmd.Process.Kill()
+	require.NoError(t, err)
 
-		err = cmd2.Process.Kill()
-		require.NoError(t, err)
+	err = cmd2.Process.Kill()
+	require.NoError(t, err)
 
-		expectedPIDs := []int{cmd.Process.Pid, cmd2.Process.Pid}
+	expectedPIDs := []int{cmd.Process.Pid, cmd2.Process.Pid}
 
-		for i := 0; i < len(expectedPIDs); i++ {
-			select {
-			case <-time.After(testutil.WaitShort):
-				t.Fatalf("Timed out waiting for process")
-			case pid := <-pids:
-				require.Contains(t, expectedPIDs, pid)
-			}
+	for i := 0; i < len(expectedPIDs); i++ {
+		select {
+		case <-time.After(testutil.WaitShort):
+			t.Fatalf("Timed out waiting for process")
+		case pid := <-pids:
+			require.Contains(t, expectedPIDs, pid)
 		}
-	})
+	}
 }
 
 //nolint:paralleltest // Signal handling.

diff --git a/cli/portforward_test.go b/cli/portforward_test.go
@@ -21,10 +21,8 @@ import (
 	"github.com/coder/coder/testutil"
 )
 
+//nolint:paralleltest // Non-parallel subtests.
 func TestPortForward(t *testing.T) {
-	t.Parallel()
-	t.Skip("These tests flake... a lot. It seems related to the Tailscale change, but all other tests pass...")
-
 	t.Run("None", func(t *testing.T) {
 		t.Parallel()
 
@@ -116,7 +114,7 @@ func TestPortForward(t *testing.T) {
 		workspace = runAgent(t, client, user.UserID)
 	)
 
-	for _, c := range cases { //nolint:paralleltest // the `c := c` confuses the linter
+	for _, c := range cases {
 		c := c
 		// Delay parallel tests here because setupLocal reserves
 		// a free open port which is not guaranteed to be free
@@ -164,7 +162,6 @@ func TestPortForward(t *testing.T) {
 				require.ErrorIs(t, err, context.Canceled)
 			})
 
-			//nolint:paralleltest
 			t.Run("TwoPorts", func(t *testing.T) {
 				var (
 					p1 = setupTestListener(t, c.setupRemote(t))
@@ -215,7 +212,6 @@ func TestPortForward(t *testing.T) {
 	}
 
 	// Test doing TCP and UDP at the same time.
-	//nolint:paralleltest
 	t.Run("All", func(t *testing.T) {
 		var (
 			dials = []addr{}

diff --git a/cli/server_test.go b/cli/server_test.go
@@ -104,35 +104,7 @@ func TestReadGitAuthProvidersFromEnv(t *testing.T) {
 	})
 }
 
-// This cannot be ran in parallel because it uses a signal.
-// nolint:tparallel,paralleltest
 func TestServer(t *testing.T) {
-	t.Run("Production", func(t *testing.T) {
-		if runtime.GOOS != "linux" || testing.Short() {
-			// Skip on non-Linux because it spawns a PostgreSQL instance.
-			t.SkipNow()
-		}
-		connectionURL, closeFunc, err := postgres.Open()
-		require.NoError(t, err)
-		defer closeFunc()
-
-		// Postgres + race detector + CI = slow.
-		ctx := testutil.Context(t, testutil.WaitSuperLong*3)
-
-		inv, cfg := clitest.New(t,
-			"server",
-			"--http-address", ":0",
-			"--access-url", "http://example.com",
-			"--postgres-url", connectionURL,
-			"--cache-dir", t.TempDir(),
-		)
-		clitest.Start(t, inv.WithContext(ctx))
-		accessURL := waitAccessURL(t, cfg)
-		client := codersdk.New(accessURL)
-
-		_, err = client.CreateFirstUser(ctx, coderdtest.FirstUserParams)
-		require.NoError(t, err)
-	})
 	t.Run("BuiltinPostgres", func(t *testing.T) {
 		t.Parallel()
 		if testing.Short() {
@@ -855,38 +827,6 @@ func TestServer(t *testing.T) {
 		})
 	})
 
-	// This cannot be ran in parallel because it uses a signal.
-	//nolint:paralleltest
-	t.Run("Shutdown", func(t *testing.T) {
-		if runtime.GOOS == "windows" {
-			// Sending interrupt signal isn't supported on Windows!
-			t.SkipNow()
-		}
-		ctx, cancelFunc := context.WithCancel(context.Background())
-		defer cancelFunc()
-
-		root, cfg := clitest.New(t,
-			"server",
-			"--in-memory",
-			"--http-address", ":0",
-			"--access-url", "http://example.com",
-			"--provisioner-daemons", "1",
-			"--cache-dir", t.TempDir(),
-		)
-		serverErr := make(chan error, 1)
-		go func() {
-			serverErr <- root.WithContext(ctx).Run()
-		}()
-		_ = waitAccessURL(t, cfg)
-		currentProcess, err := os.FindProcess(os.Getpid())
-		require.NoError(t, err)
-		err = currentProcess.Signal(os.Interrupt)
-		require.NoError(t, err)
-		// We cannot send more signals here, because it's possible Coder
-		// has already exited, which could cause the test to fail due to interrupt.
-		err = <-serverErr
-		require.NoError(t, err)
-	})
 	t.Run("TracerNoLeak", func(t *testing.T) {
 		t.Parallel()
 
@@ -1493,6 +1433,66 @@ func TestServer(t *testing.T) {
 	})
 }
 
+//nolint:paralleltest // This test spawns or connects to an existing PostgreSQL instance.
+func TestServer_Production(t *testing.T) {
+	if runtime.GOOS != "linux" || testing.Short() {
+		// Skip on non-Linux because it spawns a PostgreSQL instance.
+		t.SkipNow()
+	}
+	connectionURL, closeFunc, err := postgres.Open()
+	require.NoError(t, err)
+	defer closeFunc()
+
+	// Postgres + race detector + CI = slow.
+	ctx := testutil.Context(t, testutil.WaitSuperLong*3)
+
+	inv, cfg := clitest.New(t,
+		"server",
+		"--http-address", ":0",
+		"--access-url", "http://example.com",
+		"--postgres-url", connectionURL,
+		"--cache-dir", t.TempDir(),
+	)
+	clitest.Start(t, inv.WithContext(ctx))
+	accessURL := waitAccessURL(t, cfg)
+	client := codersdk.New(accessURL)
+
+	_, err = client.CreateFirstUser(ctx, coderdtest.FirstUserParams)
+	require.NoError(t, err)
+}
+
+//nolint:paralleltest // This test cannot be run in parallel due to signal handling.
+func TestServer_Shutdown(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		// Sending interrupt signal isn't supported on Windows!
+		t.SkipNow()
+	}
+	ctx, cancelFunc := context.WithCancel(context.Background())
+	defer cancelFunc()
+
+	root, cfg := clitest.New(t,
+		"server",
+		"--in-memory",
+		"--http-address", ":0",
+		"--access-url", "http://example.com",
+		"--provisioner-daemons", "1",
+		"--cache-dir", t.TempDir(),
+	)
+	serverErr := make(chan error, 1)
+	go func() {
+		serverErr <- root.WithContext(ctx).Run()
+	}()
+	_ = waitAccessURL(t, cfg)
+	currentProcess, err := os.FindProcess(os.Getpid())
+	require.NoError(t, err)
+	err = currentProcess.Signal(os.Interrupt)
+	require.NoError(t, err)
+	// We cannot send more signals here, because it's possible Coder
+	// has already exited, which could cause the test to fail due to interrupt.
+	err = <-serverErr
+	require.NoError(t, err)
+}
+
 func generateTLSCertificate(t testing.TB, commonName ...string) (certPath, keyPath string) {
 	dir := t.TempDir()
 

diff --git a/cli/templatepush_test.go b/cli/templatepush_test.go
@@ -156,8 +156,6 @@ func TestTemplatePush(t *testing.T) {
 		require.Equal(t, "example", templateVersions[1].Name)
 	})
 
-	// This test modifies the working directory.
-	//nolint:paralleltest
 	t.Run("UseWorkingDir", func(t *testing.T) {
 		t.Parallel()
 

diff --git a/coderd/users_test.go b/coderd/users_test.go
@@ -1570,7 +1570,6 @@ func TestPaginatedUsers(t *testing.T) {
 		{name: "username search", limit: 3, allUsers: specialUsers, opt: usernameSearch},
 		{name: "username search", limit: 3, allUsers: specialUsers, opt: usernameSearch},
 	}
-	//nolint:paralleltest // Does not detect range value.
 	for _, tt := range tests {
 		tt := tt
 		t.Run(fmt.Sprintf("%s %d", tt.name, tt.limit), func(t *testing.T) {

diff --git a/cryptorand/strings_test.go b/cryptorand/strings_test.go
@@ -91,7 +91,6 @@ func TestStringCharset(t *testing.T) {
 		},
 	}
 
-	//nolint:paralleltest
 	for _, test := range tests {
 		test := test
 		t.Run(test.Name, func(t *testing.T) {

diff --git a/scaletest/agentconn/run_test.go b/scaletest/agentconn/run_test.go
@@ -58,44 +58,6 @@ func Test_Runner(t *testing.T) {
 		require.NotContains(t, logStr, "Waiting for ")
 	})
 
-	//nolint:paralleltest // Measures timing as part of the test.
-	t.Run("Direct+Hold", func(t *testing.T) {
-		client, agentID := setupRunnerTest(t)
-
-		runner := agentconn.NewRunner(client, agentconn.Config{
-			AgentID:        agentID,
-			ConnectionMode: agentconn.ConnectionModeDirect,
-			HoldDuration:   httpapi.Duration(testutil.WaitShort),
-		})
-
-		ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
-		defer cancel()
-
-		logs := bytes.NewBuffer(nil)
-		start := time.Now()
-		err := runner.Run(ctx, "1", logs)
-		logStr := logs.String()
-		t.Log("Runner logs:\n\n" + logStr)
-		require.NoError(t, err)
-
-		require.WithinRange(t,
-			time.Now(),
-			start.Add(testutil.WaitShort-time.Second),
-			start.Add(testutil.WaitShort+5*time.Second),
-		)
-
-		require.Contains(t, logStr, "Opening connection to workspace agent")
-		require.Contains(t, logStr, "Using direct connection")
-		require.Contains(t, logStr, "Disco ping attempt 1/10...")
-		require.Contains(t, logStr, "Direct connection check 1/30...")
-		require.Contains(t, logStr, "Connection established")
-		require.Contains(t, logStr, "Verify connection attempt 1/30...")
-		require.Contains(t, logStr, "Connection verified")
-		require.NotContains(t, logStr, "Performing initial service connections")
-		require.NotContains(t, logStr, "Starting connection loops")
-		require.Contains(t, logStr, fmt.Sprintf("Waiting for %s", testutil.WaitShort))
-	})
-
 	t.Run("Derp+ServicesNoHold", func(t *testing.T) {
 		t.Parallel()
 
@@ -143,8 +105,49 @@ func Test_Runner(t *testing.T) {
 		require.EqualValues(t, 1, service1Count())
 		require.EqualValues(t, 1, service2Count())
 	})
+}
+
+//nolint:paralleltest // Measures timing as part of the test.
+func Test_Runner_Timing(t *testing.T) {
+	//nolint:paralleltest
+	t.Run("Direct+Hold", func(t *testing.T) {
+		client, agentID := setupRunnerTest(t)
+
+		runner := agentconn.NewRunner(client, agentconn.Config{
+			AgentID:        agentID,
+			ConnectionMode: agentconn.ConnectionModeDirect,
+			HoldDuration:   httpapi.Duration(testutil.WaitShort),
+		})
+
+		ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
+		defer cancel()
+
+		logs := bytes.NewBuffer(nil)
+		start := time.Now()
+		err := runner.Run(ctx, "1", logs)
+		logStr := logs.String()
+		t.Log("Runner logs:\n\n" + logStr)
+		require.NoError(t, err)
+
+		require.WithinRange(t,
+			time.Now(),
+			start.Add(testutil.WaitShort-time.Second),
+			start.Add(testutil.WaitShort+5*time.Second),
+		)
+
+		require.Contains(t, logStr, "Opening connection to workspace agent")
+		require.Contains(t, logStr, "Using direct connection")
+		require.Contains(t, logStr, "Disco ping attempt 1/10...")
+		require.Contains(t, logStr, "Direct connection check 1/30...")
+		require.Contains(t, logStr, "Connection established")
+		require.Contains(t, logStr, "Verify connection attempt 1/30...")
+		require.Contains(t, logStr, "Connection verified")
+		require.NotContains(t, logStr, "Performing initial service connections")
+		require.NotContains(t, logStr, "Starting connection loops")
+		require.Contains(t, logStr, fmt.Sprintf("Waiting for %s", testutil.WaitShort))
+	})
 
-	//nolint:paralleltest // Measures timing as part of the test.
+	//nolint:paralleltest
 	t.Run("Derp+Hold+Services", func(t *testing.T) {
 		client, agentID := setupRunnerTest(t)
 		service1URL, service1Count := testServer(t)