test: make the suite race-clean and order-independent

.github/workflows/ci.yml

@@ -53,8 +53,13 @@ jobs:
       - name: Vet
         run: go vet ./...
 
+      # -race and -shuffle because the suite shares package state (the cobra
+      # tree, swappable package vars, time.Local) and both a race on it and a
+      # test that only passed after another one went unseen until #55. A
+      # shuffle failure prints its -test.shuffle seed: rerun with that seed to
+      # reproduce the order.
       - name: Test
-        run: go test ./...
+        run: go test -race -shuffle=on ./...
 
       # The release builds four targets from this same source. A build that
       # only works on the runner's own platform is worth knowing about before

cmd/automations_test.go

@@ -617,9 +617,18 @@ func TestBrowserSessionRequiredSaysWhy(t *testing.T) {
 }
 
 // execute runs the real command tree, as main does, and returns how it
-// would exit.
+// would exit. The tree is package state: its flags start at their defaults
+// and go back to them when the test ends, so no test sees what another one
+// parsed, whatever order they run in.
 func execute(t *testing.T, args ...string) (int, string) {
 	t.Helper()
+	resetCommandFlags(rootCmd)
+	t.Cleanup(func() {
+		resetCommandFlags(rootCmd)
+		rootCmd.SetArgs(nil)
+		rootCmd.SetOut(nil)
+		rootCmd.SetErr(nil)
+	})
 	var out bytes.Buffer
 	rootCmd.SetArgs(args)
 	rootCmd.SetOut(&out)

cmd/byok_test.go

@@ -11,7 +11,6 @@ import (
 	"path/filepath"
 	"strings"
 	"testing"
-	"time"
 
 	"github.com/nxssie/nan-cli/internal/api"
 )
@@ -237,9 +236,6 @@ func checkGolden(t *testing.T, rel, got string) {
 func TestBYOKGoldenViews(t *testing.T) {
 	resetPhase2Flags()
 	defer resetPhase2Flags()
-	loc := time.Local
-	time.Local = time.UTC
-	defer func() { time.Local = loc }()
 	ctx := context.Background()
 
 	for _, tc := range []struct {

cmd/main_test.go

@@ -0,0 +1,27 @@
+package cmd
+
+import (
+	"os"
+	"testing"
+	"time"
+)
+
+// TestMain pins the clock's zone before any test or goroutine starts.
+//
+// Rendered times (runs ls, golden views) go through t.Local(), so the tests
+// need one fixed zone. Swapping time.Local inside a test races with every
+// goroutine that calls time.Now() meanwhile, including net/http servers left
+// over from earlier tests; set once here, nothing reads it concurrently.
+func TestMain(m *testing.M) {
+	time.Local = time.UTC
+	os.Exit(m.Run())
+}
+
+// swap sets *p to v for the rest of the test and puts the old value back
+// when it ends, whatever order the tests run in.
+func swap[T any](t testing.TB, p *T, v T) {
+	t.Helper()
+	old := *p
+	*p = v
+	t.Cleanup(func() { *p = old })
+}

cmd/review_test.go

@@ -236,14 +236,7 @@ func TestJSONOutputNeutralisesALoneC1Byte(t *testing.T) {
 
 func TestContradictoryWorktreeFlagsSayWhatIsWrong(t *testing.T) {
 	t.Setenv("HOME", t.TempDir())
-	rootCmd.SetArgs([]string{"run", "--worktree", "--no-worktree", "x"})
-	rootCmd.SetOut(io.Discard)
-	rootCmd.SetErr(io.Discard)
-	cmd, err := rootCmd.ExecuteC()
-	code, msg := exitCodeFor(cmd, err)
-	runOpts = runOptions{agent: "pi", timeout: 30 * time.Minute}
-	_ = runCmd.Flags().Set("worktree", "false")
-	_ = runCmd.Flags().Set("no-worktree", "false")
+	code, msg := execute(t, "run", "--worktree", "--no-worktree", "x")
 	if code != exitUsage || !strings.Contains(msg, "pick one") {
 		t.Errorf("exit %d: %s", code, msg)
 	}

cmd/root_test.go

@@ -1,7 +1,6 @@
 package cmd
 
 import (
-	"bytes"
 	"testing"
 
 	"github.com/nxssie/nan-cli/internal/tui"
@@ -10,24 +9,18 @@ import (
 // `nan --version` and `nan -v` print one line and exit 0, without opening
 // the dashboard.
 func TestVersionPrintsOneLine(t *testing.T) {
-	old := openTUI
-	defer func() { openTUI = old }()
-	openTUI = func() error {
+	swap(t, &openTUI, func() error {
 		t.Error("--version opened the dashboard")
 		return nil
-	}
+	})
 
 	for _, flag := range []string{"--version", "-v"} {
-		var out bytes.Buffer
-		rootCmd.SetArgs([]string{flag})
-		rootCmd.SetOut(&out)
-		rootCmd.SetErr(&out)
-		if err := rootCmd.Execute(); err != nil {
-			t.Fatalf("%s: %v", flag, err)
+		code, out := execute(t, flag)
+		if code != 0 {
+			t.Fatalf("%s: exit %d: %s", flag, code, out)
 		}
-		if got, want := out.String(), "nan "+tui.Version+"\n"; got != want {
+		if got, want := out, "nan "+tui.Version+"\n"; got != want {
 			t.Errorf("%s printed %q, want %q", flag, got, want)
 		}
-		_ = rootCmd.Flags().Set("version", "false")
 	}
 }

cmd/run_test.go

@@ -665,17 +665,9 @@ func TestCobraRefusalsExit64OnRunCommands(t *testing.T) {
 		{"runs", "show"},
 		{"runs", "logs", "a", "b"},
 	} {
-		rootCmd.SetArgs(args)
-		rootCmd.SetOut(io.Discard)
-		rootCmd.SetErr(io.Discard)
-		cmd, err := rootCmd.ExecuteC()
-		code, _ := exitCodeFor(cmd, err)
-		if err == nil || code != exitUsage {
-			t.Errorf("%v: exit %d (%v)", args, code, err)
-		}
-		runOpts = runOptions{agent: "pi", timeout: 30 * time.Minute}
-		_ = runCmd.Flags().Set("worktree", "false")
-		_ = runCmd.Flags().Set("no-worktree", "false")
+		if code, msg := execute(t, args...); code != exitUsage {
+			t.Errorf("%v: exit %d (%s)", args, code, msg)
+		}
 	}
 	if code, _ := exitCodeFor(meCmd, errors.New("boom")); code != 1 {
 		t.Errorf("an ordinary command's failure exits %d", code)

cmd/writeapi_test.go

@@ -14,7 +14,6 @@ import (
 	"strings"
 	"sync"
 	"testing"
-	"time"
 
 	"github.com/nxssie/nan-cli/internal/api"
 
@@ -313,21 +312,13 @@ func runWrite(t *testing.T, fake *fakeWrite, setup writeSetup, args ...string) w
 	home := fakeHome(t, setup.session)
 	t.Setenv(tokenEnvVar, setup.env)
 
-	oldBase, oldIn, oldTTY, oldOut, oldErr, oldAttempts := runsBaseURL, writeStdin, writeStdinTTY, writeStdout, writeStderr, createAttempts
-	t.Cleanup(func() {
-		runsBaseURL, writeStdin, writeStdinTTY, writeStdout, writeStderr, createAttempts = oldBase, oldIn, oldTTY, oldOut, oldErr, oldAttempts
-		resetCommandFlags(rootCmd)
-	})
-	runsBaseURL = srv.URL
-	loc := time.Local
-	time.Local = time.UTC
-	t.Cleanup(func() { time.Local = loc })
-	writeStdin = strings.NewReader(setup.stdin)
+	swap(t, &runsBaseURL, srv.URL)
+	swap(t, &writeStdin, io.Reader(strings.NewReader(setup.stdin)))
 	tty := setup.tty
-	writeStdinTTY = func() bool { return tty }
+	swap(t, &writeStdinTTY, func() bool { return tty })
 	var out, errOut bytes.Buffer
-	writeStdout, writeStderr = &out, &errOut
-	resetCommandFlags(rootCmd)
+	swap(t, &writeStdout, io.Writer(&out))
+	swap(t, &writeStderr, io.Writer(&errOut))
 
 	code, msg := execute(t, args...)
 	return writeRun{code: code, msg: msg, stdout: out.String(), stderr: errOut.String(), home: home}
@@ -434,14 +425,13 @@ func TestSecretsSetReadsTheValueExactly(t *testing.T) {
 		"abc":       "abc",
 		" a b c \n": " a b c ",
 	} {
-		writeStdin = strings.NewReader(in)
-		writeStdinTTY = func() bool { return false }
+		swap(t, &writeStdin, io.Reader(strings.NewReader(in)))
+		swap(t, &writeStdinTTY, func() bool { return false })
 		got, err := readSecretValue("", true)
 		if err != nil || got != want {
 			t.Errorf("%q: got %q, %v", in, got, err)
 		}
 	}
-	writeStdin, writeStdinTTY = os.Stdin, func() bool { return isTerminal(os.Stdin) }
 }
 
 func TestSecretsSetRefusals(t *testing.T) {
@@ -803,8 +793,7 @@ func TestReadBodyFile(t *testing.T) {
 			t.Errorf("%s: %v", name, err)
 		}
 	}
-	writeStdinTTY = func() bool { return true }
-	defer func() { writeStdinTTY = func() bool { return isTerminal(os.Stdin) } }()
+	swap(t, &writeStdinTTY, func() bool { return true })
 	if _, err := readBodyFile("-"); exitCode(err) != exitUsage || !strings.Contains(err.Error(), "terminal") {
 		t.Errorf("tty stdin: %v", err)
 	}
@@ -1051,7 +1040,7 @@ func TestSecretsSetWithoutRetryFailsOnALostAnswer(t *testing.T) {
 	fake := newFakeWrite()
 	fake.dropFirstCreate = true
 	file := writeValueFile(t, testValue)
-	createAttempts = 1
+	swap(t, &createAttempts, 1)
 	r := runWrite(t, fake, writeSetup{session: cliSession}, "secrets", "set", "NEW_TOKEN", "--value-file", file)
 	if r.code == 0 || len(fake.sent("POST", "/v1/secrets")) != 1 {
 		t.Fatalf("exit %d, posts %v", r.code, fake.sent("POST", "/v1/secrets"))
@@ -1144,9 +1133,8 @@ func TestSecretWriteNotFound(t *testing.T) {
 }
 
 func TestSecretValueSizeLimit(t *testing.T) {
-	defer func() { writeStdin, writeStdinTTY = os.Stdin, func() bool { return isTerminal(os.Stdin) } }()
-	writeStdinTTY = func() bool { return false }
-	writeStdin = strings.NewReader(strings.Repeat("a", maxSecretValue) + "\n")
+	swap(t, &writeStdinTTY, func() bool { return false })
+	swap(t, &writeStdin, io.Reader(strings.NewReader(strings.Repeat("a", maxSecretValue)+"\n")))
 	if v, err := readSecretValue("", true); err != nil || len(v) != maxSecretValue {
 		t.Errorf("16 KiB: %d %v", len(v), err)
 	}