From 2155527349eb9700156ee78116472226b78fd258 Mon Sep 17 00:00:00 2001 From: David Negstad Date: Mon, 21 Sep 2026 15:56:18 -0700 Subject: [PATCH 1/3] Improve integration test reliability Centralize standard test environment shutdown, wait for the complete volume deletion state, and add PTY failure diagnostics for future CI investigation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- internal/termpty/conpty_windows_test.go | 6 +- internal/termpty/pty_test.go | 88 ++++++++++++++++++- internal/termpty/pty_unix_test.go | 6 +- test/integration/container_controller_test.go | 6 +- .../container_network_tunnel_proxy_test.go | 61 ++++--------- test/integration/controllers_common_test.go | 2 +- .../integration/executable_controller_test.go | 5 +- test/integration/network_controller_test.go | 4 +- test/integration/standard_test_env.go | 76 ++++++++++++++-- .../v2_physical_container_controller_test.go | 2 +- ...hysical_container_image_controller_test.go | 3 +- ...sical_container_network_controller_test.go | 8 +- ...ysical_container_volume_controller_test.go | 16 +++- test/integration/volume_controller_test.go | 18 ++-- 14 files changed, 213 insertions(+), 88 deletions(-) diff --git a/internal/termpty/conpty_windows_test.go b/internal/termpty/conpty_windows_test.go index 726c870e..f6c3f182 100644 --- a/internal/termpty/conpty_windows_test.go +++ b/internal/termpty/conpty_windows_test.go @@ -41,8 +41,7 @@ func TestStartProcessWithTerminal_StdoutFromChild(t *testing.T) { sp := startTermchildWithPTY(t, ctx, "--print", "hello-from-child") - out, err := readUntil(ctx, sp.PTY, "hello-from-child") - require.NoError(t, err, "expected to see 'hello-from-child'; got: %q", out) + _ = requireReadUntil(t, ctx, sp, "hello-from-child") ei := awaitExit(t, ctx, sp.ExitHandler) require.NoError(t, ei.Err) @@ -88,8 +87,7 @@ func TestStartProcessWithTerminal_StderrFromChild(t *testing.T) { sp := startTermchildWithPTY(t, ctx, "--print-stderr", "boom-from-stderr") - out, err := readUntil(ctx, sp.PTY, "boom-from-stderr") - require.NoError(t, err, "expected to see 'boom-from-stderr'; got: %q", out) + _ = requireReadUntil(t, ctx, sp, "boom-from-stderr") ei := awaitExit(t, ctx, sp.ExitHandler) require.NoError(t, ei.Err) diff --git a/internal/termpty/pty_test.go b/internal/termpty/pty_test.go index 29465e49..d6282790 100644 --- a/internal/termpty/pty_test.go +++ b/internal/termpty/pty_test.go @@ -10,6 +10,7 @@ package termpty_test import ( "bytes" "context" + "fmt" "io" "os/exec" "strings" @@ -88,6 +89,9 @@ func readUntil(ctx context.Context, r io.Reader, target string) (string, error) } var accumulated bytes.Buffer + readAttempts := 0 + bytesRead := 0 + startedAt := time.Now() for { if strings.Contains(accumulated.String(), target) { return accumulated.String(), nil @@ -95,6 +99,7 @@ func readUntil(ctx context.Context, r io.Reader, target string) (string, error) buf := make([]byte, 4096) resCh := make(chan readResult, 1) + readAttempts++ go func() { n, err := r.Read(buf) if n > 0 { @@ -106,21 +111,100 @@ func readUntil(ctx context.Context, r io.Reader, target string) (string, error) select { case <-ctx.Done(): - return accumulated.String(), ctx.Err() + return accumulated.String(), &readUntilError{ + target: target, + readAttempts: readAttempts, + bytesRead: bytesRead, + elapsed: time.Since(startedAt), + err: ctx.Err(), + } case res := <-resCh: if len(res.data) > 0 { accumulated.Write(res.data) + bytesRead += len(res.data) } if res.err != nil { if strings.Contains(accumulated.String(), target) { return accumulated.String(), nil } - return accumulated.String(), res.err + return accumulated.String(), &readUntilError{ + target: target, + readAttempts: readAttempts, + bytesRead: bytesRead, + elapsed: time.Since(startedAt), + err: res.err, + } } } } } +type readUntilError struct { + target string + readAttempts int + bytesRead int + elapsed time.Duration + err error +} + +func (e *readUntilError) Error() string { + return fmt.Sprintf( + "PTY read failed before observing %q after %d attempts, %d bytes, and %s: %v", + e.target, + e.readAttempts, + e.bytesRead, + e.elapsed, + e.err, + ) +} + +func (e *readUntilError) Unwrap() error { + return e.err +} + +func requireReadUntil( + t *testing.T, + ctx context.Context, + sp *termpty.PseudoTerminalProcess, + target string, +) string { + t.Helper() + + out, readErr := readUntil(ctx, sp.PTY, target) + if readErr == nil { + return out + } + + exitSummary := "process exit was not observed" + exitTimer := time.NewTimer(drainExitTimeout) + defer exitTimer.Stop() + + select { + case <-sp.ExitHandler.Exited(): + exitInfo := sp.ExitHandler.ExitInfo() + exitSummary = fmt.Sprintf( + "process exit: PID=%d, exit code=%d, error=%v", + exitInfo.PID, + exitInfo.ExitCode, + exitInfo.Err, + ) + case <-ctx.Done(): + exitSummary = fmt.Sprintf("process exit was not observed before context completion: %v", ctx.Err()) + case <-exitTimer.C: + exitSummary = fmt.Sprintf("process exit was not observed within %s", drainExitTimeout) + } + + t.Fatalf( + "expected PTY output %q; child PID=%d; %s; read error=%v; output=%q", + target, + sp.Handle.Pid, + exitSummary, + readErr, + out, + ) + return out +} + // awaitExit blocks until the exit notification arrives or ctx is done. func awaitExit(t *testing.T, ctx context.Context, exitHandler *process.ConcurrentProcessExitHandler) process.ProcessExitInfo { t.Helper() diff --git a/internal/termpty/pty_unix_test.go b/internal/termpty/pty_unix_test.go index fc0eb0ca..dc33048e 100644 --- a/internal/termpty/pty_unix_test.go +++ b/internal/termpty/pty_unix_test.go @@ -39,8 +39,7 @@ func TestStartProcessWithTerminal_StdoutFromChild(t *testing.T) { sp := startTermchildWithPTY(t, ctx, "--print", "hello-from-child") - out, err := readUntil(ctx, sp.PTY, "hello-from-child") - require.NoError(t, err, "expected to see 'hello-from-child'; got: %q", out) + _ = requireReadUntil(t, ctx, sp, "hello-from-child") ei := awaitExit(t, ctx, sp.ExitHandler) require.NoError(t, ei.Err) @@ -56,8 +55,7 @@ func TestStartProcessWithTerminal_StderrFromChild(t *testing.T) { sp := startTermchildWithPTY(t, ctx, "--print-stderr", "boom-from-stderr") - out, err := readUntil(ctx, sp.PTY, "boom-from-stderr") - require.NoError(t, err, "expected to see 'boom-from-stderr'; got: %q", out) + _ = requireReadUntil(t, ctx, sp, "boom-from-stderr") ei := awaitExit(t, ctx, sp.ExitHandler) require.NoError(t, ei.Err) diff --git a/test/integration/container_controller_test.go b/test/integration/container_controller_test.go index 88db4081..834069ff 100644 --- a/test/integration/container_controller_test.go +++ b/test/integration/container_controller_test.go @@ -118,7 +118,7 @@ func TestPersistentContainerRecordsWorkloadID(t *testing.T) { ctx, cancel := testutil.GetTestContext(t, defaultIntegrationTestTimeout) defer cancel() - serverInfo, teInfo, envStartErr := StartTestEnvironmentWithOptions(ctx, ContainerController, "PersistentContainerWorkloadID", t.TempDir(), TestEnvironmentOptions{ + serverInfo, teInfo, envStartErr := StartTestEnvironmentWithOptions(t, ctx, ContainerController, "PersistentContainerWorkloadID", t.TempDir(), TestEnvironmentOptions{ WorkloadID: "workload-a", }) require.NoError(t, envStartErr) @@ -331,7 +331,7 @@ func TestContainerRuntimeUnhealthy(t *testing.T) { // We are going to use a separate instance of the API server because we need to simulate container runtime being unhealthy, // and that might interfere with other tests if we used the shared container orchestrator. - serverInfo, _, startupErr := StartTestEnvironment(ctx, ContainerController, t.Name(), NoSeparateWorkingDir) + serverInfo, _, startupErr := StartTestEnvironment(t, ctx, ContainerController, t.Name(), NoSeparateWorkingDir) require.NoError(t, startupErr, "Failed to start the API server") defer func() { @@ -2018,7 +2018,7 @@ func TestContainerCleanupModeDeletedBeforeAdoptionRemovesExistingContainer(t *te const testName = "container-cleanup-mode-delete-before-adopt" const imageName = testName + "-image" - serverInfo, _, startupErr := StartTestEnvironment(ctx, ContainerController, t.Name(), NoSeparateWorkingDir) + serverInfo, _, startupErr := StartTestEnvironment(t, ctx, ContainerController, t.Name(), NoSeparateWorkingDir) require.NoError(t, startupErr, "failed to start the API server") defer func() { cancel() diff --git a/test/integration/container_network_tunnel_proxy_test.go b/test/integration/container_network_tunnel_proxy_test.go index 12d5c2b7..4cbfd41d 100644 --- a/test/integration/container_network_tunnel_proxy_test.go +++ b/test/integration/container_network_tunnel_proxy_test.go @@ -59,9 +59,8 @@ func TestTunnelProxyCreateDelete(t *testing.T) { // Use dedicated test environment because otherwise it is difficult to differentiate // between different server proxy processes running in parallel tests. includedControllers := ServiceController | NetworkController | ContainerNetworkTunnelProxyController - serverInfo, teInfo, startupErr := StartTestEnvironment(ctx, includedControllers, t.Name(), t.TempDir()) + serverInfo, teInfo, startupErr := StartTestEnvironment(t, ctx, includedControllers, t.Name(), t.TempDir()) require.NoError(t, startupErr, "Failed to start the API server") - defer shutdownTestEnvironment(serverInfo, cancel) network := apiv1.ContainerNetwork{ ObjectMeta: metav1.ObjectMeta{ @@ -124,9 +123,8 @@ func TestTunnelProxyDelayedNetworkCreation(t *testing.T) { const testName = "test-tunnel-proxy-delayed-network-creation" includedControllers := ServiceController | NetworkController | ContainerNetworkTunnelProxyController - serverInfo, teInfo, startupErr := StartTestEnvironment(ctx, includedControllers, t.Name(), t.TempDir()) + serverInfo, teInfo, startupErr := StartTestEnvironment(t, ctx, includedControllers, t.Name(), t.TempDir()) require.NoError(t, startupErr, "Failed to start the API server") - defer shutdownTestEnvironment(serverInfo, cancel) // Create the ContainerNetworkTunnelProxy object WITHOUT creating the ContainerNetwork first tunnelProxy := apiv1.ContainerNetworkTunnelProxy{ @@ -182,13 +180,13 @@ func TestTunnelProxyDelayedNetworkCreation(t *testing.T) { // Verifies that running ContainerNetworkTunnelProxy has the status updated with client proxy and server proxy information. func TestTunnelProxyRunningStatus(t *testing.T) { ctx, cancel := testutil.GetTestContext(t, defaultIntegrationTestTimeout) + defer cancel() dcppaths.EnableTestPathProbing() const testName = "test-tunnel-proxy-running-status" includedControllers := ServiceController | NetworkController | ContainerNetworkTunnelProxyController - serverInfo, teInfo, startupErr := StartTestEnvironment(ctx, includedControllers, t.Name(), t.TempDir()) + serverInfo, teInfo, startupErr := StartTestEnvironment(t, ctx, includedControllers, t.Name(), t.TempDir()) require.NoError(t, startupErr, "Failed to start the API server") - defer shutdownTestEnvironment(serverInfo, cancel) testContainerOrchestrator, ok := serverInfo.ContainerOrchestrator.(*ctrl_testutil.TestContainerOrchestrator) require.True(t, ok) buildContextDir := teInfo.SessionFolder @@ -303,7 +301,7 @@ func TestTunnelProxyRunningStatus(t *testing.T) { buildContextFiles, globErr := filepath.Glob(filepath.Join(buildContextDir, "dcptun-build-context-*.tar")) require.NoError(t, globErr) require.Equal(t, []string{physicalImages.Items[0].Spec.Image.Build.ContextArchive.Source}, buildContextFiles) - t.Cleanup(func() { + teInfo.addAfterShutdown(func() { require.NoFileExists(t, physicalImages.Items[0].Spec.Image.Build.ContextArchive.Source) }) require.Equal(t, apiv2.PhysicalContainerImagePhaseReady, physicalImages.Items[0].Status.Phase) @@ -406,9 +404,8 @@ func TestTunnelProxyCleanup(t *testing.T) { const testName = "test-tunnel-proxy-cleanup" includedControllers := ServiceController | NetworkController | ContainerNetworkTunnelProxyController - serverInfo, teInfo, startupErr := StartTestEnvironment(ctx, includedControllers, t.Name(), t.TempDir()) + serverInfo, teInfo, startupErr := StartTestEnvironment(t, ctx, includedControllers, t.Name(), t.TempDir()) require.NoError(t, startupErr, "Failed to start the API server") - defer shutdownTestEnvironment(serverInfo, cancel) network := apiv1.ContainerNetwork{ ObjectMeta: metav1.ObjectMeta{ @@ -496,7 +493,7 @@ func TestTunnelProxyCleanup(t *testing.T) { require.NotEmpty(t, physicalImage.Spec.Image.Build.ContextArchive.Source) require.Empty(t, physicalImage.Spec.Image.Build.ContextArchive.RawContents) require.FileExists(t, physicalImage.Spec.Image.Build.ContextArchive.Source) - t.Cleanup(func() { + teInfo.addAfterShutdown(func() { require.NoFileExists(t, physicalImage.Spec.Image.Build.ContextArchive.Source) }) @@ -541,9 +538,8 @@ func TestTunnelProxyTunnelCreate(t *testing.T) { const testName = "test-tunnel-proxy-tunnel-management" includedControllers := NetworkController | ContainerNetworkTunnelProxyController | ServiceController - serverInfo, teInfo, startupErr := StartTestEnvironment(ctx, includedControllers, t.Name(), t.TempDir()) + serverInfo, teInfo, startupErr := StartTestEnvironment(t, ctx, includedControllers, t.Name(), t.TempDir()) require.NoError(t, startupErr, "Failed to start the API server") - defer shutdownTestEnvironment(serverInfo, cancel) network := apiv1.ContainerNetwork{ ObjectMeta: metav1.ObjectMeta{ @@ -600,9 +596,8 @@ func TestTunnelProxyTunnelCreateFallsBackToRandomPort(t *testing.T) { const testName = "test-tunnel-proxy-tunnel-port-fallback" includedControllers := NetworkController | ContainerNetworkTunnelProxyController | ServiceController - serverInfo, teInfo, startupErr := StartTestEnvironment(ctx, includedControllers, t.Name(), t.TempDir()) + serverInfo, teInfo, startupErr := StartTestEnvironment(t, ctx, includedControllers, t.Name(), t.TempDir()) require.NoError(t, startupErr, "Failed to start the API server") - defer shutdownTestEnvironment(serverInfo, cancel) network := apiv1.ContainerNetwork{ ObjectMeta: metav1.ObjectMeta{ @@ -671,9 +666,8 @@ func TestTunnelProxyTunnelFailure(t *testing.T) { const testName = "test-tunnel-proxy-tunnel-failure" includedControllers := NetworkController | ContainerNetworkTunnelProxyController | ServiceController - serverInfo, teInfo, startupErr := StartTestEnvironment(ctx, includedControllers, t.Name(), t.TempDir()) + serverInfo, teInfo, startupErr := StartTestEnvironment(t, ctx, includedControllers, t.Name(), t.TempDir()) require.NoError(t, startupErr, "Failed to start the API server") - defer shutdownTestEnvironment(serverInfo, cancel) network := apiv1.ContainerNetwork{ ObjectMeta: metav1.ObjectMeta{ @@ -732,9 +726,8 @@ func TestTunnelProxyServerServiceTransition(t *testing.T) { const testName = "test-tunnel-proxy-server-service-transition" includedControllers := NetworkController | ContainerNetworkTunnelProxyController | ServiceController - serverInfo, teInfo, startupErr := StartTestEnvironment(ctx, includedControllers, t.Name(), t.TempDir()) + serverInfo, teInfo, startupErr := StartTestEnvironment(t, ctx, includedControllers, t.Name(), t.TempDir()) require.NoError(t, startupErr, "Failed to start the API server") - defer shutdownTestEnvironment(serverInfo, cancel) network := apiv1.ContainerNetwork{ ObjectMeta: metav1.ObjectMeta{ @@ -842,9 +835,8 @@ func TestTunnelProxyMultipleTunnels(t *testing.T) { const testName = "test-tunnel-proxy-multiple-tunnels" includedControllers := NetworkController | ContainerNetworkTunnelProxyController | ServiceController - serverInfo, teInfo, startupErr := StartTestEnvironment(ctx, includedControllers, t.Name(), t.TempDir()) + serverInfo, teInfo, startupErr := StartTestEnvironment(t, ctx, includedControllers, t.Name(), t.TempDir()) require.NoError(t, startupErr, "Failed to start the API server") - defer shutdownTestEnvironment(serverInfo, cancel) network := apiv1.ContainerNetwork{ ObjectMeta: metav1.ObjectMeta{ @@ -928,9 +920,8 @@ func TestTunnelProxyClientProxyAliases(t *testing.T) { const testName = "test-tunnel-proxy-client-proxy-aliases" includedControllers := ServiceController | NetworkController | ContainerNetworkTunnelProxyController - serverInfo, teInfo, startupErr := StartTestEnvironment(ctx, includedControllers, t.Name(), t.TempDir()) + serverInfo, teInfo, startupErr := StartTestEnvironment(t, ctx, includedControllers, t.Name(), t.TempDir()) require.NoError(t, startupErr, "Failed to start the API server") - defer shutdownTestEnvironment(serverInfo, cancel) network := apiv1.ContainerNetwork{ ObjectMeta: metav1.ObjectMeta{ @@ -993,9 +984,8 @@ func TestTunnelProxyServerStartupFailure(t *testing.T) { const testName = "test-tunnel-proxy-server-startup-failure" includedControllers := ServiceController | NetworkController | ContainerNetworkTunnelProxyController - serverInfo, teInfo, startupErr := StartTestEnvironment(ctx, includedControllers, t.Name(), t.TempDir()) + serverInfo, teInfo, startupErr := StartTestEnvironment(t, ctx, includedControllers, t.Name(), t.TempDir()) require.NoError(t, startupErr, "Failed to start the API server") - defer shutdownTestEnvironment(serverInfo, cancel) network := apiv1.ContainerNetwork{ ObjectMeta: metav1.ObjectMeta{ @@ -1069,9 +1059,8 @@ func TestTunnelProxyClientContainerStartupFailure(t *testing.T) { const testName = "test-tunnel-proxy-client-container-startup-failure" includedControllers := ServiceController | NetworkController | ContainerNetworkTunnelProxyController - serverInfo, _, startupErr := StartTestEnvironment(ctx, includedControllers, t.Name(), t.TempDir()) + serverInfo, _, startupErr := StartTestEnvironment(t, ctx, includedControllers, t.Name(), t.TempDir()) require.NoError(t, startupErr, "Failed to start the API server") - defer shutdownTestEnvironment(serverInfo, cancel) network := apiv1.ContainerNetwork{ ObjectMeta: metav1.ObjectMeta{ @@ -1132,9 +1121,8 @@ func TestTunnelProxyServerUnexpectedExit(t *testing.T) { const testName = "test-tunnel-proxy-server-unexpected-exit" includedControllers := ServiceController | NetworkController | ContainerNetworkTunnelProxyController - serverInfo, teInfo, startupErr := StartTestEnvironment(ctx, includedControllers, t.Name(), t.TempDir()) + serverInfo, teInfo, startupErr := StartTestEnvironment(t, ctx, includedControllers, t.Name(), t.TempDir()) require.NoError(t, startupErr, "Failed to start the API server") - defer shutdownTestEnvironment(serverInfo, cancel) network := apiv1.ContainerNetwork{ ObjectMeta: metav1.ObjectMeta{ @@ -1287,9 +1275,8 @@ func testTunnelProxyClientFailure( dcppaths.EnableTestPathProbing() includedControllers := ServiceController | NetworkController | ContainerNetworkTunnelProxyController - serverInfo, teInfo, startupErr := StartTestEnvironment(ctx, includedControllers, t.Name(), t.TempDir()) + serverInfo, teInfo, startupErr := StartTestEnvironment(t, ctx, includedControllers, t.Name(), t.TempDir()) require.NoError(t, startupErr, "Failed to start the API server") - defer shutdownTestEnvironment(serverInfo, cancel) network := apiv1.ContainerNetwork{ ObjectMeta: metav1.ObjectMeta{ @@ -1582,18 +1569,6 @@ func testTunnelProxyWithRealOrchestrator( require.Equal(t, int32(0), *updatedServerExe.Status.ExitCode, "Parrot server executable should exit with code 0 indicating successful completion of all conversations") } -func shutdownTestEnvironment(serverInfo *ctrl_testutil.ApiServerInfo, cancel context.CancelFunc) { - cancel() - - // StartTestEnvironment closes its state store before disposing the API server. Wait - // for that full cleanup path before returning so t.TempDir cleanup does not race - // open SQLite file handles on Windows. - select { - case <-serverInfo.ApiServerDisposalComplete.Wait(): - case <-time.After(20 * time.Second): - } -} - func shutdownAdvancedTestEnvironment( t *testing.T, ctx context.Context, @@ -1612,7 +1587,7 @@ func shutdownAdvancedTestEnvironment( waitErr := ctrl_testutil.WaitApiServerStatus(ctx, client, serverInfo, apiserver.ApiServerCleanupComplete) require.NoError(t, waitErr, "Failed to wait for API server to complete cleanup") - shutdownTestEnvironment(serverInfo, cancel) + shutdownTestEnvironment(t, serverInfo, cancel) } // The tunnel controller will try to create a server-side proxy diff --git a/test/integration/controllers_common_test.go b/test/integration/controllers_common_test.go index 7761c07b..8dfb4988 100644 --- a/test/integration/controllers_common_test.go +++ b/test/integration/controllers_common_test.go @@ -115,7 +115,7 @@ func v1VolumeMountsToCreateContainerVolumeMounts(mounts []apiv1.VolumeMount) []c func TestMain(m *testing.M) { ctx, cancel := context.WithCancel(context.Background()) - serverInfo, teInfo, envStartErr := StartTestEnvironment(ctx, AllControllers, "IntegrationTests", "") + serverInfo, teInfo, envStartErr := StartTestEnvironment(nil, ctx, AllControllers, "IntegrationTests", "") if envStartErr != nil { cancel() panic(envStartErr) diff --git a/test/integration/executable_controller_test.go b/test/integration/executable_controller_test.go index 870e157c..74866067 100644 --- a/test/integration/executable_controller_test.go +++ b/test/integration/executable_controller_test.go @@ -373,6 +373,7 @@ func TestExecutableCleanupModeStopsExistingProcessOnDelete(t *testing.T) { const testName = "executable-cleanup-mode-stops-existing" serverInfo, teInfo, startupErr := StartTestEnvironment( + t, ctx, ExecutableController, testName, @@ -735,7 +736,7 @@ func TestPersistentExecutableRecordsWorkloadID(t *testing.T) { ctx, cancel := testutil.GetTestContext(t, defaultIntegrationTestTimeout) defer cancel() - serverInfo, teInfo, envStartErr := StartTestEnvironmentWithOptions(ctx, ExecutableController, "PersistentExecutableWorkloadID", t.TempDir(), TestEnvironmentOptions{ + serverInfo, teInfo, envStartErr := StartTestEnvironmentWithOptions(t, ctx, ExecutableController, "PersistentExecutableWorkloadID", t.TempDir(), TestEnvironmentOptions{ WorkloadID: "workload-a", }) require.NoError(t, envStartErr) @@ -1081,7 +1082,7 @@ func TestPersistentExecutableStopsProcessWhenProcessRecordUpdateFails(t *testing ctx, cancel := testutil.GetTestContext(t, defaultIntegrationTestTimeout) defer cancel() - serverInfo, teInfo, startupErr := StartTestEnvironment(ctx, ExecutableController, t.Name(), t.TempDir()) + serverInfo, teInfo, startupErr := StartTestEnvironment(t, ctx, ExecutableController, t.Name(), t.TempDir()) require.NoError(t, startupErr, "Test environment could not be started") defer func() { cancel() diff --git a/test/integration/network_controller_test.go b/test/integration/network_controller_test.go index 9b25bb91..a72c9030 100644 --- a/test/integration/network_controller_test.go +++ b/test/integration/network_controller_test.go @@ -119,7 +119,7 @@ func TestPersistentNetworkRecordsWorkloadID(t *testing.T) { ctx, cancel := testutil.GetTestContext(t, defaultIntegrationTestTimeout) defer cancel() - serverInfo, teInfo, envStartErr := StartTestEnvironmentWithOptions(ctx, NetworkController, "PersistentNetworkWorkloadID", t.TempDir(), TestEnvironmentOptions{ + serverInfo, teInfo, envStartErr := StartTestEnvironmentWithOptions(t, ctx, NetworkController, "PersistentNetworkWorkloadID", t.TempDir(), TestEnvironmentOptions{ WorkloadID: "workload-a", }) require.NoError(t, envStartErr) @@ -477,7 +477,7 @@ func TestNetworkRuntimeUnhealthy(t *testing.T) { // We are going to use a separate instance of the API server because we need to simulate container runtime being unhealthy, // and that might interfere with other tests if we used the shared container orchestrator. - serverInfo, _, startupErr := StartTestEnvironment(ctx, NetworkController, t.Name(), NoSeparateWorkingDir) + serverInfo, _, startupErr := StartTestEnvironment(t, ctx, NetworkController, t.Name(), NoSeparateWorkingDir) require.NoError(t, startupErr, "Failed to start the API server") defer func() { diff --git a/test/integration/standard_test_env.go b/test/integration/standard_test_env.go index 1bccdb2d..960da0b3 100644 --- a/test/integration/standard_test_env.go +++ b/test/integration/standard_test_env.go @@ -9,6 +9,8 @@ import ( "context" "fmt" "math" + "sync" + "testing" "time" "github.com/go-logr/logr" @@ -43,6 +45,8 @@ type TestEnvironmentInfo struct { ResourceLeaseOwner process.ProcessHandle Log logr.Logger SessionFolder string + afterShutdownLock sync.Mutex + afterShutdown []func() } type TestEnvironmentOptions struct { @@ -50,9 +54,27 @@ type TestEnvironmentOptions struct { DecorateContainerOrchestrator func(containers.ContainerOrchestrator, *statestore.Store) containers.ContainerOrchestrator } +func (tei *TestEnvironmentInfo) addAfterShutdown(callback func()) { + tei.afterShutdownLock.Lock() + defer tei.afterShutdownLock.Unlock() + tei.afterShutdown = append(tei.afterShutdown, callback) +} + +func (tei *TestEnvironmentInfo) runAfterShutdown() { + tei.afterShutdownLock.Lock() + callbacks := tei.afterShutdown + tei.afterShutdown = nil + tei.afterShutdownLock.Unlock() + + for callbackIndex := len(callbacks) - 1; callbackIndex >= 0; callbackIndex-- { + callbacks[callbackIndex]() + } +} + // Starts the DCP API server (separate process) and standard controllers (in-proc). func StartTestEnvironment( - ctx context.Context, + t testing.TB, + parentCtx context.Context, inclCtrl IncludedController, instanceTag string, testTempDir string, @@ -61,11 +83,12 @@ func StartTestEnvironment( *TestEnvironmentInfo, error, ) { - return StartTestEnvironmentWithOptions(ctx, inclCtrl, instanceTag, testTempDir, TestEnvironmentOptions{}) + return StartTestEnvironmentWithOptions(t, parentCtx, inclCtrl, instanceTag, testTempDir, TestEnvironmentOptions{}) } func StartTestEnvironmentWithOptions( - ctx context.Context, + t testing.TB, + parentCtx context.Context, inclCtrl IncludedController, instanceTag string, testTempDir string, @@ -75,6 +98,21 @@ func StartTestEnvironmentWithOptions( *TestEnvironmentInfo, error, ) { + ctx := parentCtx + var serverInfo *ctrl_testutil.ApiServerInfo + var testEnvironmentInfo *TestEnvironmentInfo + if t != nil { + t.Helper() + var cancelEnvironment context.CancelFunc + ctx, cancelEnvironment = context.WithCancel(parentCtx) + t.Cleanup(func() { + shutdownTestEnvironment(t, serverInfo, cancelEnvironment) + if testEnvironmentInfo != nil { + testEnvironmentInfo.runAfterShutdown() + } + }) + } + inclCtrl |= NamespaceController if inclCtrl&ContainerNetworkTunnelProxyController != 0 { inclCtrl |= PhysicalContainerImageController | PhysicalContainerController | PhysicalContainerNetworkController @@ -88,9 +126,10 @@ func StartTestEnvironmentWithOptions( log := testutil.NewLogWithResourceSinkForTesting(instanceTag, sessionFolder) ctrl.SetLogger(log) - serverInfo, serverErr := ctrl_testutil.StartApiServer(ctx, ctrl_testutil.ApiServerFlagsNone, log, sessionFolder) - if serverErr != nil { - return nil, nil, fmt.Errorf("failed to start the API server: %w", serverErr) + startedServerInfo, serverStartErr := ctrl_testutil.StartApiServer(ctx, ctrl_testutil.ApiServerFlagsNone, log, sessionFolder) + serverInfo = startedServerInfo + if serverStartErr != nil { + return nil, nil, fmt.Errorf("failed to start the API server: %w", serverStartErr) } stateStore, stateStoreCleanup, stateStoreErr := createTestStateStore(ctx, testTempDir) @@ -406,7 +445,7 @@ func StartTestEnvironmentWithOptions( managerDone.Set() }() - teInfo := &TestEnvironmentInfo{ + testEnvironmentInfo = &TestEnvironmentInfo{ TestProcessExecutor: pex, TestProcessExecutableRunner: exeRunner, TestIdeRunner: ir, @@ -418,5 +457,26 @@ func StartTestEnvironmentWithOptions( Log: log, SessionFolder: sessionFolder, } - return serverInfo, teInfo, nil + return serverInfo, testEnvironmentInfo, nil +} + +func shutdownTestEnvironment( + t testing.TB, + serverInfo *ctrl_testutil.ApiServerInfo, + cancel context.CancelFunc, +) { + t.Helper() + cancel() + if serverInfo == nil { + return + } + + shutdownTimer := time.NewTimer(20 * time.Second) + defer shutdownTimer.Stop() + + select { + case <-serverInfo.ApiServerDisposalComplete.Wait(): + case <-shutdownTimer.C: + t.Errorf("timed out waiting for test environment shutdown") + } } diff --git a/test/integration/v2_physical_container_controller_test.go b/test/integration/v2_physical_container_controller_test.go index 6578b413..a1fe15f8 100644 --- a/test/integration/v2_physical_container_controller_test.go +++ b/test/integration/v2_physical_container_controller_test.go @@ -576,6 +576,7 @@ func TestV2PhysicalContainerControllerPreservesRuntimePhaseOnPortMappingFailure( const runtimeContainerName = "v2-pctr-invalid-port-runtime" var testContainerOrchestrator *ctrl_testutil.TestContainerOrchestrator serverInfo, _, startupErr := StartTestEnvironmentWithOptions( + t, ctx, PhysicalContainerController, t.Name(), @@ -596,7 +597,6 @@ func TestV2PhysicalContainerControllerPreservesRuntimePhaseOnPortMappingFailure( }, ) require.NoError(t, startupErr) - defer shutdownTestEnvironment(serverInfo, cancel) runtimeContainerID, runErr := testContainerOrchestrator.RunContainer(ctx, containers.RunContainerOptions{ CreateContainerOptions: containers.CreateContainerOptions{ diff --git a/test/integration/v2_physical_container_image_controller_test.go b/test/integration/v2_physical_container_image_controller_test.go index c268c4e0..b755616e 100644 --- a/test/integration/v2_physical_container_image_controller_test.go +++ b/test/integration/v2_physical_container_image_controller_test.go @@ -45,9 +45,11 @@ func (r *recordingBuildImageOrchestrator) BuildImage(ctx context.Context, option func TestV2PhysicalContainerImageControllerBuildsRawArchiveContext(t *testing.T) { t.Parallel() ctx, cancel := testutil.GetTestContext(t, defaultIntegrationTestTimeout) + defer cancel() var recordingOrchestrator *recordingBuildImageOrchestrator serverInfo, _, startErr := StartTestEnvironmentWithOptions( + t, ctx, NamespaceController|PhysicalContainerImageController, t.Name(), @@ -66,7 +68,6 @@ func TestV2PhysicalContainerImageControllerBuildsRawArchiveContext(t *testing.T) }, ) require.NoError(t, startErr) - defer shutdownTestEnvironment(serverInfo, cancel) namespace := &apiv2.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "v2-pci-raw-archive"}} require.NoError(t, serverInfo.Client.Create(ctx, namespace)) diff --git a/test/integration/v2_physical_container_network_controller_test.go b/test/integration/v2_physical_container_network_controller_test.go index a2a4c373..e43612df 100644 --- a/test/integration/v2_physical_container_network_controller_test.go +++ b/test/integration/v2_physical_container_network_controller_test.go @@ -144,7 +144,7 @@ func TestV2PhysicalContainerNetworkControllerRemovesCreatedNetworkOnDeletion(t * func TestV2PhysicalContainerNetworkControllerReportsDeletionFailure(t *testing.T) { ctx, cancel := testutil.GetTestContext(t, defaultIntegrationTestTimeout) - serverInfo, _, startupErr := StartTestEnvironment(ctx, NamespaceController|PhysicalContainerNetworkController, t.Name(), NoSeparateWorkingDir) + serverInfo, _, startupErr := StartTestEnvironment(t, ctx, NamespaceController|PhysicalContainerNetworkController, t.Name(), NoSeparateWorkingDir) require.NoError(t, startupErr, "Failed to start the API server") defer func() { @@ -753,7 +753,7 @@ func TestV2PhysicalContainerNetworkControllerRecoversFromRuntimeFailure(t *testi // We are going to use a separate instance of the API server because we need to simulate the // container runtime being unhealthy, and that would interfere with other tests if we used the // shared container orchestrator. - serverInfo, _, startupErr := StartTestEnvironment(ctx, NamespaceController|PhysicalContainerNetworkController, t.Name(), NoSeparateWorkingDir) + serverInfo, _, startupErr := StartTestEnvironment(t, ctx, NamespaceController|PhysicalContainerNetworkController, t.Name(), NoSeparateWorkingDir) require.NoError(t, startupErr, "Failed to start the API server") defer func() { @@ -823,7 +823,7 @@ func TestV2PhysicalContainerNetworkControllerRecoversFromRuntimeFailure(t *testi func TestV2PhysicalContainerNetworkControllerRecoversFromCreateFailure(t *testing.T) { ctx, cancel := testutil.GetTestContext(t, defaultIntegrationTestTimeout) - serverInfo, _, startupErr := StartTestEnvironment(ctx, NamespaceController|PhysicalContainerNetworkController, t.Name(), NoSeparateWorkingDir) + serverInfo, _, startupErr := StartTestEnvironment(t, ctx, NamespaceController|PhysicalContainerNetworkController, t.Name(), NoSeparateWorkingDir) require.NoError(t, startupErr, "Failed to start the API server") defer func() { @@ -876,7 +876,7 @@ func TestV2PhysicalContainerNetworkControllerRecoversFromCreateFailure(t *testin func TestV2PhysicalContainerNetworkControllerRecoversFromReplacementFailure(t *testing.T) { ctx, cancel := testutil.GetTestContext(t, defaultIntegrationTestTimeout) - serverInfo, _, startupErr := StartTestEnvironment(ctx, NamespaceController|PhysicalContainerNetworkController, t.Name(), NoSeparateWorkingDir) + serverInfo, _, startupErr := StartTestEnvironment(t, ctx, NamespaceController|PhysicalContainerNetworkController, t.Name(), NoSeparateWorkingDir) require.NoError(t, startupErr, "Failed to start the API server") defer func() { diff --git a/test/integration/v2_physical_container_volume_controller_test.go b/test/integration/v2_physical_container_volume_controller_test.go index 7e0fa02f..00dbe6b2 100644 --- a/test/integration/v2_physical_container_volume_controller_test.go +++ b/test/integration/v2_physical_container_volume_controller_test.go @@ -8,6 +8,7 @@ package integration_test import ( "context" "errors" + "slices" "testing" "time" @@ -249,10 +250,17 @@ func TestV2PhysicalContainerVolumeControllerWaitsForCreateBeforeDeletion(t *test waitCreateVolumeCallCount(t, ctx, volumeName, 1) require.NoError(t, client.Delete(ctx, volume)) + finalizer := apiv2.GroupName + "/physicalcontainervolume-reconciler" terminatingVolume := waitObjectAssumesState(t, ctx, volume.NamespacedName(), func(current *apiv2.PhysicalContainerVolume) (bool, error) { - return current.DeletionTimestamp != nil && !current.DeletionTimestamp.IsZero(), nil + readyCondition := apimeta.FindStatusCondition(current.Status.Conditions, string(apiv2.ConditionReady)) + return current.DeletionTimestamp != nil && + !current.DeletionTimestamp.IsZero() && + slices.Contains(current.Finalizers, finalizer) && + readyCondition != nil && + readyCondition.Status == metav1.ConditionFalse && + readyCondition.Reason == string(apiv2.PhysicalContainerVolumeReasonCreating), nil }) - require.Contains(t, terminatingVolume.Finalizers, apiv2.GroupName+"/physicalcontainervolume-reconciler") + require.Contains(t, terminatingVolume.Finalizers, finalizer) requireReadyCondition(t, terminatingVolume.Status.Conditions, metav1.ConditionFalse, apiv2.PhysicalContainerVolumeReasonCreating) require.Equal(t, 0, containerOrchestrator.RemoveVolumeCallCount(volumeName)) @@ -452,7 +460,7 @@ func TestV2PhysicalContainerVolumeControllerRetriesTransientReplacementRemovalFa func TestV2PhysicalContainerVolumeControllerRetriesTransientReplacementInspectionFailure(t *testing.T) { ctx, cancel := testutil.GetTestContext(t, defaultIntegrationTestTimeout) - serverInfo, _, startupErr := StartTestEnvironment(ctx, NamespaceController|PhysicalContainerVolumeController, t.Name(), NoSeparateWorkingDir) + serverInfo, _, startupErr := StartTestEnvironment(t, ctx, NamespaceController|PhysicalContainerVolumeController, t.Name(), NoSeparateWorkingDir) require.NoError(t, startupErr) defer func() { cancel() @@ -580,7 +588,7 @@ func TestV2PhysicalContainerVolumeControllerDoesNotChurnReadyStatus(t *testing.T func TestV2PhysicalContainerVolumeControllerRecoversFromRuntimeAndCreateFailures(t *testing.T) { ctx, cancel := testutil.GetTestContext(t, defaultIntegrationTestTimeout) - serverInfo, _, startupErr := StartTestEnvironment(ctx, NamespaceController|PhysicalContainerVolumeController, t.Name(), NoSeparateWorkingDir) + serverInfo, _, startupErr := StartTestEnvironment(t, ctx, NamespaceController|PhysicalContainerVolumeController, t.Name(), NoSeparateWorkingDir) require.NoError(t, startupErr) defer func() { cancel() diff --git a/test/integration/volume_controller_test.go b/test/integration/volume_controller_test.go index ff822efb..cc510c89 100644 --- a/test/integration/volume_controller_test.go +++ b/test/integration/volume_controller_test.go @@ -101,7 +101,7 @@ func TestPersistentVolumeRecordsWorkloadID(t *testing.T) { ctx, cancel := testutil.GetTestContext(t, defaultIntegrationTestTimeout) defer cancel() - serverInfo, teInfo, envStartErr := StartTestEnvironmentWithOptions(ctx, VolumeController, "PersistentVolumeWorkloadID", t.TempDir(), TestEnvironmentOptions{ + serverInfo, teInfo, envStartErr := StartTestEnvironmentWithOptions(t, ctx, VolumeController, "PersistentVolumeWorkloadID", t.TempDir(), TestEnvironmentOptions{ WorkloadID: "workload-a", }) require.NoError(t, envStartErr) @@ -133,7 +133,7 @@ func TestExistingPersistentVolumeIsNotRecordedForWorkloadCleanup(t *testing.T) { ctx, cancel := testutil.GetTestContext(t, defaultIntegrationTestTimeout) defer cancel() - serverInfo, teInfo, envStartErr := StartTestEnvironmentWithOptions(ctx, VolumeController, "ExistingPersistentVolumeWorkloadID", t.TempDir(), TestEnvironmentOptions{ + serverInfo, teInfo, envStartErr := StartTestEnvironmentWithOptions(t, ctx, VolumeController, "ExistingPersistentVolumeWorkloadID", t.TempDir(), TestEnvironmentOptions{ WorkloadID: "workload-a", }) require.NoError(t, envStartErr) @@ -163,7 +163,7 @@ func TestPersistentVolumeRecordPrecedesRuntimeCreation(t *testing.T) { defer cancel() var recordingOrchestrator *recordingVolumeCreateOrchestrator - serverInfo, teInfo, envStartErr := StartTestEnvironmentWithOptions(ctx, VolumeController, "PersistentVolumeRecordBeforeCreate", t.TempDir(), TestEnvironmentOptions{ + serverInfo, teInfo, envStartErr := StartTestEnvironmentWithOptions(t, ctx, VolumeController, "PersistentVolumeRecordBeforeCreate", t.TempDir(), TestEnvironmentOptions{ WorkloadID: "workload-a", DecorateContainerOrchestrator: func( orchestrator containers.ContainerOrchestrator, @@ -194,7 +194,7 @@ func TestPersistentVolumePersistenceFailurePreventsRuntimeCreation(t *testing.T) defer cancel() var failingOrchestrator *volumePersistenceFailureOrchestrator - serverInfo, _, envStartErr := StartTestEnvironmentWithOptions(ctx, VolumeController, "PersistentVolumePersistenceFailure", t.TempDir(), TestEnvironmentOptions{ + serverInfo, _, envStartErr := StartTestEnvironmentWithOptions(t, ctx, VolumeController, "PersistentVolumePersistenceFailure", t.TempDir(), TestEnvironmentOptions{ WorkloadID: "workload-a", DecorateContainerOrchestrator: func( orchestrator containers.ContainerOrchestrator, @@ -232,7 +232,7 @@ func TestPersistentVolumeWithoutWorkloadIDDoesNotUseStateStore(t *testing.T) { ctx, cancel := testutil.GetTestContext(t, defaultIntegrationTestTimeout) defer cancel() - serverInfo, _, envStartErr := StartTestEnvironmentWithOptions(ctx, VolumeController, "PersistentVolumeWithoutWorkloadID", t.TempDir(), TestEnvironmentOptions{ + serverInfo, _, envStartErr := StartTestEnvironmentWithOptions(t, ctx, VolumeController, "PersistentVolumeWithoutWorkloadID", t.TempDir(), TestEnvironmentOptions{ DecorateContainerOrchestrator: func( orchestrator containers.ContainerOrchestrator, stateStore *statestore.Store, @@ -253,7 +253,7 @@ func TestPersistentVolumeCreateRaceAdoptsUnlabeledVolume(t *testing.T) { ctx, cancel := testutil.GetTestContext(t, defaultIntegrationTestTimeout) defer cancel() - serverInfo, teInfo, envStartErr := StartTestEnvironmentWithOptions(ctx, VolumeController, "PersistentVolumeCreateRace", t.TempDir(), TestEnvironmentOptions{ + serverInfo, teInfo, envStartErr := StartTestEnvironmentWithOptions(t, ctx, VolumeController, "PersistentVolumeCreateRace", t.TempDir(), TestEnvironmentOptions{ WorkloadID: "workload-a", DecorateContainerOrchestrator: func( orchestrator containers.ContainerOrchestrator, @@ -278,7 +278,7 @@ func TestPersistentVolumeAmbiguousCreateFailureRetainsOwnershipRecord(t *testing ctx, cancel := testutil.GetTestContext(t, defaultIntegrationTestTimeout) defer cancel() - serverInfo, teInfo, envStartErr := StartTestEnvironmentWithOptions(ctx, VolumeController, "PersistentVolumeAmbiguousCreate", t.TempDir(), TestEnvironmentOptions{ + serverInfo, teInfo, envStartErr := StartTestEnvironmentWithOptions(t, ctx, VolumeController, "PersistentVolumeAmbiguousCreate", t.TempDir(), TestEnvironmentOptions{ WorkloadID: "workload-a", DecorateContainerOrchestrator: func( orchestrator containers.ContainerOrchestrator, @@ -496,7 +496,7 @@ func TestContainerVolumeCleanup(t *testing.T) { ctx, cancel := testutil.GetTestContext(t, defaultIntegrationTestTimeout) - serverInfo, _, startupErr := StartTestEnvironment(ctx, VolumeController, t.Name(), NoSeparateWorkingDir) + serverInfo, _, startupErr := StartTestEnvironment(t, ctx, VolumeController, t.Name(), NoSeparateWorkingDir) require.NoError(t, startupErr, "Failed to start the API server") defer func() { @@ -592,7 +592,7 @@ func TestContainerVolumeRuntimeUnhealthy(t *testing.T) { // We are going to use a separate instance of the API server because we need to simulate container runtime being unhealthy, // and that might interfere with other tests if we used the shared container orchestrator. - serverInfo, _, startupErr := StartTestEnvironment(ctx, VolumeController, t.Name(), NoSeparateWorkingDir) + serverInfo, _, startupErr := StartTestEnvironment(t, ctx, VolumeController, t.Name(), NoSeparateWorkingDir) require.NoError(t, startupErr, "Failed to start the API server") defer func() { From 1b99fa92864e89190c1e97031f79c9b56e13a49c Mon Sep 17 00:00:00 2001 From: David Negstad Date: Mon, 21 Sep 2026 16:09:09 -0700 Subject: [PATCH 2/3] Remove duplicate test environment teardown Rely on centralized standard environment cleanup and follow lowercase error string conventions for PTY diagnostics. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- internal/termpty/pty_test.go | 2 +- test/integration/container_controller_test.go | 19 +-------- .../integration/executable_controller_test.go | 7 ---- test/integration/network_controller_test.go | 10 ----- ...sical_container_network_controller_test.go | 42 ++----------------- ...ysical_container_volume_controller_test.go | 16 +------ test/integration/volume_controller_test.go | 23 +--------- 7 files changed, 11 insertions(+), 108 deletions(-) diff --git a/internal/termpty/pty_test.go b/internal/termpty/pty_test.go index d6282790..282a59be 100644 --- a/internal/termpty/pty_test.go +++ b/internal/termpty/pty_test.go @@ -149,7 +149,7 @@ type readUntilError struct { func (e *readUntilError) Error() string { return fmt.Sprintf( - "PTY read failed before observing %q after %d attempts, %d bytes, and %s: %v", + "pty read failed before observing %q after %d attempts, %d bytes, and %s: %v", e.target, e.readAttempts, e.bytesRead, diff --git a/test/integration/container_controller_test.go b/test/integration/container_controller_test.go index 834069ff..9715e8e2 100644 --- a/test/integration/container_controller_test.go +++ b/test/integration/container_controller_test.go @@ -324,6 +324,7 @@ func TestContainerInstanceStarts(t *testing.T) { func TestContainerRuntimeUnhealthy(t *testing.T) { t.Parallel() ctx, cancel := testutil.GetTestContext(t, defaultIntegrationTestTimeout) + defer cancel() const testName = "container-runtime-unhealthy" const imageName = testName + "-image" @@ -334,16 +335,6 @@ func TestContainerRuntimeUnhealthy(t *testing.T) { serverInfo, _, startupErr := StartTestEnvironment(t, ctx, ContainerController, t.Name(), NoSeparateWorkingDir) require.NoError(t, startupErr, "Failed to start the API server") - defer func() { - cancel() - - // Wait for the API server cleanup to complete. - select { - case <-serverInfo.ApiServerDisposalComplete.Wait(): - case <-time.After(5 * time.Second): - } - }() - ctr := apiv1.Container{ ObjectMeta: metav1.ObjectMeta{ Name: testName, @@ -2014,19 +2005,13 @@ func TestContainerCleanupModeRemovesExistingContainerOnDelete(t *testing.T) { func TestContainerCleanupModeDeletedBeforeAdoptionRemovesExistingContainer(t *testing.T) { t.Parallel() ctx, cancel := testutil.GetTestContext(t, defaultIntegrationTestTimeout) + defer cancel() const testName = "container-cleanup-mode-delete-before-adopt" const imageName = testName + "-image" serverInfo, _, startupErr := StartTestEnvironment(t, ctx, ContainerController, t.Name(), NoSeparateWorkingDir) require.NoError(t, startupErr, "failed to start the API server") - defer func() { - cancel() - select { - case <-serverInfo.ApiServerDisposalComplete.Wait(): - case <-time.After(5 * time.Second): - } - }() ctr := apiv1.Container{ ObjectMeta: metav1.ObjectMeta{ diff --git a/test/integration/executable_controller_test.go b/test/integration/executable_controller_test.go index 74866067..db2f15c4 100644 --- a/test/integration/executable_controller_test.go +++ b/test/integration/executable_controller_test.go @@ -1084,13 +1084,6 @@ func TestPersistentExecutableStopsProcessWhenProcessRecordUpdateFails(t *testing serverInfo, teInfo, startupErr := StartTestEnvironment(t, ctx, ExecutableController, t.Name(), t.TempDir()) require.NoError(t, startupErr, "Test environment could not be started") - defer func() { - cancel() - select { - case <-serverInfo.ApiServerDisposalComplete.Wait(): - case <-time.After(5 * time.Second): - } - }() exe := &apiv1.Executable{ ObjectMeta: metav1.ObjectMeta{ diff --git a/test/integration/network_controller_test.go b/test/integration/network_controller_test.go index a72c9030..731e2911 100644 --- a/test/integration/network_controller_test.go +++ b/test/integration/network_controller_test.go @@ -480,16 +480,6 @@ func TestNetworkRuntimeUnhealthy(t *testing.T) { serverInfo, _, startupErr := StartTestEnvironment(t, ctx, NetworkController, t.Name(), NoSeparateWorkingDir) require.NoError(t, startupErr, "Failed to start the API server") - defer func() { - cancel() - - // Wait for the API server cleanup to complete. - select { - case <-serverInfo.ApiServerDisposalComplete.Wait(): - case <-time.After(5 * time.Second): - } - }() - tco, isTCO := serverInfo.ContainerOrchestrator.(*ctrl_testutil.TestContainerOrchestrator) require.True(t, isTCO, "Container orchestrator should be a TestContainerOrchestrator") diff --git a/test/integration/v2_physical_container_network_controller_test.go b/test/integration/v2_physical_container_network_controller_test.go index e43612df..b450951d 100644 --- a/test/integration/v2_physical_container_network_controller_test.go +++ b/test/integration/v2_physical_container_network_controller_test.go @@ -143,20 +143,11 @@ func TestV2PhysicalContainerNetworkControllerRemovesCreatedNetworkOnDeletion(t * func TestV2PhysicalContainerNetworkControllerReportsDeletionFailure(t *testing.T) { ctx, cancel := testutil.GetTestContext(t, defaultIntegrationTestTimeout) + defer cancel() serverInfo, _, startupErr := StartTestEnvironment(t, ctx, NamespaceController|PhysicalContainerNetworkController, t.Name(), NoSeparateWorkingDir) require.NoError(t, startupErr, "Failed to start the API server") - defer func() { - cancel() - - // Wait for the API server cleanup to complete. - select { - case <-serverInfo.ApiServerDisposalComplete.Wait(): - case <-time.After(5 * time.Second): - } - }() - tco, isTCO := serverInfo.ContainerOrchestrator.(*ctrl_testutil.TestContainerOrchestrator) require.True(t, isTCO, "Container orchestrator should be a TestContainerOrchestrator") @@ -749,6 +740,7 @@ func TestV2PhysicalContainerNetworkControllerReportsMissingRuntimeNetwork(t *tes func TestV2PhysicalContainerNetworkControllerRecoversFromRuntimeFailure(t *testing.T) { ctx, cancel := testutil.GetTestContext(t, defaultIntegrationTestTimeout) + defer cancel() // We are going to use a separate instance of the API server because we need to simulate the // container runtime being unhealthy, and that would interfere with other tests if we used the @@ -756,16 +748,6 @@ func TestV2PhysicalContainerNetworkControllerRecoversFromRuntimeFailure(t *testi serverInfo, _, startupErr := StartTestEnvironment(t, ctx, NamespaceController|PhysicalContainerNetworkController, t.Name(), NoSeparateWorkingDir) require.NoError(t, startupErr, "Failed to start the API server") - defer func() { - cancel() - - // Wait for the API server cleanup to complete. - select { - case <-serverInfo.ApiServerDisposalComplete.Wait(): - case <-time.After(5 * time.Second): - } - }() - tco, isTCO := serverInfo.ContainerOrchestrator.(*ctrl_testutil.TestContainerOrchestrator) require.True(t, isTCO, "Container orchestrator should be a TestContainerOrchestrator") @@ -822,19 +804,11 @@ func TestV2PhysicalContainerNetworkControllerRecoversFromRuntimeFailure(t *testi func TestV2PhysicalContainerNetworkControllerRecoversFromCreateFailure(t *testing.T) { ctx, cancel := testutil.GetTestContext(t, defaultIntegrationTestTimeout) + defer cancel() serverInfo, _, startupErr := StartTestEnvironment(t, ctx, NamespaceController|PhysicalContainerNetworkController, t.Name(), NoSeparateWorkingDir) require.NoError(t, startupErr, "Failed to start the API server") - defer func() { - cancel() - - select { - case <-serverInfo.ApiServerDisposalComplete.Wait(): - case <-time.After(5 * time.Second): - } - }() - tco, isTCO := serverInfo.ContainerOrchestrator.(*ctrl_testutil.TestContainerOrchestrator) require.True(t, isTCO, "Container orchestrator should be a TestContainerOrchestrator") @@ -875,19 +849,11 @@ func TestV2PhysicalContainerNetworkControllerRecoversFromCreateFailure(t *testin func TestV2PhysicalContainerNetworkControllerRecoversFromReplacementFailure(t *testing.T) { ctx, cancel := testutil.GetTestContext(t, defaultIntegrationTestTimeout) + defer cancel() serverInfo, _, startupErr := StartTestEnvironment(t, ctx, NamespaceController|PhysicalContainerNetworkController, t.Name(), NoSeparateWorkingDir) require.NoError(t, startupErr, "Failed to start the API server") - defer func() { - cancel() - - select { - case <-serverInfo.ApiServerDisposalComplete.Wait(): - case <-time.After(5 * time.Second): - } - }() - tco, isTCO := serverInfo.ContainerOrchestrator.(*ctrl_testutil.TestContainerOrchestrator) require.True(t, isTCO, "Container orchestrator should be a TestContainerOrchestrator") diff --git a/test/integration/v2_physical_container_volume_controller_test.go b/test/integration/v2_physical_container_volume_controller_test.go index 00dbe6b2..08ddf234 100644 --- a/test/integration/v2_physical_container_volume_controller_test.go +++ b/test/integration/v2_physical_container_volume_controller_test.go @@ -460,15 +460,9 @@ func TestV2PhysicalContainerVolumeControllerRetriesTransientReplacementRemovalFa func TestV2PhysicalContainerVolumeControllerRetriesTransientReplacementInspectionFailure(t *testing.T) { ctx, cancel := testutil.GetTestContext(t, defaultIntegrationTestTimeout) + defer cancel() serverInfo, _, startupErr := StartTestEnvironment(t, ctx, NamespaceController|PhysicalContainerVolumeController, t.Name(), NoSeparateWorkingDir) require.NoError(t, startupErr) - defer func() { - cancel() - select { - case <-serverInfo.ApiServerDisposalComplete.Wait(): - case <-time.After(5 * time.Second): - } - }() testOrchestrator, isTestOrchestrator := serverInfo.ContainerOrchestrator.(*ctrl_testutil.TestContainerOrchestrator) require.True(t, isTestOrchestrator) @@ -588,15 +582,9 @@ func TestV2PhysicalContainerVolumeControllerDoesNotChurnReadyStatus(t *testing.T func TestV2PhysicalContainerVolumeControllerRecoversFromRuntimeAndCreateFailures(t *testing.T) { ctx, cancel := testutil.GetTestContext(t, defaultIntegrationTestTimeout) + defer cancel() serverInfo, _, startupErr := StartTestEnvironment(t, ctx, NamespaceController|PhysicalContainerVolumeController, t.Name(), NoSeparateWorkingDir) require.NoError(t, startupErr) - defer func() { - cancel() - select { - case <-serverInfo.ApiServerDisposalComplete.Wait(): - case <-time.After(5 * time.Second): - } - }() testOrchestrator, isTestOrchestrator := serverInfo.ContainerOrchestrator.(*ctrl_testutil.TestContainerOrchestrator) require.True(t, isTestOrchestrator) diff --git a/test/integration/volume_controller_test.go b/test/integration/volume_controller_test.go index cc510c89..a70d0e29 100644 --- a/test/integration/volume_controller_test.go +++ b/test/integration/volume_controller_test.go @@ -14,7 +14,6 @@ import ( "sync" "sync/atomic" "testing" - "time" "github.com/cenkalti/backoff/v4" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -495,20 +494,11 @@ func TestContainerVolumeCleanup(t *testing.T) { const testName = "container-volume-cleanup" ctx, cancel := testutil.GetTestContext(t, defaultIntegrationTestTimeout) + defer cancel() serverInfo, _, startupErr := StartTestEnvironment(t, ctx, VolumeController, t.Name(), NoSeparateWorkingDir) require.NoError(t, startupErr, "Failed to start the API server") - defer func() { - cancel() - - // Wait for the API server cleanup to complete. - select { - case <-serverInfo.ApiServerDisposalComplete.Wait(): - case <-time.After(5 * time.Second): - } - }() - adminDocUrl := serverInfo.ClientConfig.Host + apiserver.AdminPathPrefix + apiserver.ExecutionDocument pVol := apiv1.ContainerVolume{ @@ -587,6 +577,7 @@ func TestContainerVolumeCleanup(t *testing.T) { func TestContainerVolumeRuntimeUnhealthy(t *testing.T) { t.Parallel() ctx, cancel := testutil.GetTestContext(t, defaultIntegrationTestTimeout) + defer cancel() const testName = "container-volume-runtime-unhealthy" // We are going to use a separate instance of the API server because we need to simulate container runtime being unhealthy, @@ -595,16 +586,6 @@ func TestContainerVolumeRuntimeUnhealthy(t *testing.T) { serverInfo, _, startupErr := StartTestEnvironment(t, ctx, VolumeController, t.Name(), NoSeparateWorkingDir) require.NoError(t, startupErr, "Failed to start the API server") - defer func() { - cancel() - - // Wait for the API server cleanup to complete. - select { - case <-serverInfo.ApiServerDisposalComplete.Wait(): - case <-time.After(5 * time.Second): - } - }() - vol := apiv1.ContainerVolume{ ObjectMeta: metav1.ObjectMeta{ Name: testName, From bf912635e2e435032af0958a0849749b41b56497 Mon Sep 17 00:00:00 2001 From: David Negstad Date: Mon, 21 Sep 2026 16:24:09 -0700 Subject: [PATCH 3/3] Wait for PTY process exit diagnostics After a read timeout, keep waiting for either the child exit notification or the existing bounded diagnostic timer instead of immediately selecting the expired context. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- internal/termpty/pty_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/internal/termpty/pty_test.go b/internal/termpty/pty_test.go index 282a59be..a39d6061 100644 --- a/internal/termpty/pty_test.go +++ b/internal/termpty/pty_test.go @@ -188,8 +188,6 @@ func requireReadUntil( exitInfo.ExitCode, exitInfo.Err, ) - case <-ctx.Done(): - exitSummary = fmt.Sprintf("process exit was not observed before context completion: %v", ctx.Err()) case <-exitTimer.C: exitSummary = fmt.Sprintf("process exit was not observed within %s", drainExitTimeout) }