From 37974d6dece813fc9bddafdcd2c31838d3c12fce Mon Sep 17 00:00:00 2001 From: Janne Sinivirta Date: Tue, 11 Aug 2026 16:00:09 +0300 Subject: [PATCH] Make tests independent of the machine they run on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four tests depended on the developer's environment rather than the code. TestFindFile_StopAtHome created and RemoveAll'd ~/test_preflight inside the real home directory, so running the suite could delete a contributor's files. It also failed outright for anyone with a global ~/.preflight — the config location this very package exists to discover. Now uses a temporary HOME, plus a companion test that a .preflight above HOME is not picked up. TestFindFile_StopAtGit wrote .preflight into the directory it searched from, so FindFile returned on the first iteration and never reached the .git boundary it claimed to test. Confirmed by deleting that logic: the old test still passed, the new one fails. Added the mirror case so the error is known to come from the boundary rather than from the walk failing generally. The gitcheck runner tests inherited ~/.gitconfig. commit.gpgsign=true, which plenty of people set, aborted the commit with exit 128; a global core.hooksPath with a failing pre-commit hook did the same. They now run with GIT_CONFIG_GLOBAL and GIT_CONFIG_SYSTEM pointed at /dev/null, and report git's output on failure instead of a bare exit status. TestTCPCommand/unreachable_port assumed 127.0.0.1:1 refuses connections. A process bound to the IPv6 wildcard accepts IPv4-mapped connections there, so it passed in CI and failed on macOS. Binds and closes a port instead, which is what the adjacent subtest already did. Also removes two subtests that asserted nothing: "clean flag" asserted inside `if err != nil` with no else and matched ErrCheckFailed's text, which every failing check returns; and "tag flag" passed --tag, which does not exist, so cobra's parse error satisfied assert.Error and no tag logic ran. The real flag is --tag-match, now covered. --- cmd/preflight/execute_test.go | 37 ++++++++--- pkg/gitcheck/runner_test.go | 28 ++++++-- pkg/preflightfile/preflightfile_test.go | 85 ++++++++++++++----------- 3 files changed, 99 insertions(+), 51 deletions(-) diff --git a/cmd/preflight/execute_test.go b/cmd/preflight/execute_test.go index a384de9..1f1cec8 100644 --- a/cmd/preflight/execute_test.go +++ b/cmd/preflight/execute_test.go @@ -254,7 +254,16 @@ func TestTCPCommand(t *testing.T) { }) t.Run("unreachable port", func(t *testing.T) { - _, err := executeCommand("tcp", "--timeout", "100ms", "127.0.0.1:1") + // Bind then close, so the port is known to have nothing on it. Port 1 + // is not safe to assume free: a process bound to the IPv6 wildcard + // accepts IPv4-mapped connections to 127.0.0.1:1, which made this pass + // in CI and fail on developer machines. + listener, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + addr := listener.Addr().String() + require.NoError(t, listener.Close()) + + _, err = executeCommand("tcp", "--timeout", "100ms", addr) assert.Error(t, err) }) } @@ -304,16 +313,26 @@ func TestHashCommand(t *testing.T) { } func TestGitCommand(t *testing.T) { - t.Run("clean flag", func(t *testing.T) { - _, err := executeCommand("git", "--clean") - if err != nil { - assert.Contains(t, err.Error(), "check failed") - } + // "clean flag" used to live here, asserting inside `if err != nil` with no + // else. It could not fail: a clean tree asserted nothing, and a dirty tree + // only matched ErrCheckFailed's text, which every failing check returns. Its + // result also depended on the developer's uncommitted files, and it printed + // their private file list into test output. pkg/gitcheck covers the logic + // against mocks, so it is gone rather than rewritten. + + t.Run("unknown flag is rejected", func(t *testing.T) { + // Previously "tag flag with nonexistent tag", passing --tag. There is no + // --tag flag, so assert.Error was satisfied by cobra's parse error and + // nothing about tag matching ran. The real flag is --tag-match. + _, err := executeCommand("git", "--tag", "v999.999.999") + require.Error(t, err) + assert.Contains(t, err.Error(), "unknown flag") }) - t.Run("tag flag with nonexistent tag", func(t *testing.T) { - _, err := executeCommand("git", "--tag", "v999.999.999") - assert.Error(t, err) + t.Run("tag-match against a tag that cannot exist", func(t *testing.T) { + _, err := executeCommand("git", "--tag-match", "v999.999.999-nonexistent") + require.Error(t, err) + assert.ErrorIs(t, err, ErrCheckFailed) }) t.Run("missing argument", func(t *testing.T) { diff --git a/pkg/gitcheck/runner_test.go b/pkg/gitcheck/runner_test.go index 7deafc9..d288827 100644 --- a/pkg/gitcheck/runner_test.go +++ b/pkg/gitcheck/runner_test.go @@ -37,6 +37,22 @@ func TestRealGitRunner_TagsAtHead(t *testing.T) { assert.NoError(t, err) } +// git runs a command with the developer's global and system config ignored. +// Inheriting them made these tests fail on real machines: commit.gpgsign=true +// aborts the commit with exit 128, and a global core.hooksPath with a failing +// pre-commit hook does the same. +func git(t *testing.T, args ...string) { + t.Helper() + cmd := exec.Command("git", args...) //nolint:gosec // args are literals from this test file + cmd.Env = append(os.Environ(), + "GIT_CONFIG_GLOBAL=/dev/null", + "GIT_CONFIG_SYSTEM=/dev/null", + "GIT_TERMINAL_PROMPT=0", + ) + out, err := cmd.CombinedOutput() + require.NoError(t, err, "git %v: %s", args, out) +} + func TestRealGitRunner_IsGitRepo_NotRepo(t *testing.T) { tmpDir := t.TempDir() oldWd, err := os.Getwd() @@ -60,13 +76,13 @@ func TestRealGitRunner_TagsAtHead_WithTag(t *testing.T) { require.NoError(t, os.Chdir(tmpDir)) // Initialize git repo with a commit and tag - require.NoError(t, exec.Command("git", "init").Run()) - require.NoError(t, exec.Command("git", "config", "user.email", "test@test.com").Run()) - require.NoError(t, exec.Command("git", "config", "user.name", "Test").Run()) + git(t, "init") + git(t, "config", "user.email", "test@test.com") + git(t, "config", "user.name", "Test") require.NoError(t, os.WriteFile("test.txt", []byte("test"), 0o600)) - require.NoError(t, exec.Command("git", "add", "test.txt").Run()) - require.NoError(t, exec.Command("git", "commit", "-m", "initial").Run()) - require.NoError(t, exec.Command("git", "tag", "v1.0.0").Run()) + git(t, "add", "test.txt") + git(t, "commit", "-m", "initial") + git(t, "tag", "v1.0.0") runner := &RealGitRunner{} tags, err := runner.TagsAtHead() diff --git a/pkg/preflightfile/preflightfile_test.go b/pkg/preflightfile/preflightfile_test.go index 0d8de96..ac60640 100644 --- a/pkg/preflightfile/preflightfile_test.go +++ b/pkg/preflightfile/preflightfile_test.go @@ -5,6 +5,8 @@ import ( "path/filepath" "reflect" "testing" + + "github.com/stretchr/testify/require" ) func TestFindFile_ExplicitPath(t *testing.T) { @@ -51,54 +53,65 @@ func TestFindFile_TraverseUp(t *testing.T) { } } +// A repo root is a boundary: a .preflight above it belongs to something else. +// The .preflight must live only in the parent, or FindFile returns on its first +// iteration and the .git logic is never reached — which is what the previous +// version of this test did, so it passed with that logic deleted. func TestFindFile_StopAtGit(t *testing.T) { tmpDir := t.TempDir() - projectDir := filepath.Join(tmpDir, "project") - gitDir := filepath.Join(projectDir, ".git") - if err := os.MkdirAll(gitDir, 0o700); err != nil { - t.Fatalf("failed to create directories: %v", err) - } + require.NoError(t, os.MkdirAll(filepath.Join(projectDir, ".git"), 0o700)) + require.NoError(t, os.WriteFile(filepath.Join(tmpDir, ".preflight"), []byte("env HOME\n"), 0o600)) - preflightPath := filepath.Join(tmpDir, ".preflight") - if err := os.WriteFile(preflightPath, []byte("test"), 0o600); err != nil { - t.Fatalf("failed to create test file: %v", err) - } + _, err := FindFile(projectDir, "") + require.Error(t, err, ".git is a boundary: the parent's .preflight is not ours") +} - projectPreflight := filepath.Join(projectDir, ".preflight") - if err := os.WriteFile(projectPreflight, []byte("test"), 0o600); err != nil { - t.Fatalf("failed to create test file: %v", err) - } +// The mirror image: without a .git boundary the parent's file is found, which +// pins that the error above comes from the boundary and not from the walk +// failing generally. +func TestFindFile_WalksUpWithoutGitBoundary(t *testing.T) { + tmpDir := t.TempDir() + projectDir := filepath.Join(tmpDir, "project") + require.NoError(t, os.MkdirAll(projectDir, 0o700)) + parentPreflight := filepath.Join(tmpDir, ".preflight") + require.NoError(t, os.WriteFile(parentPreflight, []byte("env HOME\n"), 0o600)) found, err := FindFile(projectDir, "") - if err != nil { - t.Fatalf("FindFile failed: %v", err) - } - if found != projectPreflight { - t.Errorf("expected %q, got %q", projectPreflight, found) - } + require.NoError(t, err) + require.Equal(t, parentPreflight, found) } +// The search stops at $HOME. This uses a temporary HOME rather than the real +// one: the previous version created and RemoveAll'd ~/test_preflight on the +// developer's machine, and failed outright for anyone who has a global +// ~/.preflight — which is a supported location this package exists to find. func TestFindFile_StopAtHome(t *testing.T) { - homeDir, err := os.UserHomeDir() - if err != nil { - t.Fatalf("failed to get home directory: %v", err) - } + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) // os.UserHomeDir on Windows - testDir := filepath.Join(homeDir, "test_preflight") - if err := os.MkdirAll(testDir, 0o700); err != nil { - t.Fatalf("failed to create test directory: %v", err) - } - defer func() { - if err := os.RemoveAll(testDir); err != nil { - t.Errorf("failed to clean up test directory: %v", err) - } - }() + testDir := filepath.Join(home, "project") + require.NoError(t, os.MkdirAll(testDir, 0o700)) - _, err = FindFile(testDir, "") - if err == nil { - t.Error("expected error when .preflight not found") - } + _, err := FindFile(testDir, "") + require.Error(t, err, "no .preflight anywhere up to HOME") +} + +func TestFindFile_StopsBeforeLeavingHome(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + + // A .preflight above HOME must not be picked up. + require.NoError(t, os.WriteFile(filepath.Join(filepath.Dir(home), ".preflight"), []byte("env HOME\n"), 0o600)) + t.Cleanup(func() { _ = os.Remove(filepath.Join(filepath.Dir(home), ".preflight")) }) + + testDir := filepath.Join(home, "project") + require.NoError(t, os.MkdirAll(testDir, 0o700)) + + _, err := FindFile(testDir, "") + require.Error(t, err, "search must stop at HOME, not walk past it") } func TestParseFile(t *testing.T) {