Skip to content

Commit 9b67a9c

Browse files
deansheatherdeansheather
committed
Refactor clientNumber
1 parent d31bd8c commit 9b67a9c

File tree

4 files changed

+70
-74
lines changed

4 files changed

+70
-74
lines changed

‎coderd/rbac/roles_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ func TestOwnerExec(t *testing.T) {
7777
})
7878
}
7979

80-
// nolint:tparallel,paralleltest -- subtests share a map, just run sequentially.
80+
// nolint:tparallel,paralleltest // subtests share a map, just run sequentially.
8181
func TestRolePermissions(t *testing.T) {
8282
t.Parallel()
8383

@@ -557,7 +557,7 @@ func TestRolePermissions(t *testing.T) {
557557
// nolint:tparallel,paralleltest
558558
for _, c := range testCases {
559559
c := c
560-
// nolint:tparallel,paralleltest -- These share the same remainingPermissions map
560+
// nolint:tparallel,paralleltest // These share the same remainingPermissions map
561561
t.Run(c.Name, func(t *testing.T) {
562562
remainingSubjs := make(map[string]struct{})
563563
for _, subj := range requiredSubjects {
@@ -600,7 +600,7 @@ func TestRolePermissions(t *testing.T) {
600600
// Only run these if the tests on top passed. Otherwise, the error output is too noisy.
601601
if passed {
602602
for rtype, v := range remainingPermissions {
603-
// nolint:tparallel,paralleltest -- Making a subtest for easier diagnosing failures.
603+
// nolint:tparallel,paralleltest // Making a subtest for easier diagnosing failures.
604604
t.Run(fmt.Sprintf("%s-AllActions", rtype), func(t *testing.T) {
605605
if len(v) > 0 {
606606
assert.Equal(t, map[policy.Action]bool{}, v, "remaining permissions should be empty for type %q", rtype)

‎tailnet/test/integration/integration.go

Lines changed: 44 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -41,12 +41,34 @@ import (
4141
"github.com/coder/coder/v2/testutil"
4242
)
4343

44-
// IDs used in tests.
45-
var (
46-
Client1ID = uuid.MustParse("00000000-0000-0000-0000-000000000001")
47-
Client2ID = uuid.MustParse("00000000-0000-0000-0000-000000000002")
44+
type ClientNumber int
45+
46+
const (
47+
ClientNumber1 ClientNumber = 1
48+
ClientNumber2 ClientNumber = 2
4849
)
4950

51+
type Client struct {
52+
Number ClientNumber
53+
ID uuid.UUID
54+
ListenPort uint16
55+
ShouldRunTests bool
56+
}
57+
58+
var Client1 = Client{
59+
Number: ClientNumber1,
60+
ID: uuid.MustParse("00000000-0000-0000-0000-000000000001"),
61+
ListenPort: client1Port,
62+
ShouldRunTests: true,
63+
}
64+
65+
var Client2 = Client{
66+
Number: ClientNumber2,
67+
ID: uuid.MustParse("00000000-0000-0000-0000-000000000002"),
68+
ListenPort: client2Port,
69+
ShouldRunTests: false,
70+
}
71+
5072
type TestTopology struct {
5173
Name string
5274
// SetupNetworking creates interfaces and network namespaces for the test.
@@ -59,12 +81,12 @@ type TestTopology struct {
5981
Server ServerStarter
6082
// StartClient gets called in each client subprocess. It's expected to
6183
// create the tailnet.Conn and ensure connectivity to it's peer.
62-
StartClient func(t *testing.T, logger slog.Logger, serverURL *url.URL, derpMap *tailcfg.DERPMap, clientNumber int, myID uuid.UUID, peerID uuid.UUID) *tailnet.Conn
84+
StartClient func(t *testing.T, logger slog.Logger, serverURL *url.URL, derpMap *tailcfg.DERPMap, me Client, peer Client) *tailnet.Conn
6385

6486
// RunTests is the main test function. It's called in each of the client
6587
// subprocesses. If tests can only run once, they should check the client ID
6688
// and return early if it's not the expected one.
67-
RunTests func(t *testing.T, logger slog.Logger, serverURL *url.URL, myID uuid.UUID, peerID uuid.UUID, conn *tailnet.Conn)
89+
RunTests func(t *testing.T, logger slog.Logger, serverURL *url.URL, conn *tailnet.Conn, me Client, peer Client)
6890
}
6991

7092
type ServerStarter interface {
@@ -264,18 +286,14 @@ http {
264286

265287
// StartClientDERP creates a client connection to the server for coordination
266288
// and creates a tailnet.Conn which will only use DERP to connect to the peer.
267-
func StartClientDERP(t *testing.T, logger slog.Logger, serverURL *url.URL, derpMap *tailcfg.DERPMap, clientNumber int, myID, peerID uuid.UUID) *tailnet.Conn {
268-
listenPort := uint16(client1Port)
269-
if clientNumber == 2 {
270-
listenPort = client2Port
271-
}
272-
return startClientOptions(t, logger, serverURL, myID, peerID, &tailnet.Options{
273-
Addresses: []netip.Prefix{netip.PrefixFrom(tailnet.IPFromUUID(myID), 128)},
289+
func StartClientDERP(t *testing.T, logger slog.Logger, serverURL *url.URL, derpMap *tailcfg.DERPMap, me, peer Client) *tailnet.Conn {
290+
return startClientOptions(t, logger, serverURL, me, peer, &tailnet.Options{
291+
Addresses: []netip.Prefix{netip.PrefixFrom(tailnet.IPFromUUID(me.ID), 128)},
274292
DERPMap: derpMap,
275293
BlockEndpoints: true,
276294
Logger: logger,
277295
DERPForceWebSockets: false,
278-
ListenPort: listenPort,
296+
ListenPort: me.ListenPort,
279297
// These tests don't have internet connection, so we need to force
280298
// magicsock to do anything.
281299
ForceNetworkUp: true,
@@ -284,18 +302,14 @@ func StartClientDERP(t *testing.T, logger slog.Logger, serverURL *url.URL, derpM
284302

285303
// StartClientDERPWebSockets does the same thing as StartClientDERP but will
286304
// only use DERP WebSocket fallback.
287-
func StartClientDERPWebSockets(t *testing.T, logger slog.Logger, serverURL *url.URL, derpMap *tailcfg.DERPMap, clientNumber int, myID, peerID uuid.UUID) *tailnet.Conn {
288-
listenPort := uint16(client1Port)
289-
if clientNumber == 2 {
290-
listenPort = client2Port
291-
}
292-
return startClientOptions(t, logger, serverURL, myID, peerID, &tailnet.Options{
293-
Addresses: []netip.Prefix{netip.PrefixFrom(tailnet.IPFromUUID(myID), 128)},
305+
func StartClientDERPWebSockets(t *testing.T, logger slog.Logger, serverURL *url.URL, derpMap *tailcfg.DERPMap, me, peer Client) *tailnet.Conn {
306+
return startClientOptions(t, logger, serverURL, me, peer, &tailnet.Options{
307+
Addresses: []netip.Prefix{netip.PrefixFrom(tailnet.IPFromUUID(me.ID), 128)},
294308
DERPMap: derpMap,
295309
BlockEndpoints: true,
296310
Logger: logger,
297311
DERPForceWebSockets: true,
298-
ListenPort: listenPort,
312+
ListenPort: me.ListenPort,
299313
// These tests don't have internet connection, so we need to force
300314
// magicsock to do anything.
301315
ForceNetworkUp: true,
@@ -305,25 +319,21 @@ func StartClientDERPWebSockets(t *testing.T, logger slog.Logger, serverURL *url.
305319
// StartClientDirect does the same thing as StartClientDERP but disables
306320
// BlockEndpoints (which enables Direct connections), and waits for a direct
307321
// connection to be established between the two peers.
308-
func StartClientDirect(t *testing.T, logger slog.Logger, serverURL *url.URL, derpMap *tailcfg.DERPMap, clientNumber int, myID, peerID uuid.UUID) *tailnet.Conn {
309-
listenPort := uint16(client1Port)
310-
if clientNumber == 2 {
311-
listenPort = client2Port
312-
}
313-
conn := startClientOptions(t, logger, serverURL, myID, peerID, &tailnet.Options{
314-
Addresses: []netip.Prefix{netip.PrefixFrom(tailnet.IPFromUUID(myID), 128)},
322+
func StartClientDirect(t *testing.T, logger slog.Logger, serverURL *url.URL, derpMap *tailcfg.DERPMap, me, peer Client) *tailnet.Conn {
323+
conn := startClientOptions(t, logger, serverURL, me, peer, &tailnet.Options{
324+
Addresses: []netip.Prefix{netip.PrefixFrom(tailnet.IPFromUUID(me.ID), 128)},
315325
DERPMap: derpMap,
316326
BlockEndpoints: false,
317327
Logger: logger,
318328
DERPForceWebSockets: true,
319-
ListenPort: listenPort,
329+
ListenPort: me.ListenPort,
320330
// These tests don't have internet connection, so we need to force
321331
// magicsock to do anything.
322332
ForceNetworkUp: true,
323333
})
324334

325335
// Wait for direct connection to be established.
326-
peerIP := tailnet.IPFromUUID(peerID)
336+
peerIP := tailnet.IPFromUUID(peer.ID)
327337
require.Eventually(t, func() bool {
328338
t.Log("attempting ping to peer to judge direct connection")
329339
ctx := testutil.Context(t, testutil.WaitShort)
@@ -347,8 +357,8 @@ type ClientStarter struct {
347357
Options *tailnet.Options
348358
}
349359

350-
func startClientOptions(t *testing.T, logger slog.Logger, serverURL *url.URL, myID, peerID uuid.UUID, options *tailnet.Options) *tailnet.Conn {
351-
u, err := serverURL.Parse(fmt.Sprintf("/api/v2/workspaceagents/%s/coordinate", myID.String()))
360+
func startClientOptions(t *testing.T, logger slog.Logger, serverURL *url.URL, me, peer Client, options *tailnet.Options) *tailnet.Conn {
361+
u, err := serverURL.Parse(fmt.Sprintf("/api/v2/workspaceagents/%s/coordinate", me.ID.String()))
352362
require.NoError(t, err)
353363
//nolint:bodyclose
354364
ws, _, err := websocket.Dial(context.Background(), u.String(), nil)
@@ -372,7 +382,7 @@ func startClientOptions(t *testing.T, logger slog.Logger, serverURL *url.URL, my
372382
_ = conn.Close()
373383
})
374384

375-
coordination := tailnet.NewRemoteCoordination(logger, coord, conn, peerID)
385+
coordination := tailnet.NewRemoteCoordination(logger, coord, conn, peer.ID)
376386
t.Cleanup(func() {
377387
_ = coordination.Close()
378388
})

‎tailnet/test/integration/integration_test.go

Lines changed: 21 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ import (
2020
"testing"
2121
"time"
2222

23-
"github.com/google/uuid"
2423
"github.com/stretchr/testify/assert"
2524
"github.com/stretchr/testify/require"
2625
"tailscale.com/net/stun/stuntest"
@@ -50,11 +49,8 @@ var (
5049
// Role: client
5150
clientName = flag.String("client-name", "", "The name of the client for logs")
5251
clientNumber = flag.Int("client-number", 0, "The number of the client")
53-
clientMyID = flag.String("client-id", "", "The id of the client")
54-
clientPeerID = flag.String("client-peer-id", "", "The id of the other client")
5552
clientServerURL = flag.String("client-server-url", "", "The url to connect to the server")
5653
clientDERPMapPath = flag.String("client-derp-map-path", "", "The path to the DERP map file to use on this client")
57-
clientRunTests = flag.Bool("client-run-tests", false, "Run the tests in the client subprocess")
5854
)
5955

6056
func TestMain(m *testing.M) {
@@ -183,8 +179,8 @@ func TestIntegration(t *testing.T) {
183179
require.NoError(t, err, "write client 2 DERP map")
184180

185181
// client1 runs the tests.
186-
client1ErrCh, _ := startClientSubprocess(t, topo.Name, networking, 1, client1DERPMapPath)
187-
_, closeClient2 := startClientSubprocess(t, topo.Name, networking, 2, client2DERPMapPath)
182+
client1ErrCh, _ := startClientSubprocess(t, topo.Name, networking, integration.Client1, client1DERPMapPath)
183+
_, closeClient2 := startClientSubprocess(t, topo.Name, networking, integration.Client2, client2DERPMapPath)
188184

189185
// Wait for client1 to exit.
190186
require.NoError(t, <-client1ErrCh, "client 1 exited")
@@ -230,15 +226,16 @@ func handleTestSubprocess(t *testing.T) {
230226

231227
case "client":
232228
logger = logger.Named(*clientName)
233-
if *clientNumber != 1 && *clientNumber != 2 {
229+
if *clientNumber != int(integration.ClientNumber1) && *clientNumber != int(integration.ClientNumber2) {
234230
t.Fatalf("invalid client number %d", clientNumber)
235231
}
232+
me, peer := integration.Client1, integration.Client2
233+
if *clientNumber == int(integration.ClientNumber2) {
234+
me, peer = peer, me
235+
}
236+
236237
serverURL, err := url.Parse(*clientServerURL)
237238
require.NoErrorf(t, err, "parse server url %q", *clientServerURL)
238-
myID, err := uuid.Parse(*clientMyID)
239-
require.NoErrorf(t, err, "parse client id %q", *clientMyID)
240-
peerID, err := uuid.Parse(*clientPeerID)
241-
require.NoErrorf(t, err, "parse peer id %q", *clientPeerID)
242239

243240
// Load the DERP map.
244241
var derpMap tailcfg.DERPMap
@@ -251,16 +248,16 @@ func handleTestSubprocess(t *testing.T) {
251248

252249
waitForServerAvailable(t, serverURL)
253250

254-
conn := topo.StartClient(t, logger, serverURL, &derpMap, *clientNumber, myID, peerID)
251+
conn := topo.StartClient(t, logger, serverURL, &derpMap, me, peer)
255252

256-
if *clientRunTests {
253+
if me.ShouldRunTests {
257254
// Wait for connectivity.
258-
peerIP := tailnet.IPFromUUID(peerID)
255+
peerIP := tailnet.IPFromUUID(peer.ID)
259256
if !conn.AwaitReachable(testutil.Context(t, testutil.WaitLong), peerIP) {
260257
t.Fatalf("peer %v did not become reachable", peerIP)
261258
}
262259

263-
topo.RunTests(t, logger, serverURL, myID, peerID, conn)
260+
topo.RunTests(t, logger, serverURL, conn, me, peer)
264261
// then exit
265262
return
266263
}
@@ -338,36 +335,26 @@ func startSTUNSubprocess(t *testing.T, topologyName string, networking integrati
338335
return closeFn
339336
}
340337

341-
func startClientSubprocess(t *testing.T, topologyName string, networking integration.TestNetworking, clientNumber int, derpMapPath string) (<-chan error, func() error) {
342-
require.True(t, clientNumber == 1 || clientNumber == 2)
343-
338+
func startClientSubprocess(t *testing.T, topologyName string, networking integration.TestNetworking, me integration.Client, derpMapPath string) (<-chan error, func() error) {
344339
var (
345-
clientName = fmt.Sprintf("client%d", clientNumber)
346-
myID = integration.Client1ID
347-
peerID = integration.Client2ID
348-
clientConfig = networking.Client1
340+
clientName = fmt.Sprintf("client%d", me.Number)
341+
clientProcessConfig = networking.Client1
349342
)
350-
if clientNumber == 2 {
351-
myID, peerID = peerID, myID
352-
clientConfig = networking.Client2
343+
if me.Number == integration.ClientNumber2 {
344+
clientProcessConfig = networking.Client2
353345
}
354346

355347
flags := []string{
356348
"--subprocess",
357349
"--test-name=" + topologyName,
358350
"--role=client",
359351
"--client-name=" + clientName,
360-
"--client-number=" + strconv.Itoa(clientNumber),
361-
"--client-server-url=" + clientConfig.ServerAccessURL,
362-
"--client-id=" + myID.String(),
363-
"--client-peer-id=" + peerID.String(),
352+
"--client-number=" + strconv.Itoa(int(me.Number)),
353+
"--client-server-url=" + clientProcessConfig.ServerAccessURL,
364354
"--client-derp-map-path=" + derpMapPath,
365355
}
366-
if clientNumber == 1 {
367-
flags = append(flags, "--client-run-tests")
368-
}
369356

370-
return startSubprocess(t, clientName, clientConfig.Process.NetNS, flags)
357+
return startSubprocess(t, clientName, clientProcessConfig.Process.NetNS, flags)
371358
}
372359

373360
// startSubprocess launches the test binary with the same flags as the test, but
@@ -377,7 +364,7 @@ func startClientSubprocess(t *testing.T, topologyName string, networking integra
377364
func startSubprocess(t *testing.T, processName string, netNS *os.File, flags []string) (<-chan error, func() error) {
378365
name := os.Args[0]
379366
// Always use verbose mode since it gets piped to the parent test anyways.
380-
args := append(os.Args[1:], append([]string{"-test.v=true"}, flags...)...)
367+
args := append(os.Args[1:], append([]string{"-test.v=true"}, flags...)...) //nolint:gocritic
381368
return integration.ExecBackground(t, processName, netNS, name, args)
382369
}
383370

‎tailnet/test/integration/suite.go

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77
"net/url"
88
"testing"
99

10-
"github.com/google/uuid"
1110
"github.com/stretchr/testify/require"
1211

1312
"cdr.dev/slog"
@@ -17,12 +16,12 @@ import (
1716

1817
// TODO: instead of reusing one conn for each suite, maybe we should make a new
1918
// one for each subtest?
20-
func TestSuite(t *testing.T, _ slog.Logger, _ *url.URL, _, peerID uuid.UUID, conn *tailnet.Conn) {
19+
func TestSuite(t *testing.T, _ slog.Logger, _ *url.URL, conn *tailnet.Conn, _, peer Client) {
2120
t.Parallel()
2221

2322
t.Run("Connectivity", func(t *testing.T) {
2423
t.Parallel()
25-
peerIP := tailnet.IPFromUUID(peerID)
24+
peerIP := tailnet.IPFromUUID(peer.ID)
2625
_, _, _, err := conn.Ping(testutil.Context(t, testutil.WaitLong), peerIP)
2726
require.NoError(t, err, "ping peer")
2827
})

0 commit comments

Comments
 (0)