Skip to content

Commit 94eb9b8

Browse files
spikecurtisspikecurtis
authored
fix: disable t.Parallel on TestPortForward (#10449)
I've said it before, I'll say it again: you can't create a timed context before calling `t.Parallel()` and then use it after. Fixes flakes like https://github.com/coder/coder/actions/runs/6716682414/job/18253279157 I've chosen just to drop `t.Parallel()` entirely rather than create a second context after the parallel call, since the vast majority of the test time happens before where the parallel call was. It does all the tailnet setup before `t.Parallel()`. Leaving a call to `t.Parallel()` is a bug risk for future maintainers to come in and use the wrong context in the latter part of the test by accident.
1 parent 6882e8e commit 94eb9b8

File tree

3 files changed

+28
-10
lines changed

3 files changed

+28
-10
lines changed

‎cli/portforward.go

+16-3
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ func (r *RootCmd) portForward() *clibase.Cmd {
9898
return xerrors.Errorf("await agent: %w", err)
9999
}
100100

101-
var logger slog.Logger
101+
logger := slog.Make()
102102
if r.verbose {
103103
logger = slog.Make(sloghuman.Sink(inv.Stdout)).Leveled(slog.LevelDebug)
104104
}
@@ -131,7 +131,7 @@ func (r *RootCmd) portForward() *clibase.Cmd {
131131
defer closeAllListeners()
132132

133133
for i, spec := range specs {
134-
l, err := listenAndPortForward(ctx, inv, conn, wg, spec)
134+
l, err := listenAndPortForward(ctx, inv, conn, wg, spec, logger)
135135
if err != nil {
136136
return err
137137
}
@@ -185,7 +185,15 @@ func (r *RootCmd) portForward() *clibase.Cmd {
185185
return cmd
186186
}
187187

188-
func listenAndPortForward(ctx context.Context, inv *clibase.Invocation, conn *codersdk.WorkspaceAgentConn, wg *sync.WaitGroup, spec portForwardSpec) (net.Listener, error) {
188+
func listenAndPortForward(
189+
ctx context.Context,
190+
inv *clibase.Invocation,
191+
conn *codersdk.WorkspaceAgentConn,
192+
wg *sync.WaitGroup,
193+
spec portForwardSpec,
194+
logger slog.Logger,
195+
) (net.Listener, error) {
196+
logger = logger.With(slog.F("network", spec.listenNetwork), slog.F("address", spec.listenAddress))
189197
_, _ = fmt.Fprintf(inv.Stderr, "Forwarding '%v://%v' locally to '%v://%v' in the workspace\n", spec.listenNetwork, spec.listenAddress, spec.dialNetwork, spec.dialAddress)
190198

191199
var (
@@ -218,6 +226,7 @@ func listenAndPortForward(ctx context.Context, inv *clibase.Invocation, conn *co
218226
if err != nil {
219227
return nil, xerrors.Errorf("listen '%v://%v': %w", spec.listenNetwork, spec.listenAddress, err)
220228
}
229+
logger.Debug(ctx, "listening")
221230

222231
wg.Add(1)
223232
go func(spec portForwardSpec) {
@@ -227,12 +236,14 @@ func listenAndPortForward(ctx context.Context, inv *clibase.Invocation, conn *co
227236
if err != nil {
228237
// Silently ignore net.ErrClosed errors.
229238
if xerrors.Is(err, net.ErrClosed) {
239+
logger.Debug(ctx, "listener closed")
230240
return
231241
}
232242
_, _ = fmt.Fprintf(inv.Stderr, "Error accepting connection from '%v://%v': %v\n", spec.listenNetwork, spec.listenAddress, err)
233243
_, _ = fmt.Fprintln(inv.Stderr, "Killing listener")
234244
return
235245
}
246+
logger.Debug(ctx, "accepted connection", slog.F("remote_addr", netConn.RemoteAddr()))
236247

237248
go func(netConn net.Conn) {
238249
defer netConn.Close()
@@ -242,8 +253,10 @@ func listenAndPortForward(ctx context.Context, inv *clibase.Invocation, conn *co
242253
return
243254
}
244255
defer remoteConn.Close()
256+
logger.Debug(ctx, "dialed remote", slog.F("remote_addr", netConn.RemoteAddr()))
245257

246258
agentssh.Bicopy(ctx, netConn, remoteConn)
259+
logger.Debug(ctx, "connection closing", slog.F("remote_addr", netConn.RemoteAddr()))
247260
}(netConn)
248261
}
249262
}(spec)

‎cli/portforward_test.go

+10-7
Original file line numberDiff line numberDiff line change
@@ -140,9 +140,10 @@ func TestPortForward(t *testing.T) {
140140

141141
for _, c := range cases {
142142
c := c
143-
// Delay parallel tests here because setupLocal reserves
143+
// No parallel tests here because setupLocal reserves
144144
// a free open port which is not guaranteed to be free
145145
// between the listener closing and port-forward ready.
146+
//nolint:tparallel,paralleltest
146147
t.Run(c.name+"_OnePort", func(t *testing.T) {
147148
p1 := setupTestListener(t, c.setupRemote(t))
148149

@@ -166,8 +167,6 @@ func TestPortForward(t *testing.T) {
166167
}()
167168
pty.ExpectMatchContext(ctx, "Ready!")
168169

169-
t.Parallel() // Port is reserved, enable parallel execution.
170-
171170
// Open two connections simultaneously and test them out of
172171
// sync.
173172
d := net.Dialer{Timeout: testutil.WaitShort}
@@ -185,6 +184,10 @@ func TestPortForward(t *testing.T) {
185184
require.ErrorIs(t, err, context.Canceled)
186185
})
187186

187+
// No parallel tests here because setupLocal reserves
188+
// a free open port which is not guaranteed to be free
189+
// between the listener closing and port-forward ready.
190+
//nolint:tparallel,paralleltest
188191
t.Run(c.name+"_TwoPorts", func(t *testing.T) {
189192
var (
190193
p1 = setupTestListener(t, c.setupRemote(t))
@@ -213,8 +216,6 @@ func TestPortForward(t *testing.T) {
213216
}()
214217
pty.ExpectMatchContext(ctx, "Ready!")
215218

216-
t.Parallel() // Port is reserved, enable parallel execution.
217-
218219
// Open a connection to both listener 1 and 2 simultaneously and
219220
// then test them out of order.
220221
d := net.Dialer{Timeout: testutil.WaitShort}
@@ -234,6 +235,10 @@ func TestPortForward(t *testing.T) {
234235
}
235236

236237
// Test doing TCP and UDP at the same time.
238+
// No parallel tests here because setupLocal reserves
239+
// a free open port which is not guaranteed to be free
240+
// between the listener closing and port-forward ready.
241+
//nolint:tparallel,paralleltest
237242
t.Run("All", func(t *testing.T) {
238243
var (
239244
dials = []addr{}
@@ -266,8 +271,6 @@ func TestPortForward(t *testing.T) {
266271
}()
267272
pty.ExpectMatchContext(ctx, "Ready!")
268273

269-
t.Parallel() // Port is reserved, enable parallel execution.
270-
271274
// Open connections to all items in the "dial" array.
272275
var (
273276
d = net.Dialer{Timeout: testutil.WaitShort}

‎tailnet/conn.go

+2
Original file line numberDiff line numberDiff line change
@@ -936,10 +936,12 @@ func (c *Conn) Listen(network, addr string) (net.Listener, error) {
936936
}
937937

938938
func (c *Conn) DialContextTCP(ctx context.Context, ipp netip.AddrPort) (*gonet.TCPConn, error) {
939+
c.logger.Debug(ctx, "dial tcp", slog.F("addr_port", ipp))
939940
return c.netStack.DialContextTCP(ctx, ipp)
940941
}
941942

942943
func (c *Conn) DialContextUDP(ctx context.Context, ipp netip.AddrPort) (*gonet.UDPConn, error) {
944+
c.logger.Debug(ctx, "dial udp", slog.F("addr_port", ipp))
943945
return c.netStack.DialContextUDP(ctx, ipp)
944946
}
945947

0 commit comments

Comments
 (0)