all: tests that change the working directory should use defer to restore it

[perillo]

What version of Go are you using (go version)?

$ go version
go version go1.16.2 linux/amd64

I have noted that the tests that need to change the current working directory use the following pattern:

1. call os.Getwd to get the current working directory
2. some code
3. call os.Chdir to change the current working directory
4. some code
5. call os.Chdir to restore the original working directory

An example is:
https://github.com/golang/go/blob/master/src/os/removeall_test.go#L159

The code should probably use defer, using a support function like:

// chdir changes the current working directory to the named directory and
// returns a function that, when called, restores the original working
// directory.
func chdir(t *testing.T, dir string) func() {
	wd, err := os.Getwd()
	if err != nil {
		t.Fatalf("chdir %s: %v", dir, err)
	}
	if err := os.Chdir(dir); err != nil {
		t.Fatal(err)
	}

	return func() {
		if err := os.Chdir(wd); err != nil {
			t.Fatalf("restoring working directory: %v", err)
		}
	}
}

The new pattern is:

1. call defer chdir(dir)()
2. some code

This is more readable and ensures that the working directory is restored in case of test failures.

[ericlagergren]

or t.Cleanup

[ianlancetaylor]

Sure, please send a patch. Thanks.

[perillo]

@ericlagergren, thanks. Using t.Cleanup is much better.

[perillo]

By the way, in https://github.com/golang/go/blob/master/src/go/build/build_test.go#L543 there is a similar pattern when changing an environment variable

defer os.Setenv("GO111MODULE", os.Getenv("GO111MODULE"))
os.Setenv("GO111MODULE", "off")

I have also noted that that many tests calls os.MkdirTemp instead of T.TempDir, using a descriptive name. Is that name really important?

Thanks.

[ianlancetaylor]

T.TempDir is relatively new (added in the Go 1.14 release), so a lot of tests were using the older ioutil.TempDir which has now been renamed to os.MkdirTemp. It would be OK to adjust most or perhaps all of those tests to use T.TempDir, but it's also fine to just let them be.

[perillo]

Ok, thanks.

Probably moving to T.TempDir is better, since currently the name passed to os.MkdirTemp is not consistent; it is either:

• empty
• t.Name
• the test name but using a string literal instead of t.Name
• short name.

[gopherbot]

Change https://golang.org/cl/306290 mentions this issue: os,path/filepath: use defer to restore the working directory

[perillo]

I have noted that a similar pattern is also used in https://github.com/golang/go/blob/master/src/syscall/syscall_linux_test.go#L27, where a chtmpdir is defined, and in https://github.com/golang/go/blob/master/src/runtime/syscall_windows_test.go#L945.

Maybe the support chdir function should be defined in a shared package (ostest?), together with a setenv function:
https://play.golang.org/

[gopherbot]

Change https://golang.org/cl/307189 mentions this issue: os: don't use T.Cleanup in TestRemoveAllLongPath