From 97c3b066c3ca3ddeec7f754a0975cc1f9972453b Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 23 May 2022 14:23:03 +0300 Subject: [PATCH 01/10] fix: Guard against CLI cmd running after test exit --- cli/agent_test.go | 39 +++++++++++++++++++++------------------ cli/gitssh_test.go | 7 +++++-- 2 files changed, 26 insertions(+), 20 deletions(-) diff --git a/cli/agent_test.go b/cli/agent_test.go index 71bc7b1d6949c..6058b601bc2dc 100644 --- a/cli/agent_test.go +++ b/cli/agent_test.go @@ -49,13 +49,12 @@ func TestWorkspaceAgent(t *testing.T) { cmd, _ := clitest.New(t, "agent", "--auth", "azure-instance-identity", "--agent-url", client.URL.String()) ctx, cancelFunc := context.WithCancel(context.Background()) defer cancelFunc() + errC := make(chan error) go func() { - // A linting error occurs for weakly typing the context value here, - // but it seems reasonable for a one-off test. - // nolint - ctx = context.WithValue(ctx, "azure-client", metadataClient) - err := cmd.ExecuteContext(ctx) - require.NoError(t, err) + // A linting error occurs for weakly typing the context value here. + //nolint // The above seems reasonable for a one-off test. + ctx := context.WithValue(ctx, "azure-client", metadataClient) + errC <- cmd.ExecuteContext(ctx) }() coderdtest.AwaitWorkspaceAgents(t, client, workspace.LatestBuild.ID) resources, err := client.WorkspaceResourcesByBuild(ctx, workspace.LatestBuild.ID) @@ -66,6 +65,8 @@ func TestWorkspaceAgent(t *testing.T) { _, err = dialer.Ping() require.NoError(t, err) cancelFunc() + err = <-errC + require.NoError(t, err) }) t.Run("AWS", func(t *testing.T) { @@ -103,13 +104,12 @@ func TestWorkspaceAgent(t *testing.T) { cmd, _ := clitest.New(t, "agent", "--auth", "aws-instance-identity", "--agent-url", client.URL.String()) ctx, cancelFunc := context.WithCancel(context.Background()) defer cancelFunc() + errC := make(chan error) go func() { - // A linting error occurs for weakly typing the context value here, - // but it seems reasonable for a one-off test. - // nolint - ctx = context.WithValue(ctx, "aws-client", metadataClient) - err := cmd.ExecuteContext(ctx) - require.NoError(t, err) + // A linting error occurs for weakly typing the context value here. + //nolint // The above seems reasonable for a one-off test. + ctx := context.WithValue(ctx, "aws-client", metadataClient) + errC <- cmd.ExecuteContext(ctx) }() coderdtest.AwaitWorkspaceAgents(t, client, workspace.LatestBuild.ID) resources, err := client.WorkspaceResourcesByBuild(ctx, workspace.LatestBuild.ID) @@ -120,6 +120,8 @@ func TestWorkspaceAgent(t *testing.T) { _, err = dialer.Ping() require.NoError(t, err) cancelFunc() + err = <-errC + require.NoError(t, err) }) t.Run("GoogleCloud", func(t *testing.T) { @@ -157,13 +159,12 @@ func TestWorkspaceAgent(t *testing.T) { cmd, _ := clitest.New(t, "agent", "--auth", "google-instance-identity", "--agent-url", client.URL.String()) ctx, cancelFunc := context.WithCancel(context.Background()) defer cancelFunc() + errC := make(chan error) go func() { - // A linting error occurs for weakly typing the context value here, - // but it seems reasonable for a one-off test. - // nolint - ctx = context.WithValue(ctx, "gcp-client", metadata) - err := cmd.ExecuteContext(ctx) - require.NoError(t, err) + // A linting error occurs for weakly typing the context value here. + //nolint // The above seems reasonable for a one-off test. + ctx := context.WithValue(ctx, "gcp-client", metadata) + errC <- cmd.ExecuteContext(ctx) }() coderdtest.AwaitWorkspaceAgents(t, client, workspace.LatestBuild.ID) resources, err := client.WorkspaceResourcesByBuild(ctx, workspace.LatestBuild.ID) @@ -174,5 +175,7 @@ func TestWorkspaceAgent(t *testing.T) { _, err = dialer.Ping() require.NoError(t, err) cancelFunc() + err = <-errC + require.NoError(t, err) }) } diff --git a/cli/gitssh_test.go b/cli/gitssh_test.go index 084407c9e3a72..a5182d832a8b4 100644 --- a/cli/gitssh_test.go +++ b/cli/gitssh_test.go @@ -63,9 +63,9 @@ func TestGitSSH(t *testing.T) { clitest.SetupConfig(t, agentClient, root) ctx, cancelFunc := context.WithCancel(context.Background()) defer cancelFunc() + errC := make(chan error) go func() { - err := cmd.ExecuteContext(ctx) - require.NoError(t, err) + errC <- cmd.ExecuteContext(ctx) }() coderdtest.AwaitWorkspaceAgents(t, client, workspace.LatestBuild.ID) @@ -103,5 +103,8 @@ func TestGitSSH(t *testing.T) { err = cmd.ExecuteContext(context.Background()) require.NoError(t, err) require.EqualValues(t, 1, inc) + + err = <-errC + require.NoError(t, err) }) } From 21da84554f9af26aeda703e4eef4507440befc8f Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 23 May 2022 14:35:20 +0300 Subject: [PATCH 02/10] Cancel context --- cli/gitssh_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cli/gitssh_test.go b/cli/gitssh_test.go index a5182d832a8b4..3023afe3b8af1 100644 --- a/cli/gitssh_test.go +++ b/cli/gitssh_test.go @@ -104,6 +104,7 @@ func TestGitSSH(t *testing.T) { require.NoError(t, err) require.EqualValues(t, 1, inc) + cancelFunc() err = <-errC require.NoError(t, err) }) From 53417bb09aecdf9fcd682eb2d85051465ed0372f Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 23 May 2022 13:24:03 +0100 Subject: [PATCH 03/10] fix: cli: avoid calling t.FailNow in non-test-main goroutine --- cli/userlist_test.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/cli/userlist_test.go b/cli/userlist_test.go index 37baed59d3d67..76f9488ec996a 100644 --- a/cli/userlist_test.go +++ b/cli/userlist_test.go @@ -16,15 +16,13 @@ func TestUserList(t *testing.T) { coderdtest.CreateFirstUser(t, client) cmd, root := clitest.New(t, "users", "list") clitest.SetupConfig(t, client, root) - doneChan := make(chan struct{}) pty := ptytest.New(t) cmd.SetIn(pty.Input()) cmd.SetOut(pty.Output()) + errC := make(chan error) go func() { - defer close(doneChan) - err := cmd.Execute() - require.NoError(t, err) + errC <- cmd.Execute() }() + require.NoError(t, <-errC) pty.ExpectMatch("coder.com") - <-doneChan } From 5530f2addaacf12fccf3d9d1c0c56c6274c93295 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 23 May 2022 14:07:02 +0100 Subject: [PATCH 04/10] fix: cli: server_test: avoid calling t.FailNow outside main goroutine --- cli/server_test.go | 81 ++++++++++++++++++++++------------------------ 1 file changed, 39 insertions(+), 42 deletions(-) diff --git a/cli/server_test.go b/cli/server_test.go index 1f2525975a836..cd67bc6df4ac4 100644 --- a/cli/server_test.go +++ b/cli/server_test.go @@ -17,7 +17,6 @@ import ( "os" "runtime" "strings" - "sync" "testing" "time" @@ -45,12 +44,11 @@ func TestServer(t *testing.T) { require.NoError(t, err) defer closeFunc() ctx, cancelFunc := context.WithCancel(context.Background()) - done := make(chan struct{}) + defer cancelFunc() root, cfg := clitest.New(t, "server", "--address", ":0", "--postgres-url", connectionURL) + errC := make(chan error) go func() { - defer close(done) - err = root.ExecuteContext(ctx) - require.ErrorIs(t, err, context.Canceled) + errC <- root.ExecuteContext(ctx) }() var client *codersdk.Client require.Eventually(t, func() bool { @@ -71,8 +69,9 @@ func TestServer(t *testing.T) { }) require.NoError(t, err) cancelFunc() - <-done + require.ErrorIs(t, <-errC, context.Canceled) }) + t.Run("Development", func(t *testing.T) { t.Parallel() ctx, cancelFunc := context.WithCancel(context.Background()) @@ -82,26 +81,12 @@ func TestServer(t *testing.T) { root, cfg := clitest.New(t, "server", "--dev", "--tunnel=false", "--address", ":0") var buf strings.Builder + errC := make(chan error) root.SetOutput(&buf) - var wg sync.WaitGroup - wg.Add(1) go func() { - defer wg.Done() - - err := root.ExecuteContext(ctx) - require.ErrorIs(t, err, context.Canceled) - - // Verify that credentials were output to the terminal. - assert.Contains(t, buf.String(), fmt.Sprintf("email: %s", wantEmail), "expected output %q; got no match", wantEmail) - // Check that the password line is output and that it's non-empty. - if _, after, found := strings.Cut(buf.String(), "password: "); found { - before, _, _ := strings.Cut(after, "\n") - before = strings.Trim(before, "\r") // Ensure no control character is left. - assert.NotEmpty(t, before, "expected non-empty password; got empty") - } else { - t.Error("expected password line output; got no match") - } + errC <- root.ExecuteContext(ctx) }() + var token string require.Eventually(t, func() bool { var err error @@ -119,8 +104,20 @@ func TestServer(t *testing.T) { require.NoError(t, err) cancelFunc() - wg.Wait() + require.ErrorIs(t, <-errC, context.Canceled) + + // Verify that credentials were output to the terminal. + assert.Contains(t, buf.String(), fmt.Sprintf("email: %s", wantEmail), "expected output %q; got no match", wantEmail) + // Check that the password line is output and that it's non-empty. + if _, after, found := strings.Cut(buf.String(), "password: "); found { + before, _, _ := strings.Cut(after, "\n") + before = strings.Trim(before, "\r") // Ensure no control character is left. + assert.NotEmpty(t, before, "expected non-empty password; got empty") + } else { + t.Error("expected password line output; got no match") + } }) + // Duplicated test from "Development" above to test setting email/password via env. // Cannot run parallel due to os.Setenv. //nolint:paralleltest @@ -136,18 +133,11 @@ func TestServer(t *testing.T) { root, cfg := clitest.New(t, "server", "--dev", "--tunnel=false", "--address", ":0") var buf strings.Builder root.SetOutput(&buf) - var wg sync.WaitGroup - wg.Add(1) + errC := make(chan error) go func() { - defer wg.Done() - - err := root.ExecuteContext(ctx) - require.ErrorIs(t, err, context.Canceled) - - // Verify that credentials were output to the terminal. - assert.Contains(t, buf.String(), fmt.Sprintf("email: %s", wantEmail), "expected output %q; got no match", wantEmail) - assert.Contains(t, buf.String(), fmt.Sprintf("password: %s", wantPassword), "expected output %q; got no match", wantPassword) + errC <- root.ExecuteContext(ctx) }() + var token string require.Eventually(t, func() bool { var err error @@ -165,8 +155,12 @@ func TestServer(t *testing.T) { require.NoError(t, err) cancelFunc() - wg.Wait() + require.ErrorIs(t, <-errC, context.Canceled) + // Verify that credentials were output to the terminal. + assert.Contains(t, buf.String(), fmt.Sprintf("email: %s", wantEmail), "expected output %q; got no match", wantEmail) + assert.Contains(t, buf.String(), fmt.Sprintf("password: %s", wantPassword), "expected output %q; got no match", wantPassword) }) + t.Run("TLSBadVersion", func(t *testing.T) { t.Parallel() ctx, cancelFunc := context.WithCancel(context.Background()) @@ -202,10 +196,12 @@ func TestServer(t *testing.T) { certPath, keyPath := generateTLSCertificate(t) root, cfg := clitest.New(t, "server", "--dev", "--tunnel=false", "--address", ":0", "--tls-enable", "--tls-cert-file", certPath, "--tls-key-file", keyPath) + errC := make(chan error) go func() { - err := root.ExecuteContext(ctx) - require.ErrorIs(t, err, context.Canceled) + errC <- root.ExecuteContext(ctx) }() + + // Verify HTTPS var accessURLRaw string require.Eventually(t, func() bool { var err error @@ -226,6 +222,9 @@ func TestServer(t *testing.T) { } _, err = client.HasFirstUser(ctx) require.NoError(t, err) + + cancelFunc() + require.ErrorIs(t, <-errC, context.Canceled) }) // This cannot be ran in parallel because it uses a signal. //nolint:paralleltest @@ -284,14 +283,12 @@ func TestServer(t *testing.T) { ctx, cancelFunc := context.WithCancel(context.Background()) defer cancelFunc() root, _ := clitest.New(t, "server", "--dev", "--tunnel=false", "--address", ":0", "--trace=true") - done := make(chan struct{}) + errC := make(chan error) go func() { - defer close(done) - err := root.ExecuteContext(ctx) - require.ErrorIs(t, err, context.Canceled) + errC <- root.ExecuteContext(ctx) }() cancelFunc() - <-done + require.ErrorIs(t, <-errC, context.Canceled) require.Error(t, goleak.Find()) }) } From 4ea0ed787567d71c6d34494c14feb01b433f9cec Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 23 May 2022 14:23:14 +0100 Subject: [PATCH 05/10] fix: cli: clitest_test: avoid calling t.FailNow outside main goroutine --- cli/clitest/clitest_test.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/cli/clitest/clitest_test.go b/cli/clitest/clitest_test.go index a71c220783fe9..b32cfe46559ea 100644 --- a/cli/clitest/clitest_test.go +++ b/cli/clitest/clitest_test.go @@ -1,7 +1,9 @@ package clitest_test import ( + "context" "testing" + "time" "github.com/stretchr/testify/require" "go.uber.org/goleak" @@ -17,6 +19,8 @@ func TestMain(m *testing.M) { func TestCli(t *testing.T) { t.Parallel() + ctx, cancelFunc := context.WithTimeout(context.Background(), 5*time.Second) + defer cancelFunc() clitest.CreateTemplateVersionSource(t, nil) client := coderdtest.New(t, nil) cmd, config := clitest.New(t) @@ -24,9 +28,11 @@ func TestCli(t *testing.T) { pty := ptytest.New(t) cmd.SetIn(pty.Input()) cmd.SetOut(pty.Output()) + errC := make(chan error) go func() { - err := cmd.Execute() - require.NoError(t, err) + errC <- cmd.ExecuteContext(ctx) }() pty.ExpectMatch("coder") + cancelFunc() + require.NoError(t, <-errC) } From 3998794b245071724c9805583bc3ab19a8bd6f5c Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 23 May 2022 14:32:51 +0100 Subject: [PATCH 06/10] fix: cli: list_test: avoid calling t.FailNow outside main goroutine --- cli/list_test.go | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/cli/list_test.go b/cli/list_test.go index 76408ea3f1f33..994b364dd7489 100644 --- a/cli/list_test.go +++ b/cli/list_test.go @@ -1,7 +1,9 @@ package cli_test import ( + "context" "testing" + "time" "github.com/stretchr/testify/require" @@ -14,6 +16,8 @@ func TestList(t *testing.T) { t.Parallel() t.Run("Single", func(t *testing.T) { t.Parallel() + ctx, cancelFunc := context.WithTimeout(context.Background(), 5*time.Second) + defer cancelFunc() client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerD: true}) user := coderdtest.CreateFirstUser(t, client) version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) @@ -23,17 +27,16 @@ func TestList(t *testing.T) { coderdtest.AwaitWorkspaceBuildJob(t, client, workspace.LatestBuild.ID) cmd, root := clitest.New(t, "ls") clitest.SetupConfig(t, client, root) - doneChan := make(chan struct{}) pty := ptytest.New(t) cmd.SetIn(pty.Input()) cmd.SetOut(pty.Output()) + errC := make(chan error) go func() { - defer close(doneChan) - err := cmd.Execute() - require.NoError(t, err) + errC <- cmd.ExecuteContext(ctx) }() pty.ExpectMatch(workspace.Name) pty.ExpectMatch("Running") - <-doneChan + cancelFunc() + require.NoError(t, <-errC) }) } From 6519ff8b5a5a5f24dc3db1daf9158ced57f054f6 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 23 May 2022 17:47:11 +0300 Subject: [PATCH 07/10] Fix TestGitSSH t-after-exit --- cli/gitssh_test.go | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/cli/gitssh_test.go b/cli/gitssh_test.go index 3023afe3b8af1..0fc2bde8ffadf 100644 --- a/cli/gitssh_test.go +++ b/cli/gitssh_test.go @@ -63,9 +63,9 @@ func TestGitSSH(t *testing.T) { clitest.SetupConfig(t, agentClient, root) ctx, cancelFunc := context.WithCancel(context.Background()) defer cancelFunc() - errC := make(chan error) + agentErrC := make(chan error) go func() { - errC <- cmd.ExecuteContext(ctx) + agentErrC <- cmd.ExecuteContext(ctx) }() coderdtest.AwaitWorkspaceAgents(t, client, workspace.LatestBuild.ID) @@ -85,13 +85,13 @@ func TestGitSSH(t *testing.T) { return ssh.KeysEqual(publicKey, key) }) var inc int64 + sshErrC := make(chan error) go func() { // as long as we get a successful session we don't care if the server errors _ = ssh.Serve(l, func(s ssh.Session) { atomic.AddInt64(&inc, 1) t.Log("got authenticated session") - err := s.Exit(0) - require.NoError(t, err) + sshErrC <- s.Exit(0) }, publicKeyOption) }() @@ -99,13 +99,16 @@ func TestGitSSH(t *testing.T) { addr, ok := l.Addr().(*net.TCPAddr) require.True(t, ok) // set to agent config dir - cmd, _ = clitest.New(t, "gitssh", "--agent-url", agentClient.URL.String(), "--agent-token", agentToken, "--", fmt.Sprintf("-p%d", addr.Port), "-o", "StrictHostKeyChecking=no", "127.0.0.1") - err = cmd.ExecuteContext(context.Background()) + gitsshCmd, _ := clitest.New(t, "gitssh", "--agent-url", agentClient.URL.String(), "--agent-token", agentToken, "--", fmt.Sprintf("-p%d", addr.Port), "-o", "StrictHostKeyChecking=no", "127.0.0.1") + err = gitsshCmd.ExecuteContext(context.Background()) require.NoError(t, err) require.EqualValues(t, 1, inc) + err = <-sshErrC + require.NoError(t, err, "error in ssh session exit") + cancelFunc() - err = <-errC - require.NoError(t, err) + err = <-agentErrC + require.NoError(t, err, "error in agent execute") }) } From 5c741f4d9116c33feec844cffadc203d203dd70a Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 23 May 2022 17:47:45 +0300 Subject: [PATCH 08/10] Fix TestGitSSH "too many authentication failures" Due to local SSH keys being given --- cli/gitssh_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/gitssh_test.go b/cli/gitssh_test.go index 0fc2bde8ffadf..999c6ce7314df 100644 --- a/cli/gitssh_test.go +++ b/cli/gitssh_test.go @@ -99,7 +99,7 @@ func TestGitSSH(t *testing.T) { addr, ok := l.Addr().(*net.TCPAddr) require.True(t, ok) // set to agent config dir - gitsshCmd, _ := clitest.New(t, "gitssh", "--agent-url", agentClient.URL.String(), "--agent-token", agentToken, "--", fmt.Sprintf("-p%d", addr.Port), "-o", "StrictHostKeyChecking=no", "127.0.0.1") + gitsshCmd, _ := clitest.New(t, "gitssh", "--agent-url", agentClient.URL.String(), "--agent-token", agentToken, "--", fmt.Sprintf("-p%d", addr.Port), "-o", "StrictHostKeyChecking=no", "-o", "IdentitiesOnly=yes", "127.0.0.1") err = gitsshCmd.ExecuteContext(context.Background()) require.NoError(t, err) require.EqualValues(t, 1, inc) From de6b5ed561659f0a194d8f28bad0437e4660acfe Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Mon, 23 May 2022 16:15:46 +0100 Subject: [PATCH 09/10] chore: clitest: fix TestCli --- cli/clitest/clitest_test.go | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/cli/clitest/clitest_test.go b/cli/clitest/clitest_test.go index b32cfe46559ea..441b84048d05e 100644 --- a/cli/clitest/clitest_test.go +++ b/cli/clitest/clitest_test.go @@ -1,11 +1,8 @@ package clitest_test import ( - "context" "testing" - "time" - "github.com/stretchr/testify/require" "go.uber.org/goleak" "github.com/coder/coder/cli/clitest" @@ -19,8 +16,6 @@ func TestMain(m *testing.M) { func TestCli(t *testing.T) { t.Parallel() - ctx, cancelFunc := context.WithTimeout(context.Background(), 5*time.Second) - defer cancelFunc() clitest.CreateTemplateVersionSource(t, nil) client := coderdtest.New(t, nil) cmd, config := clitest.New(t) @@ -28,11 +23,8 @@ func TestCli(t *testing.T) { pty := ptytest.New(t) cmd.SetIn(pty.Input()) cmd.SetOut(pty.Output()) - errC := make(chan error) go func() { - errC <- cmd.ExecuteContext(ctx) + _ = cmd.Execute() }() pty.ExpectMatch("coder") - cancelFunc() - require.NoError(t, <-errC) } From ed95609a2575166d279c51622deee2e0e70a4aa3 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Mon, 23 May 2022 18:27:40 +0300 Subject: [PATCH 10/10] chore: Simplify TestTemplateInit --- cli/templateinit_test.go | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/cli/templateinit_test.go b/cli/templateinit_test.go index a5fada4310906..46969ea7c287a 100644 --- a/cli/templateinit_test.go +++ b/cli/templateinit_test.go @@ -16,16 +16,11 @@ func TestTemplateInit(t *testing.T) { t.Parallel() tempDir := t.TempDir() cmd, _ := clitest.New(t, "templates", "init", tempDir) - doneChan := make(chan struct{}) pty := ptytest.New(t) cmd.SetIn(pty.Input()) cmd.SetOut(pty.Output()) - go func() { - defer close(doneChan) - err := cmd.Execute() - require.NoError(t, err) - }() - <-doneChan + err := cmd.Execute() + require.NoError(t, err) files, err := os.ReadDir(tempDir) require.NoError(t, err) require.Greater(t, len(files), 0)