Repository navigation
proposal: testing: add more support to automatically cleanup temporary resources or temporary changes to the global environment #45403
Description
Activity
- changed the title
[-]testing: add more support to automatically cleanup temporary resources or temporary changes to the global environment[/-][+]proposal: testing: add more support to automatically cleanup temporary resources or temporary changes to the global environment[/+]on Apr 6, 2021 I noted only now that Setenv was already added to the testing package a month ago.
Is it possible to send now changes to use the new API or it is better to wait after go 1.17 has been released?
Most API additions to the
testingpackage would likely need to go through the proposal process and get approved before being added. It is better to wait until the proposal is approved before sending CLs, unless you need to prototype the changes in order to inform the proposal itself.Yes, but as I wrote
T.Setenvhas already be added a month ago. However I don't know if it is still possible to remove it before the freeze for Go 1.17.Now that t.Setenv exists, it is OK to send CLs to the Go repo to use t.Setenv if that cleans up a test.
We can always rollback those CLs with the rollback of Setenv if that happens (seems unlikely).Given that Setenv and TempDir already exist, it doesn't seem like we need to add ChTempDir (that's just Chdir), nor t.TempFile (that can use os.TempFile with the t.TempDir result).
We could consider adding t.Chdir instead of t.ChTempDir, but any test that uses Chdir at all is unparallelizable. Adding t.Chdir to the API may suggest writing tests that don't scale well. I also don't understand why tests would really need to Chdir. It's almost always better to pass the directory to the function that needs it (like setting cmd.Dir in os/exec). So probably t.Chdir is below the bar, especially since you can already use t.Cleanup to un-Chdir.
Reacted by Tom Thorogood, Dmitri Shuralyov, antichris and pymqThis 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@rsc I agree that functions like ChTempDir or Chdir are not suitable for the testing package. However adding them to an internal package should be fine. The problem is that that package may become something like
ioutil.As for why to chdir in a test, I'm not sure.
Some test probably do it for convenience, as an example: https://github.com/golang/go/blob/master/src/os/os_test.go#L968.
Several tests that tests symlinks seems to call Chdir.For some other test it may necessary, as an example: https://github.com/golang/go/blob/master/src/runtime/syscall_windows_test.go#L1046
It sounds like the only thing left is t.Chdir and we agree it's not a good idea.
We don't even need to add it to an internal package.
Sometimes duplicated code is OK.Fine with me, thanks. However if possible I would like to have all these functions in a custom, unique file, e.g.
testutil_test.go, so that it easy to find them and make sure that the functions defined in multiple packages are in sync.Currently we have
chtmpdir,tempDirCanonicaland I plan to addwriteFile(t *testing.T, name string, data string)to make tests a bit more readable and consistent. Later some functions may be moved totestingor a shared internal package, if they are really useful.Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group- locked and limited conversation to collaborators
on Apr 21, 2022
In go 1.15, the
testingpackage supports theT.TempDirfunction.Currently the method is not used in the tests of the go distribution, but
os.MkdirTempis used 164 times.Here is a list of additional API to manage temporary resources or temporary changes to the global environment in tests:
T.ChTempDirCurrently all: tests that change the working directory should use defer to restore it #45182 has introduced the
chdirfunction, but there are several tests that use it.os.Chdiris used 53 times (including inchdirandchtmpdir), the newchdiris used 8 times and the oldchtmpdiris used 22 times.T.TempFile.Currently
os.CreateTempis used 33 times in tests.T.SetTempEnvCurrently
os.Setenvis used 108 times.The usage number has been computed with a simple:
grep -F "fname(" **/*_test.go | wc -l