From 9a5db339d1af1025b194dee92a9cc3f9ceebb886 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 22 Jul 2022 14:56:02 +0300 Subject: [PATCH 1/7] fix: Change uses of t.Cleanup -> defer in test bodies Mixing t.Cleanup and defer can lead to unexpected order of execution. --- cli/configssh_test.go | 8 ++++---- cli/logout_test.go | 12 ++++++------ cli/portforward_test.go | 9 ++++----- cli/server_test.go | 2 +- coderd/provisionerjobs_internal_test.go | 2 +- coderd/provisionerjobs_test.go | 4 ++-- coderd/templateversions_test.go | 2 +- coderd/workspaceagents_test.go | 20 +++++++++---------- coderd/workspaceapps_test.go | 12 ++++++------ coderd/workspacebuilds_test.go | 2 +- coderd/wsconncache/wsconncache_test.go | 26 ++++++++++++------------- peerbroker/proxy_test.go | 4 ++-- provisioner/echo/serve_test.go | 4 ++-- provisioner/terraform/parse_test.go | 4 ++-- provisionersdk/agent_test.go | 2 +- 15 files changed, 56 insertions(+), 57 deletions(-) diff --git a/cli/configssh_test.go b/cli/configssh_test.go index 9be0c216c5d7c..a606a8414c9a4 100644 --- a/cli/configssh_test.go +++ b/cli/configssh_test.go @@ -109,9 +109,9 @@ func TestConfigSSH(t *testing.T) { agentCloser := agent.New(agentClient.ListenWorkspaceAgent, &agent.Options{ Logger: slogtest.Make(t, nil), }) - t.Cleanup(func() { + defer func() { _ = agentCloser.Close() - }) + }() resources := coderdtest.AwaitWorkspaceAgents(t, client, workspace.LatestBuild.ID) agentConn, err := client.DialWorkspaceAgent(context.Background(), resources[0].Agents[0].ID, nil) require.NoError(t, err) @@ -119,9 +119,9 @@ func TestConfigSSH(t *testing.T) { listener, err := net.Listen("tcp", "127.0.0.1:0") require.NoError(t, err) - t.Cleanup(func() { + defer func() { _ = listener.Close() - }) + }() go func() { for { conn, err := listener.Accept() diff --git a/cli/logout_test.go b/cli/logout_test.go index a0184a565e6f0..b715989627aef 100644 --- a/cli/logout_test.go +++ b/cli/logout_test.go @@ -152,19 +152,19 @@ func TestLogout(t *testing.T) { err = os.Chmod(string(config), 0500) require.NoError(t, err) } - t.Cleanup(func() { + defer func() { if runtime.GOOS == "windows" { // Closing the opened files for cleanup. err = urlFile.Close() - require.NoError(t, err) + assert.NoError(t, err) err = sessionFile.Close() - require.NoError(t, err) + assert.NoError(t, err) } else { // Setting the permissions back for cleanup. - err = os.Chmod(string(config), 0700) - require.NoError(t, err) + err = os.Chmod(string(config), 0o700) + assert.NoError(t, err) } - }) + }() logoutChan := make(chan struct{}) logout, _ := clitest.New(t, "logout", "--global-config", string(config)) diff --git a/cli/portforward_test.go b/cli/portforward_test.go index 5f314a3658850..9f5371b78271d 100644 --- a/cli/portforward_test.go +++ b/cli/portforward_test.go @@ -394,11 +394,7 @@ func runAgent(t *testing.T, client *codersdk.Client, userID uuid.UUID) ([]coders clitest.SetupConfig(t, client, root) errC := make(chan error) agentCtx, agentCancel := context.WithCancel(ctx) - t.Cleanup(func() { - agentCancel() - err := <-errC - require.NoError(t, err) - }) + defer agentCancel() go func() { errC <- cmd.ExecuteContext(agentCtx) }() @@ -407,6 +403,9 @@ func runAgent(t *testing.T, client *codersdk.Client, userID uuid.UUID) ([]coders resources, err := client.WorkspaceResourcesByBuild(context.Background(), workspace.LatestBuild.ID) require.NoError(t, err) + err := <-errC + require.NoError(t, err) + return resources, workspace } diff --git a/cli/server_test.go b/cli/server_test.go index 6d924088439e7..0a25ca3c72bac 100644 --- a/cli/server_test.go +++ b/cli/server_test.go @@ -249,7 +249,7 @@ func TestServer(t *testing.T) { snapshot <- ss }) server := httptest.NewServer(r) - t.Cleanup(server.Close) + defer server.Close() root, _ := clitest.New(t, "server", "--in-memory", "--address", ":0", "--telemetry", "--telemetry-url", server.URL) errC := make(chan error) diff --git a/coderd/provisionerjobs_internal_test.go b/coderd/provisionerjobs_internal_test.go index b09cd3d623d37..6a5cbcccfe5cd 100644 --- a/coderd/provisionerjobs_internal_test.go +++ b/coderd/provisionerjobs_internal_test.go @@ -38,7 +38,7 @@ func TestProvisionerJobLogs_Unit(t *testing.T) { } api := New(&opts) server := httptest.NewServer(api.Handler) - t.Cleanup(server.Close) + defer server.Close() userID := uuid.New() keyID, keySecret, err := generateAPIKeyIDSecret() require.NoError(t, err) diff --git a/coderd/provisionerjobs_test.go b/coderd/provisionerjobs_test.go index 9d35f482dadc6..e93d9b0dc421f 100644 --- a/coderd/provisionerjobs_test.go +++ b/coderd/provisionerjobs_test.go @@ -41,7 +41,7 @@ func TestProvisionerJobLogs(t *testing.T) { coderdtest.AwaitWorkspaceBuildJob(t, client, workspace.LatestBuild.ID) ctx, cancelFunc := context.WithCancel(context.Background()) - t.Cleanup(cancelFunc) + defer cancelFunc() logs, err := client.WorkspaceBuildLogsAfter(ctx, workspace.LatestBuild.ID, before) require.NoError(t, err) for { @@ -77,7 +77,7 @@ func TestProvisionerJobLogs(t *testing.T) { workspace := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) before := database.Now() ctx, cancelFunc := context.WithCancel(context.Background()) - t.Cleanup(cancelFunc) + defer cancelFunc() logs, err := client.WorkspaceBuildLogsAfter(ctx, workspace.LatestBuild.ID, before) require.NoError(t, err) for { diff --git a/coderd/templateversions_test.go b/coderd/templateversions_test.go index a1b987bfcc7a2..8706c77d25019 100644 --- a/coderd/templateversions_test.go +++ b/coderd/templateversions_test.go @@ -380,7 +380,7 @@ func TestTemplateVersionLogs(t *testing.T) { }}, }) ctx, cancelFunc := context.WithCancel(context.Background()) - t.Cleanup(cancelFunc) + defer cancelFunc() logs, err := client.TemplateVersionLogsAfter(ctx, version.ID, before) require.NoError(t, err) for { diff --git a/coderd/workspaceagents_test.go b/coderd/workspaceagents_test.go index b6855d376d417..bbd9540c7cc01 100644 --- a/coderd/workspaceagents_test.go +++ b/coderd/workspaceagents_test.go @@ -108,15 +108,15 @@ func TestWorkspaceAgentListen(t *testing.T) { agentCloser := agent.New(agentClient.ListenWorkspaceAgent, &agent.Options{ Logger: slogtest.Make(t, nil).Named("agent").Leveled(slog.LevelDebug), }) - t.Cleanup(func() { + defer func() { _ = agentCloser.Close() - }) + }() resources := coderdtest.AwaitWorkspaceAgents(t, client, workspace.LatestBuild.ID) conn, err := client.DialWorkspaceAgent(context.Background(), resources[0].Agents[0].ID, nil) require.NoError(t, err) - t.Cleanup(func() { + defer func() { _ = conn.Close() - }) + }() _, err = conn.Ping() require.NoError(t, err) }) @@ -233,9 +233,9 @@ func TestWorkspaceAgentTURN(t *testing.T) { agentCloser := agent.New(agentClient.ListenWorkspaceAgent, &agent.Options{ Logger: slogtest.Make(t, nil), }) - t.Cleanup(func() { + defer func() { _ = agentCloser.Close() - }) + }() resources := coderdtest.AwaitWorkspaceAgents(t, client, workspace.LatestBuild.ID) opts := &peer.ConnOptions{ Logger: slogtest.Make(t, nil).Named("client"), @@ -244,9 +244,9 @@ func TestWorkspaceAgentTURN(t *testing.T) { opts.SettingEngine.SetNetworkTypes([]webrtc.NetworkType{webrtc.NetworkTypeTCP4}) conn, err := client.DialWorkspaceAgent(context.Background(), resources[0].Agents[0].ID, opts) require.NoError(t, err) - t.Cleanup(func() { + defer func() { _ = conn.Close() - }) + }() _, err = conn.Ping() require.NoError(t, err) } @@ -294,9 +294,9 @@ func TestWorkspaceAgentPTY(t *testing.T) { agentCloser := agent.New(agentClient.ListenWorkspaceAgent, &agent.Options{ Logger: slogtest.Make(t, nil), }) - t.Cleanup(func() { + defer func() { _ = agentCloser.Close() - }) + }() resources := coderdtest.AwaitWorkspaceAgents(t, client, workspace.LatestBuild.ID) conn, err := client.WorkspaceAgentReconnectingPTY(context.Background(), resources[0].Agents[0].ID, uuid.New(), 80, 80, "/bin/bash") diff --git a/coderd/workspaceapps_test.go b/coderd/workspaceapps_test.go index 3dde860bed519..69b0493d5bfc7 100644 --- a/coderd/workspaceapps_test.go +++ b/coderd/workspaceapps_test.go @@ -24,16 +24,16 @@ func TestWorkspaceAppsProxyPath(t *testing.T) { // #nosec ln, err := net.Listen("tcp", ":0") require.NoError(t, err) + defer func() { + _ = ln.Close() + }() server := http.Server{ Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) }), } - t.Cleanup(func() { - _ = server.Close() - _ = ln.Close() - }) go server.Serve(ln) + defer server.Close() tcpAddr, _ := ln.Addr().(*net.TCPAddr) client := coderdtest.New(t, &coderdtest.Options{ @@ -78,9 +78,9 @@ func TestWorkspaceAppsProxyPath(t *testing.T) { agentCloser := agent.New(agentClient.ListenWorkspaceAgent, &agent.Options{ Logger: slogtest.Make(t, nil), }) - t.Cleanup(func() { + defer func() { _ = agentCloser.Close() - }) + }() coderdtest.AwaitWorkspaceAgents(t, client, workspace.LatestBuild.ID) client.HTTPClient.CheckRedirect = func(req *http.Request, via []*http.Request) error { return http.ErrUseLastResponse diff --git a/coderd/workspacebuilds_test.go b/coderd/workspacebuilds_test.go index 9c4894ff704fb..56f705940a72b 100644 --- a/coderd/workspacebuilds_test.go +++ b/coderd/workspacebuilds_test.go @@ -327,7 +327,7 @@ func TestWorkspaceBuildLogs(t *testing.T) { template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) workspace := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) ctx, cancelFunc := context.WithCancel(context.Background()) - t.Cleanup(cancelFunc) + defer cancelFunc() logs, err := client.WorkspaceBuildLogsAfter(ctx, workspace.LatestBuild.ID, before.Add(-time.Hour)) require.NoError(t, err) for { diff --git a/coderd/wsconncache/wsconncache_test.go b/coderd/wsconncache/wsconncache_test.go index 34ce39e20b86d..46e7df383e552 100644 --- a/coderd/wsconncache/wsconncache_test.go +++ b/coderd/wsconncache/wsconncache_test.go @@ -40,9 +40,9 @@ func TestCache(t *testing.T) { cache := wsconncache.New(func(r *http.Request, id uuid.UUID) (*agent.Conn, error) { return setupAgent(t, agent.Metadata{}, 0), nil }, 0) - t.Cleanup(func() { + defer func() { _ = cache.Close() - }) + }() conn1, _, err := cache.Acquire(httptest.NewRequest(http.MethodGet, "/", nil), uuid.Nil) require.NoError(t, err) conn2, _, err := cache.Acquire(httptest.NewRequest(http.MethodGet, "/", nil), uuid.Nil) @@ -56,9 +56,9 @@ func TestCache(t *testing.T) { called.Add(1) return setupAgent(t, agent.Metadata{}, 0), nil }, time.Microsecond) - t.Cleanup(func() { + defer func() { _ = cache.Close() - }) + }() conn, release, err := cache.Acquire(httptest.NewRequest(http.MethodGet, "/", nil), uuid.Nil) require.NoError(t, err) release() @@ -74,9 +74,9 @@ func TestCache(t *testing.T) { cache := wsconncache.New(func(r *http.Request, id uuid.UUID) (*agent.Conn, error) { return setupAgent(t, agent.Metadata{}, 0), nil }, time.Microsecond) - t.Cleanup(func() { + defer func() { _ = cache.Close() - }) + }() conn, release, err := cache.Acquire(httptest.NewRequest(http.MethodGet, "/", nil), uuid.Nil) require.NoError(t, err) time.Sleep(time.Millisecond) @@ -87,9 +87,9 @@ func TestCache(t *testing.T) { t.Parallel() random, err := net.Listen("tcp", "127.0.0.1:0") require.NoError(t, err) - t.Cleanup(func() { + defer func() { _ = random.Close() - }) + }() tcpAddr, valid := random.Addr().(*net.TCPAddr) require.True(t, valid) @@ -98,17 +98,17 @@ func TestCache(t *testing.T) { w.WriteHeader(http.StatusOK) }), } - t.Cleanup(func() { + defer func() { _ = server.Close() - }) + }() go server.Serve(random) cache := wsconncache.New(func(r *http.Request, id uuid.UUID) (*agent.Conn, error) { return setupAgent(t, agent.Metadata{}, 0), nil }, time.Microsecond) - t.Cleanup(func() { + defer func() { _ = cache.Close() - }) + }() var wg sync.WaitGroup // Perform many requests in parallel to simulate @@ -132,7 +132,7 @@ func TestCache(t *testing.T) { res := httptest.NewRecorder() proxy.ServeHTTP(res, req) res.Result().Body.Close() - require.Equal(t, http.StatusOK, res.Result().StatusCode) + assert.Equal(t, http.StatusOK, res.Result().StatusCode) }() } wg.Wait() diff --git a/peerbroker/proxy_test.go b/peerbroker/proxy_test.go index 71db969cd652b..80fe405c24fcf 100644 --- a/peerbroker/proxy_test.go +++ b/peerbroker/proxy_test.go @@ -43,9 +43,9 @@ func TestProxy(t *testing.T) { Pubsub: pubsub, }) require.NoError(t, err) - t.Cleanup(func() { + defer func() { _ = proxyCloser.Close() - }) + }() var wg sync.WaitGroup wg.Add(1) diff --git a/provisioner/echo/serve_test.go b/provisioner/echo/serve_test.go index cad2755f2a027..1c2d6ddfbcf0e 100644 --- a/provisioner/echo/serve_test.go +++ b/provisioner/echo/serve_test.go @@ -25,11 +25,11 @@ func TestEcho(t *testing.T) { // Create an in-memory provisioner to communicate with. client, server := provisionersdk.TransportPipe() ctx, cancelFunc := context.WithCancel(context.Background()) - t.Cleanup(func() { + defer func() { _ = client.Close() _ = server.Close() cancelFunc() - }) + }() go func() { err := echo.Serve(ctx, fs, &provisionersdk.ServeOptions{ Listener: server, diff --git a/provisioner/terraform/parse_test.go b/provisioner/terraform/parse_test.go index dab994d335b7d..0a0f616885d8f 100644 --- a/provisioner/terraform/parse_test.go +++ b/provisioner/terraform/parse_test.go @@ -23,11 +23,11 @@ func TestParse(t *testing.T) { // Create an in-memory provisioner to communicate with. client, server := provisionersdk.TransportPipe() ctx, cancelFunc := context.WithCancel(context.Background()) - t.Cleanup(func() { + defer func() { _ = client.Close() _ = server.Close() cancelFunc() - }) + }() go func() { err := terraform.Serve(ctx, &terraform.ServeOptions{ ServeOptions: &provisionersdk.ServeOptions{ diff --git a/provisionersdk/agent_test.go b/provisionersdk/agent_test.go index 3df0da6e805ca..43fe411954cd9 100644 --- a/provisionersdk/agent_test.go +++ b/provisionersdk/agent_test.go @@ -35,7 +35,7 @@ func TestAgentScript(t *testing.T) { render.Status(r, http.StatusOK) render.Data(rw, r, content) })) - t.Cleanup(srv.Close) + defer srv.Close() srvURL, err := url.Parse(srv.URL) require.NoError(t, err) From 628d9f207fde76be696ac8c18bf77bc03b52b628 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 22 Jul 2022 15:11:37 +0300 Subject: [PATCH 2/7] fix: Ensure t.Cleanup is not aborted by require --- cli/configssh_test.go | 3 --- coderd/coderd_test.go | 9 ++++++--- coderd/coderdtest/coderdtest.go | 15 ++++++++++----- 3 files changed, 16 insertions(+), 11 deletions(-) diff --git a/cli/configssh_test.go b/cli/configssh_test.go index a606a8414c9a4..beb95b58f575e 100644 --- a/cli/configssh_test.go +++ b/cli/configssh_test.go @@ -134,9 +134,6 @@ func TestConfigSSH(t *testing.T) { go io.Copy(ssh, conn) } }() - t.Cleanup(func() { - _ = listener.Close() - }) sshConfigFile, _ := sshConfigFileNames(t) diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index d7103d39f9d89..9ce15ecc3af91 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -94,6 +94,8 @@ func TestAuthorizeAllEndpoints(t *testing.T) { t.Cleanup(func() { close(tickerCh) }) ctx, cancelFunc := context.WithCancel(context.Background()) + t.Cleanup(cancelFunc) + lifecycleExecutor := executor.New( ctx, db, @@ -109,9 +111,13 @@ func TestAuthorizeAllEndpoints(t *testing.T) { srv.Start() serverURL, err := url.Parse(srv.URL) require.NoError(t, err) + t.Cleanup(srv.Close) turnServer, err := turnconn.New(nil) require.NoError(t, err) + t.Cleanup(func() { + _ = turnServer.Close() + }) validator, err := idtoken.NewValidator(ctx, option.WithoutAuthentication()) require.NoError(t, err) @@ -138,9 +144,6 @@ func TestAuthorizeAllEndpoints(t *testing.T) { _ = coderdtest.NewProvisionerDaemon(t, coderAPI) t.Cleanup(func() { - cancelFunc() - _ = turnServer.Close() - srv.Close() _ = coderAPI.Close() }) diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index eb5d089201a63..c4eb863534134 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -143,6 +143,8 @@ func newWithCloser(t *testing.T, options *Options) (*codersdk.Client, io.Closer) } ctx, cancelFunc := context.WithCancel(context.Background()) + t.Cleanup(cancelFunc) + lifecycleExecutor := executor.New( ctx, db, @@ -156,6 +158,8 @@ func newWithCloser(t *testing.T, options *Options) (*codersdk.Client, io.Closer) return ctx } srv.Start() + t.Cleanup(srv.Close) + serverURL, err := url.Parse(srv.URL) require.NoError(t, err) @@ -166,6 +170,9 @@ func newWithCloser(t *testing.T, options *Options) (*codersdk.Client, io.Closer) turnServer, err := turnconn.New(nil) require.NoError(t, err) + t.Cleanup(func() { + _ = turnServer.Close() + }) // We set the handler after server creation for the access URL. coderAPI := coderd.New(&coderd.Options{ @@ -188,18 +195,16 @@ func newWithCloser(t *testing.T, options *Options) (*codersdk.Client, io.Closer) Authorizer: options.Authorizer, Telemetry: telemetry.NewNoop(), }) + t.Cleanup(func() { + _ = coderAPI.Close() + }) srv.Config.Handler = coderAPI.Handler var provisionerCloser io.Closer = nopcloser{} if options.IncludeProvisionerD { provisionerCloser = NewProvisionerDaemon(t, coderAPI) } - t.Cleanup(func() { - cancelFunc() - _ = turnServer.Close() - srv.Close() - _ = coderAPI.Close() _ = provisionerCloser.Close() }) From 2334d8abd1042e8c19e7ebfb6ab41f3595d76e2f Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 22 Jul 2022 15:12:29 +0300 Subject: [PATCH 3/7] chore: Add helper annotations --- provisionerd/provisionerd_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/provisionerd/provisionerd_test.go b/provisionerd/provisionerd_test.go index ecd4552ab8989..fd724babea853 100644 --- a/provisionerd/provisionerd_test.go +++ b/provisionerd/provisionerd_test.go @@ -962,6 +962,7 @@ func createProvisionerd(t *testing.T, dialer provisionerd.Dialer, provisioners p // Creates a provisionerd protobuf client that's connected // to the server implementation provided. func createProvisionerDaemonClient(t *testing.T, server provisionerDaemonTestServer) proto.DRPCProvisionerDaemonClient { + t.Helper() if server.failJob == nil { // Default to asserting the error from the failure, otherwise // it can be lost in tests! @@ -990,6 +991,7 @@ func createProvisionerDaemonClient(t *testing.T, server provisionerDaemonTestSer // Creates a provisioner protobuf client that's connected // to the server implementation provided. func createProvisionerClient(t *testing.T, server provisionerTestServer) sdkproto.DRPCProvisionerClient { + t.Helper() clientPipe, serverPipe := provisionersdk.TransportPipe() t.Cleanup(func() { _ = clientPipe.Close() From d263af2d9246efd81e8276426647f65a93d7a266 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 22 Jul 2022 15:20:48 +0300 Subject: [PATCH 4/7] fix: Revert bad change --- cli/portforward_test.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/cli/portforward_test.go b/cli/portforward_test.go index 9f5371b78271d..5f314a3658850 100644 --- a/cli/portforward_test.go +++ b/cli/portforward_test.go @@ -394,7 +394,11 @@ func runAgent(t *testing.T, client *codersdk.Client, userID uuid.UUID) ([]coders clitest.SetupConfig(t, client, root) errC := make(chan error) agentCtx, agentCancel := context.WithCancel(ctx) - defer agentCancel() + t.Cleanup(func() { + agentCancel() + err := <-errC + require.NoError(t, err) + }) go func() { errC <- cmd.ExecuteContext(agentCtx) }() @@ -403,9 +407,6 @@ func runAgent(t *testing.T, client *codersdk.Client, userID uuid.UUID) ([]coders resources, err := client.WorkspaceResourcesByBuild(context.Background(), workspace.LatestBuild.ID) require.NoError(t, err) - err := <-errC - require.NoError(t, err) - return resources, workspace } From 7b356b3da1e94b41ef2fe96e62b51ab4c7b5a93b Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 22 Jul 2022 15:22:09 +0300 Subject: [PATCH 5/7] fix: Bad placement --- coderd/coderd_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index 9ce15ecc3af91..d2c2055914dd8 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -109,9 +109,9 @@ func TestAuthorizeAllEndpoints(t *testing.T) { return ctx } srv.Start() + t.Cleanup(srv.Close) serverURL, err := url.Parse(srv.URL) require.NoError(t, err) - t.Cleanup(srv.Close) turnServer, err := turnconn.New(nil) require.NoError(t, err) From f245f64b849550feed4acbdaf51a7e0f9e22efdb Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 22 Jul 2022 15:54:47 +0300 Subject: [PATCH 6/7] fix: Run ctx cancel first in t.Cleanup --- coderd/coderd_test.go | 2 +- coderd/coderdtest/coderdtest.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index d2c2055914dd8..7eb9167caa05a 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -94,7 +94,7 @@ func TestAuthorizeAllEndpoints(t *testing.T) { t.Cleanup(func() { close(tickerCh) }) ctx, cancelFunc := context.WithCancel(context.Background()) - t.Cleanup(cancelFunc) + defer t.Cleanup(cancelFunc) // Defer to ensure cancelFunc is executed first. lifecycleExecutor := executor.New( ctx, diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index c4eb863534134..eafac63ed4677 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -143,7 +143,7 @@ func newWithCloser(t *testing.T, options *Options) (*codersdk.Client, io.Closer) } ctx, cancelFunc := context.WithCancel(context.Background()) - t.Cleanup(cancelFunc) + defer t.Cleanup(cancelFunc) // Defer to ensure cancelFunc is executed first. lifecycleExecutor := executor.New( ctx, From b624871867098bf7d238d1cb7fc24cd728554034 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 22 Jul 2022 16:56:08 +0300 Subject: [PATCH 7/7] fix: Revert bad cleanup changes --- coderd/workspaceapps_test.go | 12 ++++++------ provisioner/echo/serve_test.go | 4 ++-- provisioner/terraform/parse_test.go | 4 ++-- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/coderd/workspaceapps_test.go b/coderd/workspaceapps_test.go index 69b0493d5bfc7..3dde860bed519 100644 --- a/coderd/workspaceapps_test.go +++ b/coderd/workspaceapps_test.go @@ -24,16 +24,16 @@ func TestWorkspaceAppsProxyPath(t *testing.T) { // #nosec ln, err := net.Listen("tcp", ":0") require.NoError(t, err) - defer func() { - _ = ln.Close() - }() server := http.Server{ Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) }), } + t.Cleanup(func() { + _ = server.Close() + _ = ln.Close() + }) go server.Serve(ln) - defer server.Close() tcpAddr, _ := ln.Addr().(*net.TCPAddr) client := coderdtest.New(t, &coderdtest.Options{ @@ -78,9 +78,9 @@ func TestWorkspaceAppsProxyPath(t *testing.T) { agentCloser := agent.New(agentClient.ListenWorkspaceAgent, &agent.Options{ Logger: slogtest.Make(t, nil), }) - defer func() { + t.Cleanup(func() { _ = agentCloser.Close() - }() + }) coderdtest.AwaitWorkspaceAgents(t, client, workspace.LatestBuild.ID) client.HTTPClient.CheckRedirect = func(req *http.Request, via []*http.Request) error { return http.ErrUseLastResponse diff --git a/provisioner/echo/serve_test.go b/provisioner/echo/serve_test.go index 1c2d6ddfbcf0e..cad2755f2a027 100644 --- a/provisioner/echo/serve_test.go +++ b/provisioner/echo/serve_test.go @@ -25,11 +25,11 @@ func TestEcho(t *testing.T) { // Create an in-memory provisioner to communicate with. client, server := provisionersdk.TransportPipe() ctx, cancelFunc := context.WithCancel(context.Background()) - defer func() { + t.Cleanup(func() { _ = client.Close() _ = server.Close() cancelFunc() - }() + }) go func() { err := echo.Serve(ctx, fs, &provisionersdk.ServeOptions{ Listener: server, diff --git a/provisioner/terraform/parse_test.go b/provisioner/terraform/parse_test.go index 0a0f616885d8f..dab994d335b7d 100644 --- a/provisioner/terraform/parse_test.go +++ b/provisioner/terraform/parse_test.go @@ -23,11 +23,11 @@ func TestParse(t *testing.T) { // Create an in-memory provisioner to communicate with. client, server := provisionersdk.TransportPipe() ctx, cancelFunc := context.WithCancel(context.Background()) - defer func() { + t.Cleanup(func() { _ = client.Close() _ = server.Close() cancelFunc() - }() + }) go func() { err := terraform.Serve(ctx, &terraform.ServeOptions{ ServeOptions: &provisionersdk.ServeOptions{