From 404616aec78f7cd2b0cc287062662680ce916fe0 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 23 May 2022 14:05:25 +0300 Subject: [PATCH 1/5] fix: Try to fix cli portforward test flakes --- cli/portforward_test.go | 49 +++++++++++++++++++++++++---------------- 1 file changed, 30 insertions(+), 19 deletions(-) diff --git a/cli/portforward_test.go b/cli/portforward_test.go index f400587d4ba0e..1999ac924d5fc 100644 --- a/cli/portforward_test.go +++ b/cli/portforward_test.go @@ -141,13 +141,16 @@ func TestPortForward(t *testing.T) { }, } - for _, c := range cases { //nolint:paralleltest // the `c := c` confuses the linter + //nolint:paralleltest + for _, c := range cases { c := c + // Avoid parallel test here because setupLocal reserves + // a free open port which is not guaranteed to be free + // after the listener closes. + //nolint:paralleltest t.Run(c.name, func(t *testing.T) { - t.Parallel() - + //nolint:paralleltest t.Run("OnePort", func(t *testing.T) { - t.Parallel() var ( client = coderdtest.New(t, &coderdtest.Options{IncludeProvisionerD: true}) user = coderdtest.CreateFirstUser(t, client) @@ -170,10 +173,9 @@ func TestPortForward(t *testing.T) { cmd.SetOut(io.MultiWriter(buf, os.Stderr)) ctx, cancel := context.WithCancel(context.Background()) defer cancel() + errC := make(chan error) go func() { - err := cmd.ExecuteContext(ctx) - require.Error(t, err) - require.ErrorIs(t, err, context.Canceled) + errC <- cmd.ExecuteContext(ctx) }() waitForPortForwardReady(t, buf) @@ -188,10 +190,13 @@ func TestPortForward(t *testing.T) { defer c2.Close() testDial(t, c2) testDial(t, c1) + + err = <-errC + require.ErrorIs(t, err, context.Canceled) }) + //nolint:paralleltest t.Run("TwoPorts", func(t *testing.T) { - t.Parallel() var ( client = coderdtest.New(t, &coderdtest.Options{IncludeProvisionerD: true}) user = coderdtest.CreateFirstUser(t, client) @@ -218,10 +223,9 @@ func TestPortForward(t *testing.T) { cmd.SetOut(io.MultiWriter(buf, os.Stderr)) ctx, cancel := context.WithCancel(context.Background()) defer cancel() + errC := make(chan error) go func() { - err := cmd.ExecuteContext(ctx) - require.Error(t, err) - require.ErrorIs(t, err, context.Canceled) + errC <- cmd.ExecuteContext(ctx) }() waitForPortForwardReady(t, buf) @@ -236,13 +240,16 @@ func TestPortForward(t *testing.T) { defer c2.Close() testDial(t, c2) testDial(t, c1) + + err = <-errC + require.ErrorIs(t, err, context.Canceled) }) }) } // Test doing a TCP -> Unix forward. + //nolint:paralleltest t.Run("TCP2Unix", func(t *testing.T) { - t.Parallel() var ( client = coderdtest.New(t, &coderdtest.Options{IncludeProvisionerD: true}) user = coderdtest.CreateFirstUser(t, client) @@ -273,10 +280,9 @@ func TestPortForward(t *testing.T) { cmd.SetOut(io.MultiWriter(buf, os.Stderr)) ctx, cancel := context.WithCancel(context.Background()) defer cancel() + errC := make(chan error) go func() { - err := cmd.ExecuteContext(ctx) - require.Error(t, err) - require.ErrorIs(t, err, context.Canceled) + errC <- cmd.ExecuteContext(ctx) }() waitForPortForwardReady(t, buf) @@ -291,11 +297,14 @@ func TestPortForward(t *testing.T) { defer c2.Close() testDial(t, c2) testDial(t, c1) + + err = <-errC + require.ErrorIs(t, err, context.Canceled) }) // Test doing TCP, UDP and Unix at the same time. + //nolint:paralleltest t.Run("All", func(t *testing.T) { - t.Parallel() var ( client = coderdtest.New(t, &coderdtest.Options{IncludeProvisionerD: true}) user = coderdtest.CreateFirstUser(t, client) @@ -334,10 +343,9 @@ func TestPortForward(t *testing.T) { cmd.SetOut(io.MultiWriter(buf, os.Stderr)) ctx, cancel := context.WithCancel(context.Background()) defer cancel() + errC := make(chan error) go func() { - err := cmd.ExecuteContext(ctx) - require.Error(t, err) - require.ErrorIs(t, err, context.Canceled) + errC <- cmd.ExecuteContext(ctx) }() waitForPortForwardReady(t, buf) @@ -359,6 +367,9 @@ func TestPortForward(t *testing.T) { for i := len(conns) - 1; i >= 0; i-- { testDial(t, conns[i]) } + + err := <-errC + require.ErrorIs(t, err, context.Canceled) }) } From b4ec075b00504ca0ef0a897dd2c00ea51c3111de Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 23 May 2022 14:09:00 +0300 Subject: [PATCH 2/5] Restore linter comment --- cli/portforward_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cli/portforward_test.go b/cli/portforward_test.go index 1999ac924d5fc..b5e4032eb3b54 100644 --- a/cli/portforward_test.go +++ b/cli/portforward_test.go @@ -141,8 +141,7 @@ func TestPortForward(t *testing.T) { }, } - //nolint:paralleltest - for _, c := range cases { + for _, c := range cases { //nolint:paralleltest // the `c := c` confuses the linter c := c // Avoid parallel test here because setupLocal reserves // a free open port which is not guaranteed to be free From f0223ac3b5ca1858ed7d593bc9c0a2b2fb33007f Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 23 May 2022 14:33:32 +0300 Subject: [PATCH 3/5] Cancel context --- cli/portforward_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/cli/portforward_test.go b/cli/portforward_test.go index b5e4032eb3b54..07900ad0ebbbf 100644 --- a/cli/portforward_test.go +++ b/cli/portforward_test.go @@ -190,6 +190,7 @@ func TestPortForward(t *testing.T) { testDial(t, c2) testDial(t, c1) + cancel() err = <-errC require.ErrorIs(t, err, context.Canceled) }) @@ -240,6 +241,7 @@ func TestPortForward(t *testing.T) { testDial(t, c2) testDial(t, c1) + cancel() err = <-errC require.ErrorIs(t, err, context.Canceled) }) @@ -297,6 +299,7 @@ func TestPortForward(t *testing.T) { testDial(t, c2) testDial(t, c1) + cancel() err = <-errC require.ErrorIs(t, err, context.Canceled) }) @@ -367,6 +370,7 @@ func TestPortForward(t *testing.T) { testDial(t, conns[i]) } + cancel() err := <-errC require.ErrorIs(t, err, context.Canceled) }) From f137169fbda6e3cb7b28dae24b653d5b9e1bc40b Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 23 May 2022 20:43:38 +0300 Subject: [PATCH 4/5] fix: Guard against agent exit outside test func --- cli/portforward_test.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/cli/portforward_test.go b/cli/portforward_test.go index 07900ad0ebbbf..1c873309cf325 100644 --- a/cli/portforward_test.go +++ b/cli/portforward_test.go @@ -416,10 +416,15 @@ func runAgent(t *testing.T, client *codersdk.Client, userID uuid.UUID) ([]coders // Start workspace agent in a goroutine cmd, root := clitest.New(t, "agent", "--agent-token", agentToken, "--agent-url", client.URL.String()) clitest.SetupConfig(t, client, root) + errC := make(chan error) agentCtx, agentCancel := context.WithCancel(ctx) - t.Cleanup(agentCancel) + t.Cleanup(func() { + agentCancel() + err := <-errC + require.NoError(t, err) + }) go func() { - err := cmd.ExecuteContext(agentCtx) + errC <- cmd.ExecuteContext(agentCtx) require.NoError(t, err) }() From 0259a190153e95423f080af3739f3ca5a85246c5 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 23 May 2022 23:20:59 +0300 Subject: [PATCH 5/5] fix: Improve test teardown in setupTestListener, cleanup --- cli/portforward_test.go | 42 ++++++++++++++++++++--------------------- 1 file changed, 20 insertions(+), 22 deletions(-) diff --git a/cli/portforward_test.go b/cli/portforward_test.go index 1c873309cf325..6427eda3fb605 100644 --- a/cli/portforward_test.go +++ b/cli/portforward_test.go @@ -154,11 +154,8 @@ func TestPortForward(t *testing.T) { client = coderdtest.New(t, &coderdtest.Options{IncludeProvisionerD: true}) user = coderdtest.CreateFirstUser(t, client) _, workspace = runAgent(t, client, user.UserID) - l1, p1 = setupTestListener(t, c.setupRemote(t)) + p1 = setupTestListener(t, c.setupRemote(t)) ) - t.Cleanup(func() { - _ = l1.Close() - }) // Create a flag that forwards from local to listener 1. localAddress, localFlag := c.setupLocal(t) @@ -201,13 +198,9 @@ func TestPortForward(t *testing.T) { client = coderdtest.New(t, &coderdtest.Options{IncludeProvisionerD: true}) user = coderdtest.CreateFirstUser(t, client) _, workspace = runAgent(t, client, user.UserID) - l1, p1 = setupTestListener(t, c.setupRemote(t)) - l2, p2 = setupTestListener(t, c.setupRemote(t)) + p1 = setupTestListener(t, c.setupRemote(t)) + p2 = setupTestListener(t, c.setupRemote(t)) ) - t.Cleanup(func() { - _ = l1.Close() - _ = l2.Close() - }) // Create a flags for listener 1 and listener 2. localAddress1, localFlag1 := c.setupLocal(t) @@ -262,11 +255,8 @@ func TestPortForward(t *testing.T) { unixCase = cases[2] // Setup remote Unix listener. - l1, p1 = setupTestListener(t, unixCase.setupRemote(t)) + p1 = setupTestListener(t, unixCase.setupRemote(t)) ) - t.Cleanup(func() { - _ = l1.Close() - }) // Create a flag that forwards from local TCP to Unix listener 1. // Notably this is a --unix flag. @@ -324,10 +314,7 @@ func TestPortForward(t *testing.T) { continue } - l, p := setupTestListener(t, c.setupRemote(t)) - t.Cleanup(func() { - _ = l.Close() - }) + p := setupTestListener(t, c.setupRemote(t)) localAddress, localFlag := c.setupLocal(t) dials = append(dials, addr{ @@ -425,7 +412,6 @@ func runAgent(t *testing.T, client *codersdk.Client, userID uuid.UUID) ([]coders }) go func() { errC <- cmd.ExecuteContext(agentCtx) - require.NoError(t, err) }() coderdtest.AwaitWorkspaceAgents(t, client, workspace.LatestBuild.ID) @@ -437,18 +423,30 @@ func runAgent(t *testing.T, client *codersdk.Client, userID uuid.UUID) ([]coders // setupTestListener starts accepting connections and echoing a single packet. // Returns the listener and the listen port or Unix path. -func setupTestListener(t *testing.T, l net.Listener) (net.Listener, string) { +func setupTestListener(t *testing.T, l net.Listener) string { + // Wait for listener to completely exit before releasing. + done := make(chan struct{}) t.Cleanup(func() { _ = l.Close() + <-done }) go func() { + defer close(done) + // Guard against testAccept running require after test completion. + var wg sync.WaitGroup + defer wg.Wait() + for { c, err := l.Accept() if err != nil { return } - go testAccept(t, c) + wg.Add(1) + go func() { + testAccept(t, c) + wg.Done() + }() } }() @@ -459,7 +457,7 @@ func setupTestListener(t *testing.T, l net.Listener) (net.Listener, string) { addr = port } - return l, addr + return addr } var dialTestPayload = []byte("dean-was-here123")