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

perillo · 2021-04-06T10:16:05Z

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.

The text was updated successfully, but these errors were encountered:

perillo · 2021-04-06T10:37:26Z

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 · 2021-04-06T10:56:12Z

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 · 2021-04-06T12:29:00Z

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 · 2021-04-06T12:50:38Z

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 · 2021-04-06T12:57:19Z

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

davecheney · 2021-04-07T00:26:41Z

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 · 2021-04-07T05:47:46Z

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

Updates: #45402 Change-Id: Ia61f422d058bf57fc3688abc25597d6cc1692c51 Reviewed-on: https://go-review.googlesource.com/c/go/+/307653 Run-TryBot: Dave Cheney <dave@cheney.net> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Dave Cheney <dave@cheney.net> Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>

ianwoolf · 2021-04-07T07:07:23Z

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 · 2021-04-07T09:56:01Z

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 · 2021-04-07T12:18:56Z

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

gopherbot · 2021-04-07T14:36:33Z

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

gopherbot · 2021-04-07T15:00:05Z

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

gopherbot · 2021-04-07T19:29:25Z

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

gopherbot · 2021-04-07T19:59:29Z

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

Updates #45402 Change-Id: I573133d6b987e8ac23e3e2018652612af684c755 Reviewed-on: https://go-review.googlesource.com/c/go/+/307990 Run-TryBot: Ian Lance Taylor <iant@golang.org> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>

Updates #45402 Change-Id: I3aa82fc2486b4de49b45388bbab24f5ffe558f91 Reviewed-on: https://go-review.googlesource.com/c/go/+/307989 Run-TryBot: Ian Lance Taylor <iant@golang.org> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org> Trust: Tobias Klauser <tobias.klauser@gmail.com>

Updates #45402 Change-Id: Idbd8067759d58bc57c52ede4ddccc98ab0ae18fc Reviewed-on: https://go-review.googlesource.com/c/go/+/308129 Run-TryBot: Dave Cheney <dave@cheney.net> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org> Trust: Tobias Klauser <tobias.klauser@gmail.com>

gopherbot · 2021-04-09T01:49:57Z

Change https://golang.org/cl/308409 mentions this issue: crypto/x509: replace os.MkdirTemp with T.TempDir

Updates #45402 Change-Id: Ifb1fa5232a0fa1be62e886643cec9deaa3b312ad Reviewed-on: https://go-review.googlesource.com/c/go/+/308409 Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>

Updates #45402 Change-Id: Ib8e62a13ddff884e4d34b3a0fdc9a10db2b68da6 Reviewed-on: https://go-review.googlesource.com/c/go/+/308109 Run-TryBot: Ian Lance Taylor <iant@golang.org> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>

Add the tempDirCanonical function, for tests that need a temporary directory that does not contain symlinks. Updates #45402 Change-Id: I3d08ef32ef911331544acce3d7d013b4c3382960 Reviewed-on: https://go-review.googlesource.com/c/go/+/308011 Run-TryBot: Ian Lance Taylor <iant@golang.org> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org> Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>

gopherbot · 2021-04-11T19:10:56Z

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

Updates #45402. Change-Id: I6fe356b51bc825a907f979d9c44432a4d43d1f6e Reviewed-on: https://go-review.googlesource.com/c/go/+/308996 Trust: Tobias Klauser <tobias.klauser@gmail.com> Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>

gopherbot · 2021-04-12T20:31:29Z

Change https://golang.org/cl/309351 mentions this issue: text/template: replace os.MkdirTemp with T.TempDir

gopherbot · 2021-04-12T20:50:53Z

Change https://golang.org/cl/309352 mentions this issue: debug/pe: replace os.MkdirTemp with T.TempDir

Updates #45402 Change-Id: I9d55191c4021387b771550b5c93c91806f694aa6 Reviewed-on: https://go-review.googlesource.com/c/go/+/309351 Reviewed-by: Ian Lance Taylor <iant@golang.org> Trust: Tobias Klauser <tobias.klauser@gmail.com>

Updates #45402 Change-Id: I3d83a66270ca38e82d6bb7f8a1367af3d5343a98 Reviewed-on: https://go-review.googlesource.com/c/go/+/309352 Trust: Tobias Klauser <tobias.klauser@gmail.com> Reviewed-by: Ian Lance Taylor <iant@golang.org>

gopherbot · 2021-04-13T10:29:11Z

Change https://golang.org/cl/309354 mentions this issue: go/build: replace os.MkdirTemp with T.TempDir

gopherbot · 2021-04-13T10:42:38Z

Change https://golang.org/cl/309355 mentions this issue: archive/tar: replace os.MkdirTemp with T.TempDir

gopherbot · 2021-04-13T10:53:56Z

Change https://golang.org/cl/309356 mentions this issue: go/internal/gccgoimporter: replace os.MkdirTemp with T.TempDir

Updates #45402 Change-Id: I296f8c676c68ed1e10b6ad1a17b5b23d2c395252 Reviewed-on: https://go-review.googlesource.com/c/go/+/309355 Run-TryBot: Ian Lance Taylor <iant@golang.org> Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com> Reviewed-by: Ian Lance Taylor <iant@golang.org> Trust: Joe Tsai <thebrokentoaster@gmail.com> TryBot-Result: Go Bot <gobot@golang.org>

Updates #45402 Change-Id: Ic2f696837034de17333a6a53127a4bfd301e96a4 Reviewed-on: https://go-review.googlesource.com/c/go/+/309354 Run-TryBot: Ian Lance Taylor <iant@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org> Trust: Tobias Klauser <tobias.klauser@gmail.com>

Updates #45402 Change-Id: Ie84795ed456883c0558fa9b5e3f2186f5f2c0fd9 Reviewed-on: https://go-review.googlesource.com/c/go/+/309356 Run-TryBot: Ian Lance Taylor <iant@golang.org> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org> Trust: Tobias Klauser <tobias.klauser@gmail.com>

gopherbot · 2021-11-08T19:43:02Z

Change https://golang.org/cl/362246 mentions this issue: go/internal/gccgoimporter: use t.TempDir() in tests

Use t.TempDir() instead of os.MkdirTemp(). This is a copy of CL 309356 from the main repo. Updates golang/go#45402. Change-Id: Ia555a701e4de568871a765ee551e65a5c998cde9 Reviewed-on: https://go-review.googlesource.com/c/tools/+/362246 Trust: Than McIntosh <thanm@google.com> Reviewed-by: Bryan C. Mills <bcmills@google.com>

gopherbot · 2023-01-20T17:58:13Z

Change https://go.dev/cl/462896 mentions this issue: cmd/compile: replace os.MkdirTemp with T.TempDir

gopherbot · 2023-02-02T09:28:05Z

Change https://go.dev/cl/464350 mentions this issue: cmd/stringer,internal/lockedfile: replace os.MkdirTemp with T.TempDir

Updates #45402 Change-Id: Ieffd1c8b0b5e4e63024b5be2e1f910fb4411eb94 GitHub-Last-Rev: fa7418c GitHub-Pull-Request: #57940 Reviewed-on: https://go-review.googlesource.com/c/go/+/462896 Reviewed-by: Bryan Mills <bcmills@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Auto-Submit: Keith Randall <khr@golang.org> Reviewed-by: Keith Randall <khr@google.com> Reviewed-by: Daniel Martí <mvdan@mvdan.cc> Reviewed-by: Keith Randall <khr@golang.org> Run-TryBot: Keith Randall <khr@golang.org>

Updates golang#45402 Change-Id: Ieffd1c8b0b5e4e63024b5be2e1f910fb4411eb94 GitHub-Last-Rev: fa7418c GitHub-Pull-Request: golang#57940 Reviewed-on: https://go-review.googlesource.com/c/go/+/462896 Reviewed-by: Bryan Mills <bcmills@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Auto-Submit: Keith Randall <khr@golang.org> Reviewed-by: Keith Randall <khr@google.com> Reviewed-by: Daniel Martí <mvdan@mvdan.cc> Reviewed-by: Keith Randall <khr@golang.org> Run-TryBot: Keith Randall <khr@golang.org>

seankhliao added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Testing An issue that has been verified to require only test changes, not just a test failure. labels Apr 6, 2021

seankhliao added this to the Unplanned milestone Aug 20, 2022

alexandear mentioned this issue Jan 20, 2023

cmd/compile: replace os.MkdirTemp with T.TempDir #57940

Closed

mvdan mentioned this issue Feb 2, 2023

cmd/stringer: streamline test subprocesses golang/tools#425

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

perillo commented Apr 6, 2021

perillo commented Apr 6, 2021

davecheney commented Apr 6, 2021

perillo commented Apr 6, 2021

perillo commented Apr 6, 2021 •

edited

perillo commented Apr 6, 2021 •

edited

davecheney commented Apr 7, 2021

gopherbot commented Apr 7, 2021

ianwoolf commented Apr 7, 2021 •

edited

perillo commented Apr 7, 2021

gopherbot commented Apr 7, 2021

gopherbot commented Apr 7, 2021

gopherbot commented Apr 7, 2021

gopherbot commented Apr 7, 2021

gopherbot commented Apr 7, 2021

gopherbot commented Apr 9, 2021

gopherbot commented Apr 11, 2021

gopherbot commented Apr 12, 2021

gopherbot commented Apr 12, 2021

gopherbot commented Apr 13, 2021

gopherbot commented Apr 13, 2021

gopherbot commented Apr 13, 2021

gopherbot commented Nov 8, 2021

gopherbot commented Jan 20, 2023

gopherbot commented Feb 2, 2023

all: replace os.MkdirTemp with T.TempDir #45402

all: replace os.MkdirTemp with T.TempDir #45402

Comments

perillo commented Apr 6, 2021

perillo commented Apr 6, 2021

davecheney commented Apr 6, 2021

perillo commented Apr 6, 2021

std

cmd

perillo commented Apr 6, 2021 • edited

perillo commented Apr 6, 2021 • edited

davecheney commented Apr 7, 2021

gopherbot commented Apr 7, 2021

ianwoolf commented Apr 7, 2021 • edited

perillo commented Apr 7, 2021

gopherbot commented Apr 7, 2021

gopherbot commented Apr 7, 2021

gopherbot commented Apr 7, 2021

gopherbot commented Apr 7, 2021

gopherbot commented Apr 7, 2021

gopherbot commented Apr 9, 2021

gopherbot commented Apr 11, 2021

gopherbot commented Apr 12, 2021

gopherbot commented Apr 12, 2021

gopherbot commented Apr 13, 2021

gopherbot commented Apr 13, 2021

gopherbot commented Apr 13, 2021

gopherbot commented Nov 8, 2021

gopherbot commented Jan 20, 2023

gopherbot commented Feb 2, 2023

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

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

perillo commented Apr 6, 2021 •

edited

perillo commented Apr 6, 2021 •

edited

ianwoolf commented Apr 7, 2021 •

edited