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 <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Kirill Kolyshkin <kolyshkin@gmail.com>
Reviewed-by: David Chase <drchase@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

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 newFile(t *testing.T) (f *File) {
-func localTmp() string {
+	t.Helper()
-	switch runtime.GOOS {
+	f, err := CreateTemp("", "_Go_"+t.Name())
-	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)
+		t.Fatal(err)
-	}
-	return
-}
-
-func newDir(testName string, t *testing.T) (name string) {
-	name, err := MkdirTemp(localTmp(), "_Go_"+testName)
-	if err != nil {
-		t.Fatalf("TempDir %s: %s", testName, 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)
+	f := newFile(t)
-	defer Remove(f.Name())
-	defer f.Close()
 	// 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)
+	f := newFile(t)
-	defer Remove(f.Name())
-	defer f.Close()
 
 	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)
+	f := newFile(t)
-	defer Remove(f.Name())
-	defer f.Close()
 
 	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)
+	f := newFile(t)
-	defer Remove(f.Name())
 
 	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)
+	testChtimes(t, t.TempDir())
-	defer RemoveAll(name)
-
-	testChtimes(t, name)
 }
 
 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)
+	f := newFile(t)
-	defer Remove(f.Name())
-	defer f.Close()
 
 	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)
+	f := newFile(t)
-	defer Remove(f.Name())
-	defer f.Close()
 
 	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)
+	f := newFile(t)
-	defer Remove(f.Name())
-	defer f.Close()
 
 	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)
+	f := newFile(t)
-	defer Remove(f.Name())
-	defer f.Close()
 
 	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)
+	f := newFile(t)
-	defer Remove(f.Name())
-	defer f.Close()
 
 	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)
+	f := newFile(t)
-	defer Remove(f.Name())
-	defer f.Close()
 
 	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)
+	f := newFile(t)
-	defer Remove(f.Name())
-	defer f.Close()
 
 	_, 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)
+	tmpdir := t.TempDir()
-	defer func(d string) {
-		if err := RemoveAll(d); err != nil {
-			t.Fatalf("RemoveAll failed: %v", err)
-		}
-	}(tmpdir)
 
 	// Test the boundary of 247 and fewer bytes (normal) and 248 and more bytes (adjusted).
 	sizes := []int{247, 248, 249, 400}

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,
+	f := newFile(t)
-	// 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()
 	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,
+	f := newFile(t)
-	// 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()
 	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,
+	f := newFile(t)
-	// 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()
 	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)
+	dir := t.TempDir()
-	defer RemoveAll(dir)
 	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)
+	dir := t.TempDir()
-	defer RemoveAll(dir)
 
 	oldUmask := syscall.Umask(umask)
 	defer syscall.Umask(oldUmask)