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

[perillo]

In path_test.go are defined both the new chdir function and the old chtmpdir that returns a function to use in a defer statement.

Probably the old function should be removed and the new chdir function should be renamed chtmpdir since it is more descriptive.

[gopherbot]

Change https://golang.org/cl/307651 mentions this issue: path/filepath: rename chdir to chtmpdir

[ianwoolf]

In path_test.go are defined both the new chdir function and the old chtmpdir that returns a function to use in a defer statement.

Probably the old function should be removed and the new chdir function should be renamed chtmpdir since it is more descriptive.

emmm.. This file does not have test function that manually chdir(olddir) and do some check. So we just need chTmpDir. i will send a patch.

[perillo]

For reference, here is a list with all tests using the os.Chdir function:

std

internal/execabs/execabs_test.go
io/fs/walk_test.go
os/error_test.go
os/exec/lp_unix_test.go
os/executable_test.go
os/os_windows_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/syscall_windows_test.go
syscall/syscall_linux_test.go

cmd

cmd/doc/doc_test.go
cmd/go/internal/fsys/fsys_test.go
cmd/go/internal/work/build_test.go
cmd/link/linkbig_test.go
cmd/pack/pack_test.go

Probably all these tests chdir to a temporary directory, so chtmpdir is the best API.

Thanks to @ianwoolf for the change.

[perillo]

As I have suggested in https://golang.org/cl/307651, the chtmpdir should be moved to a shared package, as an example internal/ostest, since it will probably be used by about 10 packages.

A possible name is ChdirTemp.

[ianwoolf]

As i wrote in https://golang.org/cl/307651, one depsRules of go/build need to be add if add the internal/testhelper package. Or move the chtmpdir to testing.

Because of the additional depsRules, it seems that testing.ChTmpDir is better? i want a help or suggestion，which way do you think is better?

[ianlancetaylor]

It's fine to modify the go/build test when adding a new internal package. That in itself is not a reason to avoid adding a new package.

I see that you've opened #45403 to add a function to the testing package.

[feluelle]

Would be really cool to have that feature as part of testing

[kolyshkin]

Would be really cool to have that feature as part of testing

I thought the same thing and opened https://github.com/golang/go/issues/62516

[gopherbot]

Change https://go.dev/cl/546217 mentions this issue: path/filepath: simplify chdir and error handling in tests

[kolyshkin]

NOTE we have t.Chdir now, and this is the easiest way to fix the issue.