-
Notifications
You must be signed in to change notification settings - Fork 17.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
testing: add TB.Chdir #62516
Comments
Change https://go.dev/cl/526717 mentions this issue: |
Change https://go.dev/cl/529895 mentions this issue: |
@rsc anything I could do to move it forward? |
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. |
This proposal has been added to the active column of the proposals project |
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.) |
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. |
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:
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. |
Based on the discussion above, this proposal seems like a likely accept. The proposal is to add a method The method calls t/b.Fatal if the chdir fails. |
Just seeing this now. This would be a welcome addition. I have at least one copy of a good test helper for The ability to fail loudly when combined with |
No change in consensus, so accepted. 🎉 The proposal is to add a method The method calls t/b.Fatal if the chdir fails. |
Sometimes a test need to call
os.Chdir()
. Here is a bare minimum implementation of what's needed from a test to do that: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:
go/src/os/os_windows_test.go
Line 35 in 1d538f1
go/src/os/os_test.go
Line 917 in 1d538f1
go/src/path/filepath/path_test.go
Line 510 in 1d538f1
go/src/path/filepath/path_test.go
Line 527 in 1d538f1
go/src/syscall/syscall_linux_test.go
Line 27 in 1d538f1
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
The text was updated successfully, but these errors were encountered: