testing: add TB.Chdir - Githubissues

kolyshkin commented 1 year ago

Sometimes a test need to call os.Chdir(). Here is a bare minimum implementation of what's needed from a test to do that:

        oldwd, err := Getwd()    
        if err != nil {
                t.Fatal(err)
        }
        if err := Chdir(dir); err != nil {
                t.Fatal(err)
        }
        t.Cleanup(func() {
                if err := Chdir(oldwd); err != nil {
                        // It's not safe to continue with tests if we can't get back to
                        // the original working directory.
                        panic(err)
                }
        })

The code above can be used as a test helper; in fact, this repository already contains at least 5 helpers similar to the one above:

In addition, there are a few in-line implementations of the same functionality, another implementation in golang.org/x/sys/unix and so on.

The problem with this (except for multiple implementations and re-implementations) is, tests that use it can not use t.Parallel. Currently, there is no way to ensure that.

The issue is very similar to one for os.Setenv (#41260, fixed by https://golang.org/cl/326790); thus the solution is also similar.

The proposal is to add a Chdir method to the testing package, which will take care about all of the above.

The implementation may look like this: https://go-review.googlesource.com/c/go/+/529895

gopherbot commented 1 year ago

Change https://go.dev/cl/526717 mentions this issue: testing: add Chdir

gopherbot commented 12 months ago

Change https://go.dev/cl/529895 mentions this issue: testing: add Chdir

kolyshkin commented 4 months ago

@rsc anything I could do to move it forward?

rsc commented 3 months ago

If tests need to chdir often, then I do think it is worth doing well, in much the same way that t.Setenv did (takes care of undoing, also takes care of not being in parallel tests). That said, I am not sure whether tests need to chdir often enough. Skimming CL 529896, it seems telling that all the changes are in packages like os, path/filepath, syscall, which are specifically about directories. Tests for code that is not about directories would be more compelling. I will try to gather some data from the open source Go corpus.

rsc commented 3 months ago

This proposal has been added to the active column of the proposals project and will now be reviewed at the weekly proposal review meetings. — rsc for the proposal review group

jimmyfrasche commented 3 months ago

I ran into this recently. If it is helpful: https://github.com/jimmyfrasche/autoreadme/blob/0fbe6087ce8309d209b6e9caf3621e781bc73fcb/autoreadme_test.go

(The program works on go modules in git repos and it needs to run in a directory within such a module and update files in that tree. Normally I would try to mock such things out but that would have increased the complexity of both the code and the tests too much. It was simpler to just have the test cd into mock projects and run the program there and then check the results against golden output. Because the projects contain go.mod in testdata they're stored indirectly in .txtar archives that are expanded into a tmp dir which is not relevant to the issue at hand but explains why there's all those other things happening in the test code.)

kolyshkin commented 3 months ago

This is needed to test any functionality/code that uses relative paths.

You can find many examples of tests that use os.Chdir here: https://sourcegraph.com/search?q=context:global+file:_test.go%24+os.Chdir&patternType=keyword&sm=0

Most of them do the same sequence as in this issue description (save cwd, chdir, defer chdir to saved). Most are not entirely correct as they should panic if Chdir to oldwd failed.

None are preventing running tests using Chdir in parallel, although some warn about it, see eg https://github.com/moby/moby/blob/ceefb7d0b9c5b6fbd1ea7511592a4ddb28ec4821/pkg/idtools/idtools_unix_test.go#L210-L221). This is somewhat hard to implement as you'd need a wrapper around t.Parallel.

rsc commented 3 months ago

Took me longer than expected but I finished scanning the latest versions of each module in the proxy cache. Overall there are 8756664 _test.go files, and 23963 files mention os.Chdir, or about 0.27%. It's not high, but we didn't expect it to be that high. The next question is whether os.Chdir is really necessary in those tests. I sampled 20 at random (numbers 1..20 in the sampled results here) and found:

1 unnecessary substitute for filepath.ResolveSymlinks (2)
4 not strictly necessary but helpful (1, 7, 8, 15)
15 necessary (3, 4, 5, 6, 9, 10, 11, 12, 13, 14, 16, 17, 18, 19, 20)

And 5 (25%) forgot to chdir back, which could break other tests (5, 10, 12, 16, 18).

It still doesn't come up often, but when it does it's difficult to get right. I think it is reasonable for the testing package to help, in the same way that it helps with os.Setenv.

rsc commented 3 months ago

Based on the discussion above, this proposal seems like a likely accept. — rsc for the proposal review group

The proposal is to add a method Chdir(dir string) to to testing.T, testing.B, testing.TB.

The method calls t/b.Fatal if the chdir fails. It also calls t/b.Fatal if called during a parallel test. It also makes sure to restore the previous current directory when the test finishes. Internally, it can use Open(".") and os.Fchdir. It should probably also call t.Setenv to set $PWD, like os/exec does when Dir is set.

ChrisHines commented 3 months ago

Just seeing this now. This would be a welcome addition. I have at least one copy of a good test helper for os.Chdir in a program that operates on git repositories. After a quick search of the code on my current project I found multiple tests that currently use os.Chdir without attempting to play nice with other tests at all. They would benefit from using this feature. Maybe some of those could be restructured to avoid the need for os.Chdir, but probably not all of them.

The ability to fail loudly when combined with t.Parallel is especially nice and currently not possible to implement in a test helper outside the testing package.

rsc commented 2 months ago

No change in consensus, so accepted. 🎉 This issue now tracks the work of implementing the proposal. — rsc for the proposal review group

The proposal is to add a method Chdir(dir string) to to testing.T, testing.B, testing.TB.

The method calls t/b.Fatal if the chdir fails. It also calls t/b.Fatal if called during a parallel test. It also makes sure to restore the previous current directory when the test finishes. Internally, it can use Open(".") and os.Fchdir. It should probably also call t.Setenv to set $PWD, like os/exec does when Dir is set.

gopherbot commented 3 weeks ago

Change https://go.dev/cl/607035 mentions this issue: testing: use temp dir without symlinks in TestChdir/relative

gopherbot commented 3 weeks ago

Change https://go.dev/cl/607095 mentions this issue: doc: revert #62516 relnote accidentally modified in CL 603959

golang / go

testing: add TB.Chdir #62516