all: replace `os.MkdirTemp` with T.TempDir

[perillo]

Following #45182 I propose to change all the calls to os.MkdirTemp in tests with T.TempDir.
The os.MkdirTemp is used 145 times in tests.

Currently os.MkdirTemp is not used consistently:

• some tests use t.Name() as name
• some tests use the test name as a literal string as name
• some tests use a short name, or the test name with lowercase or inverted
  (as an example in os_test.go: TestDirentRepeat -> direntRepeat-test)

In the tests for the os package there is a support function:

func newDir(testName string, t *testing.T) (name string) {
	name, err := os.MkdirTemp(localTmp(), "_Go_"+testName)
	if err != nil {
		t.Fatalf("TempDir %s: %s", testName, err)
	}
	return
}

This function can be probably removed.

One possible issue is mixing the use of defer and T.Cleanup, as an example in case of chdir to a temporary directory, resulting in an incorrect cleanup order; one example is https://golang.org/cl/307189 but there seems to be other possible cases.

I would like to submit the change, but a lot of packages are involved; how should I proceed?
Probably I should start with the std module followed by cmd and misc (?).
Should I submit one change for package instead of one change for module?

Thanks.

[perillo]

The newDir function calls localTmp:

// localTmp returns a local temporary directory not on NFS.
func localTmp() string {
	switch runtime.GOOS {
	case "android", "windows":
		return TempDir()
	case "darwin", "ios":
		switch runtime.GOARCH {
		case "arm64":
			return TempDir()
		}
	}
	return "/tmp"
}

Does this really ensures that the returned directory is really not on a remote filesystem?
The only difference compared to os.TempDir is that on Unix systems (excluding darwin and android) it ignores TMPDIR.

[davecheney]

The os.MkdirTemp is used 145 times in tests.

How many packages does this encompass? My feeling is, to avoid a protected review process, this is best addressed, at least until a pattern/patterns are established, on a package by package basis.

[perillo]

There are about 18 packages in std and about 15 packages in cmd.

Here is a list of all test files; in the list is included a file in testdata.

std

archive/tar/tar_test.go
crypto/x509/root_unix_test.go
debug/gosym/pclntab_test.go
debug/pe/file_test.go
go/build/build_test.go
go/internal/gccgoimporter/importer_test.go
go/internal/gcimporter/gcimporter_test.go
html/template/examplefiles_test.go
net/dnsclient_unix_test.go
net/http/filetransport_test.go
net/http/fs_test.go
os/error_test.go
os/example_test.go
os/exec/lp_unix_test.go
os/exec/lp_windows_test.go
os/fifo_test.go
os/os_test.go
os/os_windows_test.go
os/path_test.go
os/path_windows_test.go
os/removeall_test.go
os/signal/signal_windows_test.go
os/stat_test.go
path/filepath/example_unix_walk_test.go
path/filepath/match_test.go
path/filepath/path_test.go
path/filepath/path_windows_test.go
runtime/crash_test.go
runtime/crash_unix_test.go
runtime/race/output_test.go
runtime/race/testdata/io_test.go
runtime/runtime-gdb_test.go
runtime/runtime-lldb_test.go
runtime/signal_windows_test.go
runtime/syscall_windows_test.go
syscall/dirent_test.go
syscall/exec_linux_test.go
syscall/getdirentries_test.go
syscall/syscall_linux_test.go
syscall/syscall_unix_test.go
syscall/syscall_windows_test.go
text/template/examplefiles_test.go
text/template/link_test.go

cmd

cmd/addr2line/addr2line_test.go
cmd/cover/cover_test.go
cmd/go/go_test.go
cmd/go/go_windows_test.go
cmd/go/internal/cache/cache_test.go
cmd/go/internal/lockedfile/lockedfile_test.go
cmd/go/internal/modconv/convert_test.go
cmd/go/internal/modfetch/cache_test.go
cmd/go/internal/modfetch/codehost/git_test.go
cmd/go/internal/modfetch/coderepo_test.go
cmd/go/internal/modfetch/zip_sum_test/zip_sum_test.go
cmd/go/internal/modload/query_test.go
cmd/go/internal/vcs/vcs_test.go
cmd/go/internal/work/build_test.go
cmd/go/script_test.go
cmd/gofmt/gofmt_test.go
cmd/nm/nm_test.go
cmd/objdump/objdump_test.go
cmd/pack/pack_test.go
cmd/vet/vet_test.go

[perillo]

Another possible issue when using T.TempDir instead of os.MkdirTemp + defer is the case when a temporary directory is created in a sub test.

Currently it seems that there are no cases of os.MkdirTemp being called in a sub test; I verified it using
grep -F -l "os.MkdirTemp" **/*_test.go | xargs grep -F "T.Run".

However there may be cases in future, that should be handled and documented correctly.

[perillo]

Thinking more carefully, there should be no issues with calling T.TempDir in a sub test.

[davecheney]

I think path/filepath would be a good, self contained, candidate to work on. Please be mindful that the change freeze occurs at the end of the month so if you want to work on this for Go 1.17, please do not delay.

[gopherbot]

Change https://golang.org/cl/307653 mentions this issue: net/http: replace os.MkdirTemp with T.TempDir

[ianwoolf]

The usage of os.MkdirTemp in some test function is similar to the following example.

func TestLookPathUnixEmptyPath(t *testing.T) {
	tmp, err := os.MkdirTemp("", "TestLookPathUnixEmptyPath")
	if err != nil {
		t.Fatal("TempDir failed: ", err)
	}
	defer os.RemoveAll(tmp)
	wd, err := os.Getwd()
	if err != nil {
		t.Fatal("Getwd failed: ", err)
	}
	err = os.Chdir(tmp)
	if err != nil {
		t.Fatal("Chdir failed: ", err)
	}
	defer os.Chdir(wd)
    .....
}

It would be better to replace it with the function ChTmpDir to be added in #45182

[perillo]

The chtmpdir can be used when the test does not actually need to know the path to the temporary directory. It is better, for now, to not replace os.MkdirTemp with T.TempDir for tests that need to chdir to a temporary directory and do not follow this pattern.

[gopherbot]

Change https://golang.org/cl/307989 mentions this issue: runtime, syscall, text: replace os.MkdirTemp with T.TempDir

[gopherbot]

Change https://golang.org/cl/307990 mentions this issue: syscall: replace os.MkdirTemp with T.TempDir

[gopherbot]

Change https://golang.org/cl/308011 mentions this issue: path/filepath: replace os.MkdirTemp with T.TempDir