Skip to content
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

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

Closed
perillo opened this issue Apr 6, 2021 · 14 comments
Labels
FeatureRequest FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Proposal
Milestone

Comments

@perillo
Copy link
Contributor

perillo commented Apr 6, 2021

In go 1.15, the testing package supports the T.TempDir function.
Currently the method is not used in the tests of the go distribution, but os.MkdirTemp is used 164 times.

Here is a list of additional API to manage temporary resources or temporary changes to the global environment in tests:

  • T.ChTempDir
    Currently all: tests that change the working directory should use defer to restore it #45182 has introduced the chdir function, but there are several tests that use it.
    os.Chdir is used 53 times (including in chdir and chtmpdir), the new chdir is used 8 times and the old chtmpdir is used 22 times.

  • T.TempFile.
    Currently os.CreateTemp is used 33 times in tests.

  • T.SetTempEnv
    Currently os.Setenv is used 108 times.

The usage number has been computed with a simple: grep -F "fname(" **/*_test.go | wc -l

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 6, 2021
@dmitshur dmitshur added this to the Backlog milestone Apr 6, 2021
@dmitshur
Copy link
Contributor

dmitshur commented Apr 6, 2021

CC @mpvl via owners.

@perillo
Copy link
Contributor Author

perillo commented Apr 6, 2021

As an alternative these functions can be added to an internal package. In #45182 I have proposed internal/ostest.

Thanks.

@ianlancetaylor ianlancetaylor 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 Apr 6, 2021
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Apr 6, 2021
@ianlancetaylor ianlancetaylor modified the milestones: Backlog, Proposal Apr 6, 2021
@perillo
Copy link
Contributor Author

perillo commented Apr 7, 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?

@dmitshur
Copy link
Contributor

dmitshur commented Apr 7, 2021

Most API additions to the testing package 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.

@perillo
Copy link
Contributor Author

perillo commented Apr 7, 2021

Yes, but as I wrote T.Setenv has 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.

@dmitshur
Copy link
Contributor

dmitshur commented Apr 7, 2021

Adding TB.Setenv was an accepted proposal #41260.

@rsc
Copy link
Contributor

rsc commented Apr 7, 2021

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).

@rsc
Copy link
Contributor

rsc commented Apr 7, 2021

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.

@rsc rsc moved this from Incoming to Active in Proposals (old) Apr 7, 2021
@rsc
Copy link
Contributor

rsc commented Apr 7, 2021

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

@perillo
Copy link
Contributor Author

perillo commented Apr 7, 2021

@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

@rsc
Copy link
Contributor

rsc commented Apr 14, 2021

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.

@perillo
Copy link
Contributor Author

perillo commented Apr 14, 2021

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, tempDirCanonical and I plan to add writeFile(t *testing.T, name string, data string) to make tests a bit more readable and consistent. Later some functions may be moved to testing or a shared internal package, if they are really useful.

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Apr 14, 2021
@rsc
Copy link
Contributor

rsc commented Apr 14, 2021

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

@rsc
Copy link
Contributor

rsc commented Apr 21, 2021

No change in consensus, so declined.
— rsc for the proposal review group

@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Apr 21, 2021
@rsc rsc closed this as completed Apr 21, 2021
@golang golang locked and limited conversation to collaborators Apr 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FeatureRequest FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Proposal
Projects
No open projects
Development

No branches or pull requests

5 participants