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

perillo · 2021-03-23T14:33:11Z

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:

call os.Getwd to get the current working directory
some code
call os.Chdir to change the current working directory
some code
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:

call defer chdir(dir)()
some code

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

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

ericlagergren · 2021-03-23T15:38:13Z

or t.Cleanup

ianlancetaylor · 2021-03-23T22:23:37Z

Sure, please send a patch. Thanks.

perillo · 2021-03-24T10:11:09Z

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

perillo · 2021-03-24T10:27:57Z

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 · 2021-03-24T19:46:26Z

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 · 2021-03-24T19:52:32Z

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 · 2021-03-31T17:40:12Z

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

perillo · 2021-03-31T18:49:33Z

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/

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

gopherbot · 2021-04-04T11:55:24Z

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

Revert CL 306290 changes to TestRemoveAllLongPath. This breaks the test on aix, illumos and solaris. We need to chdir out of startPath before attempting to remove it. Updates #45182 Change-Id: Ic14fa1962d6f2cc83238f6fc2c6932fd9a6e52a1 Reviewed-on: https://go-review.googlesource.com/c/go/+/307189 Trust: Tobias Klauser <tobias.klauser@gmail.com> Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>

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

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 · 2021-04-06T13:30:48Z

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

ianwoolf · 2021-04-06T14:13:28Z

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 · 2021-04-06T14:17:37Z

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 · 2021-04-06T14:22:12Z

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 · 2021-04-07T04:33:38Z

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 · 2021-04-07T22:52:27Z

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 · 2022-10-28T08:50:36Z

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

kolyshkin · 2023-09-08T17:32:03Z

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

I thought the same thing and opened #62516

This change simplifies the test code for changing directories by utilizing t.TempDir() within chtmpdir function. It also replaces os.Exit(1) with t.Fatalf to handle errors more consistently with Go's testing practice. The cleanup process is now managed with t.Cleanup, ensuring that any created temporary directories are removed without the need for a deferred restore function. Updates golang#45182

gopherbot · 2023-11-30T11:12:54Z

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

This change simplifies the test code for changing directories by utilizing t.TempDir() within chtmpdir function. It also replaces os.Exit(1) with t.Fatalf to handle errors more consistently with Go's testing practice. The cleanup process is now managed with t.Cleanup, ensuring that any created temporary directories are removed without the need for a deferred restore function. Updates golang#45182

seankhliao added the Testing An issue that has been verified to require only test changes, not just a test failure. label Mar 23, 2021

ianlancetaylor added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Mar 23, 2021

ianlancetaylor added this to the Backlog milestone Mar 23, 2021

This was referenced Apr 6, 2021

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

Open

proposal: testing: add more support to automatically cleanup temporary resources or temporary changes to the global environment #45403

Closed

perillo mentioned this issue Apr 6, 2021

cmd/link: TestLargeText in linkbig_test.go does not look right #45406

Closed

dkegel-fastly mentioned this issue Apr 25, 2022

todo: tidy use of temporary directories and environment variables in tests tinygo-org/tinygo#2809

Open

aimuz mentioned this issue Nov 30, 2023

path/filepath: simplify chdir and error handling in tests #64463

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

perillo commented Mar 23, 2021

ericlagergren commented Mar 23, 2021

ianlancetaylor commented Mar 23, 2021

perillo commented Mar 24, 2021

perillo commented Mar 24, 2021

ianlancetaylor commented Mar 24, 2021

perillo commented Mar 24, 2021

gopherbot commented Mar 31, 2021

perillo commented Mar 31, 2021

gopherbot commented Apr 4, 2021

perillo commented Apr 6, 2021

gopherbot commented Apr 6, 2021

ianwoolf commented Apr 6, 2021 •

edited

perillo commented Apr 6, 2021

perillo commented Apr 6, 2021 •

edited

ianwoolf commented Apr 7, 2021

ianlancetaylor commented Apr 7, 2021

feluelle commented Oct 28, 2022

kolyshkin commented Sep 8, 2023

gopherbot commented Nov 30, 2023

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

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

Comments

perillo commented Mar 23, 2021

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

ericlagergren commented Mar 23, 2021

ianlancetaylor commented Mar 23, 2021

perillo commented Mar 24, 2021

perillo commented Mar 24, 2021

ianlancetaylor commented Mar 24, 2021

perillo commented Mar 24, 2021

gopherbot commented Mar 31, 2021

perillo commented Mar 31, 2021

gopherbot commented Apr 4, 2021

perillo commented Apr 6, 2021

gopherbot commented Apr 6, 2021

ianwoolf commented Apr 6, 2021 • edited

perillo commented Apr 6, 2021

std

cmd

perillo commented Apr 6, 2021 • edited

ianwoolf commented Apr 7, 2021

ianlancetaylor commented Apr 7, 2021

feluelle commented Oct 28, 2022

kolyshkin commented Sep 8, 2023

gopherbot commented Nov 30, 2023

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

ianwoolf commented Apr 6, 2021 •

edited

perillo commented Apr 6, 2021 •

edited