Skip to content

Commit cb337a2

Browse files
mafredrijohnstcn
authored and
kylecarbs
committed
fix: Guard against CLI cmd running after test exit (#1658)
* fix: Guard against CLI cmd running after test exit * fix: cli: avoid calling t.FailNow in non-test-main goroutine * fix: cli: server_test: avoid calling t.FailNow outside main goroutine * fix: cli: clitest_test: avoid calling t.FailNow outside main goroutine * fix: cli: list_test: avoid calling t.FailNow outside main goroutine * fix: TestGitSSH use-of-t-after-exit * fix: TestGitSSH "too many authentication failures" Due to local SSH keys being given * chore: clitest: fix TestCli * chore: Simplify TestTemplateInit Co-authored-by: Cian Johnston <cian@coder.com>
1 parent 555a98f commit cb337a2

7 files changed

+87
-86
lines changed

‎cli/agent_test.go

+21-18
Original file line numberDiff line numberDiff line change
@@ -49,13 +49,12 @@ func TestWorkspaceAgent(t *testing.T) {
4949
cmd, _ := clitest.New(t, "agent", "--auth", "azure-instance-identity", "--agent-url", client.URL.String())
5050
ctx, cancelFunc := context.WithCancel(context.Background())
5151
defer cancelFunc()
52+
errC := make(chan error)
5253
go func() {
53-
// A linting error occurs for weakly typing the context value here,
54-
// but it seems reasonable for a one-off test.
55-
// nolint
56-
ctx = context.WithValue(ctx, "azure-client", metadataClient)
57-
err := cmd.ExecuteContext(ctx)
58-
require.NoError(t, err)
54+
// A linting error occurs for weakly typing the context value here.
55+
//nolint // The above seems reasonable for a one-off test.
56+
ctx := context.WithValue(ctx, "azure-client", metadataClient)
57+
errC <- cmd.ExecuteContext(ctx)
5958
}()
6059
coderdtest.AwaitWorkspaceAgents(t, client, workspace.LatestBuild.ID)
6160
resources, err := client.WorkspaceResourcesByBuild(ctx, workspace.LatestBuild.ID)
@@ -66,6 +65,8 @@ func TestWorkspaceAgent(t *testing.T) {
6665
_, err = dialer.Ping()
6766
require.NoError(t, err)
6867
cancelFunc()
68+
err = <-errC
69+
require.NoError(t, err)
6970
})
7071

7172
t.Run("AWS", func(t *testing.T) {
@@ -103,13 +104,12 @@ func TestWorkspaceAgent(t *testing.T) {
103104
cmd, _ := clitest.New(t, "agent", "--auth", "aws-instance-identity", "--agent-url", client.URL.String())
104105
ctx, cancelFunc := context.WithCancel(context.Background())
105106
defer cancelFunc()
107+
errC := make(chan error)
106108
go func() {
107-
// A linting error occurs for weakly typing the context value here,
108-
// but it seems reasonable for a one-off test.
109-
// nolint
110-
ctx = context.WithValue(ctx, "aws-client", metadataClient)
111-
err := cmd.ExecuteContext(ctx)
112-
require.NoError(t, err)
109+
// A linting error occurs for weakly typing the context value here.
110+
//nolint // The above seems reasonable for a one-off test.
111+
ctx := context.WithValue(ctx, "aws-client", metadataClient)
112+
errC <- cmd.ExecuteContext(ctx)
113113
}()
114114
coderdtest.AwaitWorkspaceAgents(t, client, workspace.LatestBuild.ID)
115115
resources, err := client.WorkspaceResourcesByBuild(ctx, workspace.LatestBuild.ID)
@@ -120,6 +120,8 @@ func TestWorkspaceAgent(t *testing.T) {
120120
_, err = dialer.Ping()
121121
require.NoError(t, err)
122122
cancelFunc()
123+
err = <-errC
124+
require.NoError(t, err)
123125
})
124126

125127
t.Run("GoogleCloud", func(t *testing.T) {
@@ -157,13 +159,12 @@ func TestWorkspaceAgent(t *testing.T) {
157159
cmd, _ := clitest.New(t, "agent", "--auth", "google-instance-identity", "--agent-url", client.URL.String())
158160
ctx, cancelFunc := context.WithCancel(context.Background())
159161
defer cancelFunc()
162+
errC := make(chan error)
160163
go func() {
161-
// A linting error occurs for weakly typing the context value here,
162-
// but it seems reasonable for a one-off test.
163-
// nolint
164-
ctx = context.WithValue(ctx, "gcp-client", metadata)
165-
err := cmd.ExecuteContext(ctx)
166-
require.NoError(t, err)
164+
// A linting error occurs for weakly typing the context value here.
165+
//nolint // The above seems reasonable for a one-off test.
166+
ctx := context.WithValue(ctx, "gcp-client", metadata)
167+
errC <- cmd.ExecuteContext(ctx)
167168
}()
168169
coderdtest.AwaitWorkspaceAgents(t, client, workspace.LatestBuild.ID)
169170
resources, err := client.WorkspaceResourcesByBuild(ctx, workspace.LatestBuild.ID)
@@ -174,5 +175,7 @@ func TestWorkspaceAgent(t *testing.T) {
174175
_, err = dialer.Ping()
175176
require.NoError(t, err)
176177
cancelFunc()
178+
err = <-errC
179+
require.NoError(t, err)
177180
})
178181
}

‎cli/clitest/clitest_test.go

+1-3
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ package clitest_test
33
import (
44
"testing"
55

6-
"github.com/stretchr/testify/require"
76
"go.uber.org/goleak"
87

98
"github.com/coder/coder/cli/clitest"
@@ -25,8 +24,7 @@ func TestCli(t *testing.T) {
2524
cmd.SetIn(pty.Input())
2625
cmd.SetOut(pty.Output())
2726
go func() {
28-
err := cmd.Execute()
29-
require.NoError(t, err)
27+
_ = cmd.Execute()
3028
}()
3129
pty.ExpectMatch("coder")
3230
}

‎cli/gitssh_test.go

+13-6
Original file line numberDiff line numberDiff line change
@@ -63,9 +63,9 @@ func TestGitSSH(t *testing.T) {
6363
clitest.SetupConfig(t, agentClient, root)
6464
ctx, cancelFunc := context.WithCancel(context.Background())
6565
defer cancelFunc()
66+
agentErrC := make(chan error)
6667
go func() {
67-
err := cmd.ExecuteContext(ctx)
68-
require.NoError(t, err)
68+
agentErrC <- cmd.ExecuteContext(ctx)
6969
}()
7070

7171
coderdtest.AwaitWorkspaceAgents(t, client, workspace.LatestBuild.ID)
@@ -85,23 +85,30 @@ func TestGitSSH(t *testing.T) {
8585
return ssh.KeysEqual(publicKey, key)
8686
})
8787
var inc int64
88+
sshErrC := make(chan error)
8889
go func() {
8990
// as long as we get a successful session we don't care if the server errors
9091
_ = ssh.Serve(l, func(s ssh.Session) {
9192
atomic.AddInt64(&inc, 1)
9293
t.Log("got authenticated session")
93-
err := s.Exit(0)
94-
require.NoError(t, err)
94+
sshErrC <- s.Exit(0)
9595
}, publicKeyOption)
9696
}()
9797

9898
// start ssh session
9999
addr, ok := l.Addr().(*net.TCPAddr)
100100
require.True(t, ok)
101101
// set to agent config dir
102-
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")
103-
err = cmd.ExecuteContext(context.Background())
102+
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")
103+
err = gitsshCmd.ExecuteContext(context.Background())
104104
require.NoError(t, err)
105105
require.EqualValues(t, 1, inc)
106+
107+
err = <-sshErrC
108+
require.NoError(t, err, "error in ssh session exit")
109+
110+
cancelFunc()
111+
err = <-agentErrC
112+
require.NoError(t, err, "error in agent execute")
106113
})
107114
}

‎cli/list_test.go

+8-5
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
package cli_test
22

33
import (
4+
"context"
45
"testing"
6+
"time"
57

68
"github.com/stretchr/testify/require"
79

@@ -14,6 +16,8 @@ func TestList(t *testing.T) {
1416
t.Parallel()
1517
t.Run("Single", func(t *testing.T) {
1618
t.Parallel()
19+
ctx, cancelFunc := context.WithTimeout(context.Background(), 5*time.Second)
20+
defer cancelFunc()
1721
client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerD: true})
1822
user := coderdtest.CreateFirstUser(t, client)
1923
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
@@ -23,17 +27,16 @@ func TestList(t *testing.T) {
2327
coderdtest.AwaitWorkspaceBuildJob(t, client, workspace.LatestBuild.ID)
2428
cmd, root := clitest.New(t, "ls")
2529
clitest.SetupConfig(t, client, root)
26-
doneChan := make(chan struct{})
2730
pty := ptytest.New(t)
2831
cmd.SetIn(pty.Input())
2932
cmd.SetOut(pty.Output())
33+
errC := make(chan error)
3034
go func() {
31-
defer close(doneChan)
32-
err := cmd.Execute()
33-
require.NoError(t, err)
35+
errC <- cmd.ExecuteContext(ctx)
3436
}()
3537
pty.ExpectMatch(workspace.Name)
3638
pty.ExpectMatch("Running")
37-
<-doneChan
39+
cancelFunc()
40+
require.NoError(t, <-errC)
3841
})
3942
}

‎cli/server_test.go

+39-42
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ import (
1717
"os"
1818
"runtime"
1919
"strings"
20-
"sync"
2120
"testing"
2221
"time"
2322

@@ -45,12 +44,11 @@ func TestServer(t *testing.T) {
4544
require.NoError(t, err)
4645
defer closeFunc()
4746
ctx, cancelFunc := context.WithCancel(context.Background())
48-
done := make(chan struct{})
47+
defer cancelFunc()
4948
root, cfg := clitest.New(t, "server", "--address", ":0", "--postgres-url", connectionURL)
49+
errC := make(chan error)
5050
go func() {
51-
defer close(done)
52-
err = root.ExecuteContext(ctx)
53-
require.ErrorIs(t, err, context.Canceled)
51+
errC <- root.ExecuteContext(ctx)
5452
}()
5553
var client *codersdk.Client
5654
require.Eventually(t, func() bool {
@@ -71,8 +69,9 @@ func TestServer(t *testing.T) {
7169
})
7270
require.NoError(t, err)
7371
cancelFunc()
74-
<-done
72+
require.ErrorIs(t, <-errC, context.Canceled)
7573
})
74+
7675
t.Run("Development", func(t *testing.T) {
7776
t.Parallel()
7877
ctx, cancelFunc := context.WithCancel(context.Background())
@@ -82,26 +81,12 @@ func TestServer(t *testing.T) {
8281

8382
root, cfg := clitest.New(t, "server", "--dev", "--tunnel=false", "--address", ":0")
8483
var buf strings.Builder
84+
errC := make(chan error)
8585
root.SetOutput(&buf)
86-
var wg sync.WaitGroup
87-
wg.Add(1)
8886
go func() {
89-
defer wg.Done()
90-
91-
err := root.ExecuteContext(ctx)
92-
require.ErrorIs(t, err, context.Canceled)
93-
94-
// Verify that credentials were output to the terminal.
95-
assert.Contains(t, buf.String(), fmt.Sprintf("email: %s", wantEmail), "expected output %q; got no match", wantEmail)
96-
// Check that the password line is output and that it's non-empty.
97-
if _, after, found := strings.Cut(buf.String(), "password: "); found {
98-
before, _, _ := strings.Cut(after, "\n")
99-
before = strings.Trim(before, "\r") // Ensure no control character is left.
100-
assert.NotEmpty(t, before, "expected non-empty password; got empty")
101-
} else {
102-
t.Error("expected password line output; got no match")
103-
}
87+
errC <- root.ExecuteContext(ctx)
10488
}()
89+
10590
var token string
10691
require.Eventually(t, func() bool {
10792
var err error
@@ -119,8 +104,20 @@ func TestServer(t *testing.T) {
119104
require.NoError(t, err)
120105

121106
cancelFunc()
122-
wg.Wait()
107+
require.ErrorIs(t, <-errC, context.Canceled)
108+
109+
// Verify that credentials were output to the terminal.
110+
assert.Contains(t, buf.String(), fmt.Sprintf("email: %s", wantEmail), "expected output %q; got no match", wantEmail)
111+
// Check that the password line is output and that it's non-empty.
112+
if _, after, found := strings.Cut(buf.String(), "password: "); found {
113+
before, _, _ := strings.Cut(after, "\n")
114+
before = strings.Trim(before, "\r") // Ensure no control character is left.
115+
assert.NotEmpty(t, before, "expected non-empty password; got empty")
116+
} else {
117+
t.Error("expected password line output; got no match")
118+
}
123119
})
120+
124121
// Duplicated test from "Development" above to test setting email/password via env.
125122
// Cannot run parallel due to os.Setenv.
126123
//nolint:paralleltest
@@ -136,18 +133,11 @@ func TestServer(t *testing.T) {
136133
root, cfg := clitest.New(t, "server", "--dev", "--tunnel=false", "--address", ":0")
137134
var buf strings.Builder
138135
root.SetOutput(&buf)
139-
var wg sync.WaitGroup
140-
wg.Add(1)
136+
errC := make(chan error)
141137
go func() {
142-
defer wg.Done()
143-
144-
err := root.ExecuteContext(ctx)
145-
require.ErrorIs(t, err, context.Canceled)
146-
147-
// Verify that credentials were output to the terminal.
148-
assert.Contains(t, buf.String(), fmt.Sprintf("email: %s", wantEmail), "expected output %q; got no match", wantEmail)
149-
assert.Contains(t, buf.String(), fmt.Sprintf("password: %s", wantPassword), "expected output %q; got no match", wantPassword)
138+
errC <- root.ExecuteContext(ctx)
150139
}()
140+
151141
var token string
152142
require.Eventually(t, func() bool {
153143
var err error
@@ -165,8 +155,12 @@ func TestServer(t *testing.T) {
165155
require.NoError(t, err)
166156

167157
cancelFunc()
168-
wg.Wait()
158+
require.ErrorIs(t, <-errC, context.Canceled)
159+
// Verify that credentials were output to the terminal.
160+
assert.Contains(t, buf.String(), fmt.Sprintf("email: %s", wantEmail), "expected output %q; got no match", wantEmail)
161+
assert.Contains(t, buf.String(), fmt.Sprintf("password: %s", wantPassword), "expected output %q; got no match", wantPassword)
169162
})
163+
170164
t.Run("TLSBadVersion", func(t *testing.T) {
171165
t.Parallel()
172166
ctx, cancelFunc := context.WithCancel(context.Background())
@@ -202,10 +196,12 @@ func TestServer(t *testing.T) {
202196
certPath, keyPath := generateTLSCertificate(t)
203197
root, cfg := clitest.New(t, "server", "--dev", "--tunnel=false", "--address", ":0",
204198
"--tls-enable", "--tls-cert-file", certPath, "--tls-key-file", keyPath)
199+
errC := make(chan error)
205200
go func() {
206-
err := root.ExecuteContext(ctx)
207-
require.ErrorIs(t, err, context.Canceled)
201+
errC <- root.ExecuteContext(ctx)
208202
}()
203+
204+
// Verify HTTPS
209205
var accessURLRaw string
210206
require.Eventually(t, func() bool {
211207
var err error
@@ -226,6 +222,9 @@ func TestServer(t *testing.T) {
226222
}
227223
_, err = client.HasFirstUser(ctx)
228224
require.NoError(t, err)
225+
226+
cancelFunc()
227+
require.ErrorIs(t, <-errC, context.Canceled)
229228
})
230229
// This cannot be ran in parallel because it uses a signal.
231230
//nolint:paralleltest
@@ -284,14 +283,12 @@ func TestServer(t *testing.T) {
284283
ctx, cancelFunc := context.WithCancel(context.Background())
285284
defer cancelFunc()
286285
root, _ := clitest.New(t, "server", "--dev", "--tunnel=false", "--address", ":0", "--trace=true")
287-
done := make(chan struct{})
286+
errC := make(chan error)
288287
go func() {
289-
defer close(done)
290-
err := root.ExecuteContext(ctx)
291-
require.ErrorIs(t, err, context.Canceled)
288+
errC <- root.ExecuteContext(ctx)
292289
}()
293290
cancelFunc()
294-
<-done
291+
require.ErrorIs(t, <-errC, context.Canceled)
295292
require.Error(t, goleak.Find())
296293
})
297294
}

‎cli/templateinit_test.go

+2-7
Original file line numberDiff line numberDiff line change
@@ -16,16 +16,11 @@ func TestTemplateInit(t *testing.T) {
1616
t.Parallel()
1717
tempDir := t.TempDir()
1818
cmd, _ := clitest.New(t, "templates", "init", tempDir)
19-
doneChan := make(chan struct{})
2019
pty := ptytest.New(t)
2120
cmd.SetIn(pty.Input())
2221
cmd.SetOut(pty.Output())
23-
go func() {
24-
defer close(doneChan)
25-
err := cmd.Execute()
26-
require.NoError(t, err)
27-
}()
28-
<-doneChan
22+
err := cmd.Execute()
23+
require.NoError(t, err)
2924
files, err := os.ReadDir(tempDir)
3025
require.NoError(t, err)
3126
require.Greater(t, len(files), 0)

‎cli/userlist_test.go

+3-5
Original file line numberDiff line numberDiff line change
@@ -16,15 +16,13 @@ func TestUserList(t *testing.T) {
1616
coderdtest.CreateFirstUser(t, client)
1717
cmd, root := clitest.New(t, "users", "list")
1818
clitest.SetupConfig(t, client, root)
19-
doneChan := make(chan struct{})
2019
pty := ptytest.New(t)
2120
cmd.SetIn(pty.Input())
2221
cmd.SetOut(pty.Output())
22+
errC := make(chan error)
2323
go func() {
24-
defer close(doneChan)
25-
err := cmd.Execute()
26-
require.NoError(t, err)
24+
errC <- cmd.Execute()
2725
}()
26+
require.NoError(t, <-errC)
2827
pty.ExpectMatch("coder.com")
29-
<-doneChan
3028
}

0 commit comments

Comments
 (0)