From 1b4f1dc95d221c1e9d0afb9067fd6a09f12dd061 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 11 Jun 2024 17:25:06 -0700 Subject: [PATCH] os: improve newFile, rm newDir 1. Assuming that CI environments do not use NFS (and if they do, they have TMPDIR set pointing to a local file system), we can - remove localTmp; - remove newDir, replacing calls to it with t.TempDir; - remove repeated comments about NFS. 2. Use t.Name, t.Cleanup and t.Helper to improve newFile and simplify its usage. Ensure the cleanup reports all errors. Change-Id: I0a79a6a3d52faa323ed2658ef73f8802847f3c09 Reviewed-on: https://go-review.googlesource.com/c/go/+/592096 Reviewed-by: Ian Lance Taylor TryBot-Result: Gopher Robot Run-TryBot: Kirill Kolyshkin Reviewed-by: David Chase Auto-Submit: Ian Lance Taylor LUCI-TryBot-Result: Go LUCI --- src/os/os_test.go | 101 +++++++++++------------------------------ src/os/os_unix_test.go | 30 ++---------- 2 files changed, 32 insertions(+), 99 deletions(-) diff --git a/src/os/os_test.go b/src/os/os_test.go index 7348a9f01c..2a6b1bf9f5 100644 --- a/src/os/os_test.go +++ b/src/os/os_test.go @@ -158,28 +158,20 @@ func equal(name1, name2 string) (r bool) { return } -// localTmp returns a local temporary directory not on NFS. -func localTmp() string { - switch runtime.GOOS { - case "android", "ios", "windows": - return TempDir() - } - return "/tmp" -} - -func newFile(testName string, t *testing.T) (f *File) { - f, err := CreateTemp(localTmp(), "_Go_"+testName) - if err != nil { - t.Fatalf("TempFile %s: %s", testName, err) - } - return -} - -func newDir(testName string, t *testing.T) (name string) { - name, err := MkdirTemp(localTmp(), "_Go_"+testName) +func newFile(t *testing.T) (f *File) { + t.Helper() + f, err := CreateTemp("", "_Go_"+t.Name()) if err != nil { - t.Fatalf("TempDir %s: %s", testName, err) + t.Fatal(err) } + t.Cleanup(func() { + if err := f.Close(); err != nil && !errors.Is(err, ErrClosed) { + t.Fatal(err) + } + if err := Remove(f.Name()); err != nil { + t.Fatal(err) + } + }) return } @@ -1276,9 +1268,7 @@ func TestChmod(t *testing.T) { } t.Parallel() - f := newFile("TestChmod", t) - defer Remove(f.Name()) - defer f.Close() + f := newFile(t) // Creation mode is read write fm := FileMode(0456) @@ -1314,9 +1304,7 @@ func checkSize(t *testing.T, f *File, size int64) { func TestFTruncate(t *testing.T) { t.Parallel() - f := newFile("TestFTruncate", t) - defer Remove(f.Name()) - defer f.Close() + f := newFile(t) checkSize(t, f, 0) f.Write([]byte("hello, world\n")) @@ -1336,9 +1324,7 @@ func TestFTruncate(t *testing.T) { func TestTruncate(t *testing.T) { t.Parallel() - f := newFile("TestTruncate", t) - defer Remove(f.Name()) - defer f.Close() + f := newFile(t) checkSize(t, f, 0) f.Write([]byte("hello, world\n")) @@ -1375,15 +1361,10 @@ func TestTruncateNonexistentFile(t *testing.T) { assertPathError(t, path, err) } -// Use TempDir (via newFile) to make sure we're on a local file system, -// so that timings are not distorted by latency and caching. -// On NFS, timings can be off due to caching of meta-data on -// NFS servers (Issue 848). func TestChtimes(t *testing.T) { t.Parallel() - f := newFile("TestChtimes", t) - defer Remove(f.Name()) + f := newFile(t) f.Write([]byte("hello, world\n")) f.Close() @@ -1392,13 +1373,12 @@ func TestChtimes(t *testing.T) { } func TestChtimesWithZeroTimes(t *testing.T) { - file := newFile("chtimes-with-zero", t) + file := newFile(t) _, err := file.Write([]byte("hello, world\n")) if err != nil { t.Fatalf("Write: %s", err) } fName := file.Name() - defer Remove(file.Name()) err = file.Close() if err != nil { t.Errorf("%v", err) @@ -1513,17 +1493,10 @@ func TestChtimesWithZeroTimes(t *testing.T) { } } -// Use TempDir (via newDir) to make sure we're on a local file system, -// so that timings are not distorted by latency and caching. -// On NFS, timings can be off due to caching of meta-data on -// NFS servers (Issue 848). func TestChtimesDir(t *testing.T) { t.Parallel() - name := newDir("TestChtimes", t) - defer RemoveAll(name) - - testChtimes(t, name) + testChtimes(t, t.TempDir()) } func testChtimes(t *testing.T, name string) { @@ -1574,9 +1547,8 @@ func testChtimes(t *testing.T, name string) { } func TestChtimesToUnixZero(t *testing.T) { - file := newFile("chtimes-to-unix-zero", t) + file := newFile(t) fn := file.Name() - defer Remove(fn) if _, err := file.Write([]byte("hi")); err != nil { t.Fatal(err) } @@ -1796,9 +1768,7 @@ func TestProgWideChdir(t *testing.T) { func TestSeek(t *testing.T) { t.Parallel() - f := newFile("TestSeek", t) - defer Remove(f.Name()) - defer f.Close() + f := newFile(t) const data = "hello, world\n" io.WriteString(f, data) @@ -2040,9 +2010,7 @@ func TestHostname(t *testing.T) { func TestReadAt(t *testing.T) { t.Parallel() - f := newFile("TestReadAt", t) - defer Remove(f.Name()) - defer f.Close() + f := newFile(t) const data = "hello, world\n" io.WriteString(f, data) @@ -2064,9 +2032,7 @@ func TestReadAt(t *testing.T) { func TestReadAtOffset(t *testing.T) { t.Parallel() - f := newFile("TestReadAtOffset", t) - defer Remove(f.Name()) - defer f.Close() + f := newFile(t) const data = "hello, world\n" io.WriteString(f, data) @@ -2095,9 +2061,7 @@ func TestReadAtOffset(t *testing.T) { func TestReadAtNegativeOffset(t *testing.T) { t.Parallel() - f := newFile("TestReadAtNegativeOffset", t) - defer Remove(f.Name()) - defer f.Close() + f := newFile(t) const data = "hello, world\n" io.WriteString(f, data) @@ -2116,9 +2080,7 @@ func TestReadAtNegativeOffset(t *testing.T) { func TestWriteAt(t *testing.T) { t.Parallel() - f := newFile("TestWriteAt", t) - defer Remove(f.Name()) - defer f.Close() + f := newFile(t) const data = "hello, world\n" io.WriteString(f, data) @@ -2141,9 +2103,7 @@ func TestWriteAt(t *testing.T) { func TestWriteAtNegativeOffset(t *testing.T) { t.Parallel() - f := newFile("TestWriteAtNegativeOffset", t) - defer Remove(f.Name()) - defer f.Close() + f := newFile(t) n, err := f.WriteAt([]byte("WORLD"), -10) @@ -2477,9 +2437,7 @@ func TestStatRelativeSymlink(t *testing.T) { func TestReadAtEOF(t *testing.T) { t.Parallel() - f := newFile("TestReadAtEOF", t) - defer Remove(f.Name()) - defer f.Close() + f := newFile(t) _, err := f.ReadAt(make([]byte, 10), 0) switch err { @@ -2495,12 +2453,7 @@ func TestReadAtEOF(t *testing.T) { func TestLongPath(t *testing.T) { t.Parallel() - tmpdir := newDir("TestLongPath", t) - defer func(d string) { - if err := RemoveAll(d); err != nil { - t.Fatalf("RemoveAll failed: %v", err) - } - }(tmpdir) + tmpdir := t.TempDir() // Test the boundary of 247 and fewer bytes (normal) and 248 and more bytes (adjusted). sizes := []int{247, 248, 249, 400} diff --git a/src/os/os_unix_test.go b/src/os/os_unix_test.go index 98e436fae6..6cbeae1b78 100644 --- a/src/os/os_unix_test.go +++ b/src/os/os_unix_test.go @@ -45,13 +45,7 @@ func TestChown(t *testing.T) { } t.Parallel() - // Use TempDir() to make sure we're on a local file system, - // so that the group ids returned by Getgroups will be allowed - // on the file. On NFS, the Getgroups groups are - // basically useless. - f := newFile("TestChown", t) - defer Remove(f.Name()) - defer f.Close() + f := newFile(t) dir, err := f.Stat() if err != nil { t.Fatalf("stat %s: %s", f.Name(), err) @@ -99,13 +93,7 @@ func TestFileChown(t *testing.T) { } t.Parallel() - // Use TempDir() to make sure we're on a local file system, - // so that the group ids returned by Getgroups will be allowed - // on the file. On NFS, the Getgroups groups are - // basically useless. - f := newFile("TestFileChown", t) - defer Remove(f.Name()) - defer f.Close() + f := newFile(t) dir, err := f.Stat() if err != nil { t.Fatalf("stat %s: %s", f.Name(), err) @@ -151,13 +139,7 @@ func TestLchown(t *testing.T) { testenv.MustHaveSymlink(t) t.Parallel() - // Use TempDir() to make sure we're on a local file system, - // so that the group ids returned by Getgroups will be allowed - // on the file. On NFS, the Getgroups groups are - // basically useless. - f := newFile("TestLchown", t) - defer Remove(f.Name()) - defer f.Close() + f := newFile(t) dir, err := f.Stat() if err != nil { t.Fatalf("stat %s: %s", f.Name(), err) @@ -223,8 +205,7 @@ func TestReaddirRemoveRace(t *testing.T) { } return oldStat(name) } - dir := newDir("TestReaddirRemoveRace", t) - defer RemoveAll(dir) + dir := t.TempDir() if err := WriteFile(filepath.Join(dir, "some-file"), []byte("hello"), 0644); err != nil { t.Fatal(err) } @@ -255,8 +236,7 @@ func TestMkdirStickyUmask(t *testing.T) { t.Parallel() const umask = 0077 - dir := newDir("TestMkdirStickyUmask", t) - defer RemoveAll(dir) + dir := t.TempDir() oldUmask := syscall.Umask(umask) defer syscall.Umask(oldUmask) -- 2.48.1