From 1c2bf2bfebe0788f902f86947ade5aea5b92ceb8 Mon Sep 17 00:00:00 2001 From: Brandon Dyck Date: Tue, 27 Aug 2024 12:54:11 -0600 Subject: [PATCH 1/7] util: clarify TempFile custom path behavior --- util/tempfile.go | 18 ++++++++++-------- util/tempfile_test.go | 8 ++++---- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/util/tempfile.go b/util/tempfile.go index de40da0..53a03ad 100644 --- a/util/tempfile.go +++ b/util/tempfile.go @@ -20,7 +20,7 @@ type TempFileSettings struct { data []byte mode *fs.FileMode namePattern string - path *string + dir *string } type TempFileSetting func(s *TempFileSettings) @@ -55,12 +55,14 @@ func ByteData(data []byte) TempFileSetting { } } -// Path specifies a directory path to contain the temporary file. +// Dir specifies a directory path to contain the temporary file. +// If dir is the empty string, the file will be created in the +// default directory for temporary files, as returned by os.TempDir. // A temporary file created in a custom directory will still be deleted // after the test runs, though the directory may not. -func Path(path string) TempFileSetting { +func Dir(dir string) TempFileSetting { return func(s *TempFileSettings) { - s.path = &path + s.dir = &dir } } @@ -80,9 +82,9 @@ func TempFile(t T, settings ...TempFileSetting) (path string) { allSettings.mode = new(fs.FileMode) *allSettings.mode = 0600 } - if allSettings.path == nil { - allSettings.path = new(string) - *allSettings.path = t.TempDir() + if allSettings.dir == nil { + allSettings.dir = new(string) + *allSettings.dir = t.TempDir() } var err error @@ -90,7 +92,7 @@ func TempFile(t T, settings ...TempFileSetting) (path string) { t.Helper() t.Fatalf("%s: %v", "TempFile", err) } - file, err := os.CreateTemp(*allSettings.path, allSettings.namePattern) + file, err := os.CreateTemp(*allSettings.dir, allSettings.namePattern) if err != nil { crash(t) } diff --git a/util/tempfile_test.go b/util/tempfile_test.go index 0a5df02..291dda7 100644 --- a/util/tempfile_test.go +++ b/util/tempfile_test.go @@ -108,17 +108,17 @@ func TestTempFile(t *testing.T) { }) t.Run("file is deleted after test", func(t *testing.T) { - dirpath := t.TempDir() + dir := t.TempDir() var path string t.Run("uses custom path", func(t *testing.T) { - path = util.TempFile(t, util.Path(dirpath)) - entries, err := os.ReadDir(dirpath) + path = util.TempFile(t, util.Dir(dir)) + entries, err := os.ReadDir(dir) if err != nil { t.Fatalf("failed to read directory: %v", err) } if len(entries) == 0 || entries[0].Name() != filepath.Base(path) { - t.Fatalf("did not find temporary file in %s", dirpath) + t.Fatalf("did not find temporary file in %s", dir) } }) From ed5ac20edadc0a04e8473e1ed884faf5b013ea54 Mon Sep 17 00:00:00 2001 From: Brandon Dyck Date: Tue, 27 Aug 2024 14:54:18 -0600 Subject: [PATCH 2/7] util: improve TempFile tests --- util/tempfile.go | 46 +++++++----- util/tempfile_test.go | 161 ++++++++++++++++++++++++++++++++---------- 2 files changed, 154 insertions(+), 53 deletions(-) diff --git a/util/tempfile.go b/util/tempfile.go index 53a03ad..63f2600 100644 --- a/util/tempfile.go +++ b/util/tempfile.go @@ -4,6 +4,8 @@ package util import ( + "errors" + "fmt" "io/fs" "os" ) @@ -74,6 +76,23 @@ func Dir(dir string) TempFileSetting { // after the test is completed. func TempFile(t T, settings ...TempFileSetting) (path string) { t.Helper() + path, err := tempFile(t.Helper, t.TempDir, settings...) + t.Cleanup(func() { + err := os.Remove(path) + if err != nil { + t.Fatalf("failed to clean up temp file: %s", path) + } + }) + if err != nil { + t.Fatalf("%v", err) + } + return path +} + +// tempFile returns errors instead of relying upon T to stop execution, for ease +// of testing TempFile. +func tempFile(helper func(), tempDir func() string, settings ...TempFileSetting) (path string, err error) { + helper() var allSettings TempFileSettings for _, setting := range settings { setting(&allSettings) @@ -84,37 +103,32 @@ func TempFile(t T, settings ...TempFileSetting) (path string) { } if allSettings.dir == nil { allSettings.dir = new(string) - *allSettings.dir = t.TempDir() + *allSettings.dir = tempDir() } - var err error - crash := func(t T) { - t.Helper() - t.Fatalf("%s: %v", "TempFile", err) + wrap := func(err error) error { + return fmt.Errorf("TempFile: %w", err) } file, err := os.CreateTemp(*allSettings.dir, allSettings.namePattern) + if errors.Is(err, fs.ErrNotExist) { + return "", fmt.Errorf("TempFile: directory does not exist") + } if err != nil { - crash(t) + return "", wrap(err) } path = file.Name() - t.Cleanup(func() { - err := os.Remove(path) - if err != nil { - t.Fatalf("failed to clean up temp file: %s", path) - } - }) _, err = file.Write(allSettings.data) if err != nil { file.Close() - crash(t) + return path, wrap(err) } err = file.Close() if err != nil { - crash(t) + return path, wrap(err) } err = os.Chmod(path, *allSettings.mode) if err != nil { - crash(t) + return path, wrap(err) } - return file.Name() + return file.Name(), nil } diff --git a/util/tempfile_test.go b/util/tempfile_test.go index 291dda7..b859898 100644 --- a/util/tempfile_test.go +++ b/util/tempfile_test.go @@ -5,9 +5,12 @@ package util_test import ( "bytes" + "errors" + "fmt" "io/fs" "os" "path/filepath" + "slices" "strings" "testing" @@ -48,6 +51,53 @@ func (t *helperTracker) Cleanup(f func()) { t.t.Cleanup(f) } +func trackFailure(t util.T) *failureTracker { + return &failureTracker{t: t} +} + +type failureTracker struct { + failed bool + log bytes.Buffer + t util.T +} + +func (t *failureTracker) TempDir() string { + t.t.Helper() + return t.t.TempDir() +} + +func (t *failureTracker) Helper() { + t.t.Helper() +} + +func (t *failureTracker) Errorf(s string, args ...any) { + t.t.Helper() + t.failed = true + fmt.Fprintf(&t.log, s+"\n", args...) +} + +func (t *failureTracker) Fatalf(s string, args ...any) { + t.t.Helper() + t.failed = true + fmt.Fprintf(&t.log, s+"\n", args...) +} + +func (t *failureTracker) Cleanup(f func()) { + t.t.Helper() + t.t.Cleanup(f) +} + +func (t *failureTracker) AssertFailedWith(msg string) { + t.t.Helper() + if !t.failed { + t.t.Fatalf("expected test to fail with message %q", msg) + } + strlog := t.log.String() + if !strings.Contains(strlog, msg) { + t.t.Fatalf("expected test to fail with message %q\ngot message %q", msg, strlog) + } +} + func TestTempFile(t *testing.T) { t.Run("creates a read/write temp file by default", func(t *testing.T) { th := trackHelper(t) @@ -64,38 +114,44 @@ func TestTempFile(t *testing.T) { t.Fatalf("expected at least u+rw permission, got %03o", mode) } }) - t.Run("sets a custom file mode", func(t *testing.T) { + + t.Run("using multiple options", func(t *testing.T) { var expectedMode fs.FileMode = 0444 - path := util.TempFile(t, util.Mode(expectedMode)) - info, err := os.Stat(path) - if err != nil { - t.Fatalf("failed to stat temp file: %v", err) - } - actualMode := info.Mode() - if expectedMode != actualMode { - t.Fatalf("file has wrong mode\nexpected %03o\ngot %03o", expectedMode, actualMode) - } - }) - t.Run("sets a name pattern", func(t *testing.T) { + expectedData := "important data" prefix := "harvey-" pattern := prefix + "*" - path := util.TempFile(t, util.Pattern(pattern)) - if !strings.Contains(path, prefix) { - t.Fatalf("filename does not match pattern\nexpected to contain %s\ngot %s", prefix, path) - } - }) - t.Run("sets string data", func(t *testing.T) { - expectedData := "important data" - path := util.TempFile(t, util.StringData(expectedData)) - actualData, err := os.ReadFile(path) - if err != nil { - t.Fatalf("failed to read temp file: %v", err) - } - if expectedData != string(actualData) { - t.Fatalf("temp file contains wrong data\nexpected %q\ngot %q", expectedData, string(actualData)) - } + path := util.TempFile(t, + util.Mode(expectedMode), + util.Pattern(pattern), + util.StringData(expectedData)) + + t.Run("Mode sets a custom file mode", func(t *testing.T) { + info, err := os.Stat(path) + if err != nil { + t.Fatalf("failed to stat temp file: %v", err) + } + actualMode := info.Mode() + if expectedMode != actualMode { + t.Fatalf("file has wrong mode\nexpected %03o\ngot %03o", expectedMode, actualMode) + } + }) + t.Run("sets a name pattern", func(t *testing.T) { + if !strings.Contains(path, prefix) { + t.Fatalf("filename does not match pattern\nexpected to contain %s\ngot %s", prefix, path) + } + }) + t.Run("sets string data", func(t *testing.T) { + actualData, err := os.ReadFile(path) + if err != nil { + t.Fatalf("failed to read temp file: %v", err) + } + if expectedData != string(actualData) { + t.Fatalf("temp file contains wrong data\nexpected %q\ngot %q", expectedData, string(actualData)) + } + }) }) - t.Run("sets binary data", func(t *testing.T) { + + t.Run("ByteData sets binary data", func(t *testing.T) { expectedData := []byte("important data") path := util.TempFile(t, util.ByteData(expectedData)) actualData, err := os.ReadFile(path) @@ -107,27 +163,58 @@ func TestTempFile(t *testing.T) { } }) - t.Run("file is deleted after test", func(t *testing.T) { + t.Run("cleans up file (and nothing else) in custom dir", func(t *testing.T) { dir := t.TempDir() - var path string + existing, err := os.CreateTemp(dir, "") + if err != nil { + t.Fatalf("failed to create temporary file: %v", err) + } + existingPath := existing.Name() + existing.Close() + var newPath string - t.Run("uses custom path", func(t *testing.T) { - path = util.TempFile(t, util.Dir(dir)) + t.Run("uses custom directory", func(t *testing.T) { + newPath = util.TempFile(t, util.Dir(dir)) entries, err := os.ReadDir(dir) if err != nil { t.Fatalf("failed to read directory: %v", err) } - if len(entries) == 0 || entries[0].Name() != filepath.Base(path) { + found := slices.ContainsFunc(entries, func(entry fs.DirEntry) bool { + return entry.Name() == filepath.Base(newPath) + }) + if !found { t.Fatalf("did not find temporary file in %s", dir) } }) - if path == "" { + if newPath == "" { t.Fatal("expected non-empty path") } - _, err := os.Stat(path) - if err == nil { - t.Fatalf("expected temp file not to exist: %s", path) + _, err = os.Stat(newPath) + if !errors.Is(err, fs.ErrNotExist) { + if err == nil { + t.Errorf("expected temp file not to exist: %s", newPath) + } else { + t.Errorf("unexpected error: %v", err) + } + } + _, err = os.Stat(existingPath) + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + t.Error("expected pre-existing file not to be deleted") + } else { + t.Errorf("unexpected error statting pre-existing file: %v", err) + } + } + }) + + t.Run("fails if specified dir doesn't exist", func(t *testing.T) { + fakeDir := filepath.Join(t.TempDir(), "fake") + tracker := trackFailure(t) + path := util.TempFile(tracker, util.Dir(fakeDir)) + if path != "" { + t.Errorf("expected empty path\ngot %q", path) } + tracker.AssertFailedWith("TempFile: directory does not exist") }) } From ab7a42c87123890fc4cb28098ae3c3e7b4a7a0d9 Mon Sep 17 00:00:00 2001 From: Brandon Dyck Date: Tue, 27 Aug 2024 15:48:18 -0600 Subject: [PATCH 3/7] util: tests can't use Go 1.21 features --- util/tempfile_test.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/util/tempfile_test.go b/util/tempfile_test.go index b859898..9c93702 100644 --- a/util/tempfile_test.go +++ b/util/tempfile_test.go @@ -10,7 +10,6 @@ import ( "io/fs" "os" "path/filepath" - "slices" "strings" "testing" @@ -179,9 +178,12 @@ func TestTempFile(t *testing.T) { if err != nil { t.Fatalf("failed to read directory: %v", err) } - found := slices.ContainsFunc(entries, func(entry fs.DirEntry) bool { - return entry.Name() == filepath.Base(newPath) - }) + var found bool + for _, entry := range entries { + if entry.Name() == filepath.Base(newPath) { + found = true + } + } if !found { t.Fatalf("did not find temporary file in %s", dir) } From 7a5fb065aa378b109e6b9a8e4ca5ee14230d1da0 Mon Sep 17 00:00:00 2001 From: Brandon Dyck Date: Tue, 27 Aug 2024 18:19:08 -0600 Subject: [PATCH 4/7] util: move error wrapping out of tempFile --- util/tempfile.go | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/util/tempfile.go b/util/tempfile.go index 63f2600..21d40d5 100644 --- a/util/tempfile.go +++ b/util/tempfile.go @@ -5,7 +5,6 @@ package util import ( "errors" - "fmt" "io/fs" "os" ) @@ -84,7 +83,7 @@ func TempFile(t T, settings ...TempFileSetting) (path string) { } }) if err != nil { - t.Fatalf("%v", err) + t.Fatalf("TempFile: %v", err) } return path } @@ -106,29 +105,26 @@ func tempFile(helper func(), tempDir func() string, settings ...TempFileSetting) *allSettings.dir = tempDir() } - wrap := func(err error) error { - return fmt.Errorf("TempFile: %w", err) - } file, err := os.CreateTemp(*allSettings.dir, allSettings.namePattern) if errors.Is(err, fs.ErrNotExist) { - return "", fmt.Errorf("TempFile: directory does not exist") + return "", errors.New("directory does not exist") } if err != nil { - return "", wrap(err) + return "", err } path = file.Name() _, err = file.Write(allSettings.data) if err != nil { file.Close() - return path, wrap(err) + return path, err } err = file.Close() if err != nil { - return path, wrap(err) + return path, err } err = os.Chmod(path, *allSettings.mode) if err != nil { - return path, wrap(err) + return path, err } return file.Name(), nil } From decbe12848dbf908bda004d1ee167027fbf1a233 Mon Sep 17 00:00:00 2001 From: Brandon Dyck Date: Tue, 27 Aug 2024 18:28:24 -0600 Subject: [PATCH 5/7] util: pass an interface to tempFile --- util/tempfile.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/util/tempfile.go b/util/tempfile.go index 21d40d5..3b276b3 100644 --- a/util/tempfile.go +++ b/util/tempfile.go @@ -75,7 +75,7 @@ func Dir(dir string) TempFileSetting { // after the test is completed. func TempFile(t T, settings ...TempFileSetting) (path string) { t.Helper() - path, err := tempFile(t.Helper, t.TempDir, settings...) + path, err := tempFile(t, settings...) t.Cleanup(func() { err := os.Remove(path) if err != nil { @@ -88,10 +88,15 @@ func TempFile(t T, settings ...TempFileSetting) (path string) { return path } +type tempFileT interface { + Helper() + TempDir() string +} + // tempFile returns errors instead of relying upon T to stop execution, for ease // of testing TempFile. -func tempFile(helper func(), tempDir func() string, settings ...TempFileSetting) (path string, err error) { - helper() +func tempFile(t tempFileT, settings ...TempFileSetting) (path string, err error) { + t.Helper() var allSettings TempFileSettings for _, setting := range settings { setting(&allSettings) @@ -102,7 +107,7 @@ func tempFile(helper func(), tempDir func() string, settings ...TempFileSetting) } if allSettings.dir == nil { allSettings.dir = new(string) - *allSettings.dir = tempDir() + *allSettings.dir = t.TempDir() } file, err := os.CreateTemp(*allSettings.dir, allSettings.namePattern) From ecaf909ba0682166fadb747902a829b9e4d419eb Mon Sep 17 00:00:00 2001 From: Brandon Dyck Date: Tue, 27 Aug 2024 18:30:24 -0600 Subject: [PATCH 6/7] util: simplify Helper calls --- util/tempfile_test.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/util/tempfile_test.go b/util/tempfile_test.go index 9c93702..7a707ae 100644 --- a/util/tempfile_test.go +++ b/util/tempfile_test.go @@ -26,7 +26,7 @@ type helperTracker struct { } func (t *helperTracker) TempDir() string { - t.t.Helper() + t.Helper() return t.t.TempDir() } @@ -36,17 +36,17 @@ func (t *helperTracker) Helper() { } func (t *helperTracker) Errorf(s string, args ...any) { - t.t.Helper() + t.Helper() t.t.Errorf(s, args) } func (t *helperTracker) Fatalf(s string, args ...any) { - t.t.Helper() + t.Helper() t.t.Fatalf(s, args...) } func (t *helperTracker) Cleanup(f func()) { - t.t.Helper() + t.Helper() t.t.Cleanup(f) } @@ -61,7 +61,7 @@ type failureTracker struct { } func (t *failureTracker) TempDir() string { - t.t.Helper() + t.Helper() return t.t.TempDir() } @@ -70,24 +70,24 @@ func (t *failureTracker) Helper() { } func (t *failureTracker) Errorf(s string, args ...any) { - t.t.Helper() + t.Helper() t.failed = true fmt.Fprintf(&t.log, s+"\n", args...) } func (t *failureTracker) Fatalf(s string, args ...any) { - t.t.Helper() + t.Helper() t.failed = true fmt.Fprintf(&t.log, s+"\n", args...) } func (t *failureTracker) Cleanup(f func()) { - t.t.Helper() + t.Helper() t.t.Cleanup(f) } func (t *failureTracker) AssertFailedWith(msg string) { - t.t.Helper() + t.Helper() if !t.failed { t.t.Fatalf("expected test to fail with message %q", msg) } From 66ae2af2221e1541a5d8578f530dcd1e5a5516f6 Mon Sep 17 00:00:00 2001 From: Brandon Dyck Date: Tue, 27 Aug 2024 18:32:31 -0600 Subject: [PATCH 7/7] util: short-circuit dir entry loop --- util/tempfile_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/util/tempfile_test.go b/util/tempfile_test.go index 7a707ae..63b7f8c 100644 --- a/util/tempfile_test.go +++ b/util/tempfile_test.go @@ -182,6 +182,7 @@ func TestTempFile(t *testing.T) { for _, entry := range entries { if entry.Name() == filepath.Base(newPath) { found = true + break } } if !found {