From 258bb67911b7653005c05bbcbd90279c5e1efe74 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 18 May 2026 01:57:03 +0000 Subject: [PATCH] Replace log.Fatal in tests, add t.Helper, cover NewFile Three test-infrastructure fixes from issue #33: 1. Replace every test-side log.Fatal* with t.Fatalf / b.Fatalf. log.Fatal calls os.Exit(1), which kills the entire test binary without running any deferred cleanup (so temp files leak), without printing the test name that failed, and prevents subsequent tests from running. Affected helpers: newTestFile, newTestDir, TestHandleFileWithIgnoredDir. To make newTestFile/newTestDir usable from both *testing.T and *testing.B call sites, their signatures now take testing.TB as the first argument. All call sites are updated. 2. Add t.Helper() / b.Helper() to every test helper that takes *testing.T or *testing.B: assertFileExists, assertFileNonexistent, assertPathExistsAfterRename, assertNewContentsOfFile, assertRandomStringLength, newTestFile, newTestDir, and CloneRepoToTestDir. Failures are now attributed to the call site instead of the helper body. Verified by inducing a failure in assertFileExists from a throwaway test file: the reported file:line was the call site, not the helper. 3. Replace the empty TestNewFile with real, table-driven coverage: absolute-path inputs returned cleaned, redundant separators collapsed, .. resolved, relative paths resolved to absolute, and the working-directory case ("."). The filepath.Abs error path is not covered here because NewFile currently calls log.Fatalf on that failure (kills the test binary); the error path becomes testable when NewFile is refactored per issue #6. No production code is changed. Closes #33 --- file_handling_test.go | 76 ++++++++++++++++++++++++++++++++++++++++++- find_replace_test.go | 62 +++++++++++++++++++---------------- strings_test.go | 1 + 3 files changed, 110 insertions(+), 29 deletions(-) diff --git a/file_handling_test.go b/file_handling_test.go index 9dfcaee..0296964 100644 --- a/file_handling_test.go +++ b/file_handling_test.go @@ -1,6 +1,80 @@ package main -import "testing" +import ( + "path/filepath" + "testing" +) +// TestNewFile exercises NewFile's path-resolution behavior. It does NOT cover +// the filepath.Abs error path: NewFile currently calls log.Fatalf on that +// failure, which would kill the test binary, and that surface is not reachable +// on most platforms in any case. The error path will become testable when +// NewFile is refactored to return an error (see issue #6). func TestNewFile(t *testing.T) { + tmp := t.TempDir() + + tests := []struct { + name string + // input is the raw path passed to NewFile. + input string + // want is the absolute path the resulting *File should expose. If + // empty, the test computes the expected value via filepath.Abs(input) + // at runtime (useful for inputs that are inherently relative to the + // test process's working directory). + want string + }{ + { + name: "absolute path is returned cleaned", + input: filepath.Join(tmp, "foo"), + want: filepath.Join(tmp, "foo"), + }, + { + name: "absolute path with redundant separators is cleaned", + input: tmp + "//foo///bar", + want: filepath.Join(tmp, "foo", "bar"), + }, + { + name: "absolute path with .. is resolved", + input: filepath.Join(tmp, "a", "..", "b"), + want: filepath.Join(tmp, "b"), + }, + { + name: "relative path is resolved to absolute", + input: "relative/path", + // want is computed below because it depends on the test + // process's working directory. + }, + { + name: "relative path with .. is resolved", + input: "a/../b", + }, + { + name: "dot is resolved to the working directory", + input: ".", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + want := tc.want + if want == "" { + abs, err := filepath.Abs(tc.input) + if err != nil { + t.Fatalf("filepath.Abs(%q) returned unexpected error: %v", tc.input, err) + } + want = abs + } + + got := NewFile(tc.input) + if got == nil { + t.Fatalf("NewFile(%q) returned nil", tc.input) + } + if got.Path != want { + t.Errorf("NewFile(%q).Path = %q; want %q", tc.input, got.Path, want) + } + if !filepath.IsAbs(got.Path) { + t.Errorf("NewFile(%q).Path = %q; want an absolute path", tc.input, got.Path) + } + }) + } } diff --git a/find_replace_test.go b/find_replace_test.go index 5bfda8a..e52014e 100644 --- a/find_replace_test.go +++ b/find_replace_test.go @@ -2,7 +2,6 @@ package main import ( "errors" - "log" "os" "os/exec" "path/filepath" @@ -19,18 +18,19 @@ import ( // If a baseName is not provided, a random file name is generated. Returns the // directory where the file was created, the file's directory entry, and the // actual name of the file. -func newTestFile(path string, baseName string, content string) *File { +func newTestFile(tb testing.TB, path string, baseName string, content string) *File { + tb.Helper() f, err := os.CreateTemp(path, baseName) if err != nil { - log.Fatal(err) + tb.Fatalf("CreateTemp(%q, %q): %v", path, baseName, err) } if _, err := f.Write([]byte(content)); err != nil { defer os.Remove(f.Name()) - log.Fatal(err) + tb.Fatalf("write to %v: %v", f.Name(), err) } if err := f.Close(); err != nil { defer os.Remove(f.Name()) - log.Fatal(err) + tb.Fatalf("close %v: %v", f.Name(), err) } return NewFile(f.Name()) @@ -41,10 +41,11 @@ func newTestFile(path string, baseName string, content string) *File { // a baseName is not provided, a random file name is generated. Returns the // directory where the file was created, the file's directory entry, and the // actual name of the file. -func newTestDir(path string, baseName string) *File { +func newTestDir(tb testing.TB, path string, baseName string) *File { + tb.Helper() dirPath, err := os.MkdirTemp(path, baseName) if err != nil { - log.Fatal(err) + tb.Fatalf("MkdirTemp(%q, %q): %v", path, baseName, err) } return NewFile(dirPath) } @@ -59,6 +60,7 @@ func expectedPathAfterRename(f *File, fr *findReplace) string { // assertFileExists ensures that the given File exists func assertFileExists(t *testing.T, f *File) { + t.Helper() if _, err := os.Stat(f.Path); errors.Is(err, os.ErrNotExist) { t.Errorf("test file %v does not exist", f.Path) } @@ -66,6 +68,7 @@ func assertFileExists(t *testing.T, f *File) { // assertFileNonexistent ensures that the File does not exist func assertFileNonexistent(t *testing.T, f *File) { + t.Helper() if _, err := os.Stat(f.Path); !errors.Is(err, os.ErrNotExist) { if err == nil { t.Errorf("test file %v exists", f.Path) @@ -76,6 +79,7 @@ func assertFileNonexistent(t *testing.T, f *File) { } func assertPathExistsAfterRename(t *testing.T, f *File, expectedPath string) *File { + t.Helper() assertFileNonexistent(t, f) newFile := NewFile(expectedPath) assertFileExists(t, newFile) @@ -95,50 +99,50 @@ func TestWalkDir(t *testing.T) { find := "wh" replace := "f" - d := newTestDir("", "*") + d := newTestDir(t, "", "*") defer os.Remove(d.Path) // d1: who/ - d1 := newTestDir(d.Path, "who") + d1 := newTestDir(t, d.Path, "who") defer os.Remove(d1.Path) // d1d1: who/what/ - d1d1 := newTestDir(d1.Path, "what") + d1d1 := newTestDir(t, d1.Path, "what") defer os.Remove(d1d1.Path) // d1d1f1: who/what/when (contains "where") d1d1f1Contents := "where" - d1d1f1 := newTestFile(d1d1.Path, "when", d1d1f1Contents) + d1d1f1 := newTestFile(t, d1d1.Path, "when", d1d1f1Contents) defer os.Remove(d1d1f1.Path) // d2: what/ - d2 := newTestDir(d.Path, "what") + d2 := newTestDir(t, d.Path, "what") defer os.Remove(d2.Path) // d2d1: what/when/ - d2d1 := newTestDir(d2.Path, "when") + d2d1 := newTestDir(t, d2.Path, "when") defer os.Remove(d2d1.Path) // d2d1d1: what/when/where (directories with no files) - d2d1d1 := newTestDir(d2d1.Path, "where") + d2d1d1 := newTestDir(t, d2d1.Path, "where") defer os.Remove(d2d1d1.Path) // d3: when/ - d3 := newTestDir(d.Path, "when") + d3 := newTestDir(t, d.Path, "when") defer os.Remove(d3.Path) // d3f1: when/where (contains "why") d3f1Contents := "why" - d3f1 := newTestFile(d3.Path, "where", d3f1Contents) + d3f1 := newTestFile(t, d3.Path, "where", d3f1Contents) defer os.Remove(d3f1.Path) // d4: where/ (empty directory in base dir) - d4 := newTestDir(d.Path, "where") + d4 := newTestDir(t, d.Path, "where") defer os.Remove(d4.Path) // f1: why (file in base dir contains "wh") f1Contents := "wh\nwh\nwh\n" - f1 := newTestFile(d.Path, "why", f1Contents) + f1 := newTestFile(t, d.Path, "why", f1Contents) defer os.Remove(f1.Path) fr := findReplace{find: find, replace: replace} @@ -193,7 +197,7 @@ func TestHandleFileWithDir(t *testing.T) { find := "ph" replace := "f" - f := newTestDir("", initial) + f := newTestDir(t, "", initial) defer os.Remove(f.Path) expectedPath := filepath.Join(f.Dir(), strings.Replace(f.Base(), find, replace, -1)) defer os.Remove(expectedPath) @@ -211,7 +215,7 @@ func TestHandleFileWithIgnoredDir(t *testing.T) { dirPath := filepath.Join(os.TempDir(), initial) if err := os.Mkdir(dirPath, 0700); err != nil { - log.Fatal(err) + t.Fatalf("Mkdir(%q): %v", dirPath, err) } f := NewFile(dirPath) defer os.Remove(f.Path) @@ -233,7 +237,7 @@ func TestHandleFileWithFile(t *testing.T) { replace := "f" want := "alfa" - f := newTestFile("", initial, initial) + f := newTestFile(t, "", initial, initial) defer os.Remove(f.Path) expectedName := strings.Replace(f.Base(), find, replace, -1) expectedPath := filepath.Join(f.Dir(), expectedName) @@ -255,7 +259,7 @@ func TestRenameFile(t *testing.T) { find := "ph" replace := "f" - f := newTestFile("", initial, "") + f := newTestFile(t, "", initial, "") defer os.Remove(f.Path) expectedName := strings.Replace(f.Base(), find, replace, -1) expectedPath := filepath.Join(f.Dir(), expectedName) @@ -270,6 +274,7 @@ func TestRenameFile(t *testing.T) { // assertNewContentsOfFile ensures that the contents of the file at the given // path exactly match the desired string. func assertNewContentsOfFile(t *testing.T, path string, initial string, find string, replace string, want string) { + t.Helper() got := NewFile(path).Read() if got != want { t.Errorf("replace %v with %v in %v, but got %v; want %v", find, replace, initial, got, want) @@ -282,7 +287,7 @@ func TestReplaceContents(t *testing.T) { replace := "f" want := "alfa" - f := newTestFile("", "*", initial) + f := newTestFile(t, "", "*", initial) defer os.Remove(f.Path) fr := findReplace{find: find, replace: replace} fr.ReplaceContents(f) @@ -295,7 +300,7 @@ func TestReplaceContentsEntireFile(t *testing.T) { replace := "beta" want := "beta" - f := newTestFile("", "*", initial) + f := newTestFile(t, "", "*", initial) defer os.Remove(f.Path) fr := findReplace{find: find, replace: replace} fr.ReplaceContents(f) @@ -308,7 +313,7 @@ func TestReplaceContentsMultipleMatchesSingleLine(t *testing.T) { replace := "f" want := "alfaalfa" - f := newTestFile("", "*", initial) + f := newTestFile(t, "", "*", initial) defer os.Remove(f.Path) fr := findReplace{find: find, replace: replace} fr.ReplaceContents(f) @@ -321,7 +326,7 @@ func TestReplaceContentsMultipleMatchesMultipleLines(t *testing.T) { replace := "f" want := "alfa\nalfa" - f := newTestFile("", "*", initial) + f := newTestFile(t, "", "*", initial) defer os.Remove(f.Path) fr := findReplace{find: find, replace: replace} fr.ReplaceContents(f) @@ -334,7 +339,7 @@ func TestReplaceContentsNoMatches(t *testing.T) { replace := "xyz" want := "alpha" - f := newTestFile("", "*", initial) + f := newTestFile(t, "", "*", initial) defer os.Remove(f.Path) fr := findReplace{find: find, replace: replace} fr.ReplaceContents(f) @@ -342,7 +347,8 @@ func TestReplaceContentsNoMatches(t *testing.T) { } func CloneRepoToTestDir(b *testing.B, repoUrl string) *File { - d := newTestDir("", "*") + b.Helper() + d := newTestDir(b, "", "*") defer os.Remove(d.Path) cmd := exec.Command("git", "clone", "--depth=1", "--single-branch", repoUrl, ".") diff --git a/strings_test.go b/strings_test.go index e5d5d55..408c03b 100644 --- a/strings_test.go +++ b/strings_test.go @@ -7,6 +7,7 @@ import ( // assertRandomStringLength ensures that the generated string matches the // desired length. func assertRandomStringLength(t *testing.T, ask int, want int) { + t.Helper() got := len(RandomString(ask)) if got != want { t.Errorf("len(RandomString(%v)) = %v; want %v", ask, got, want)