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

testing: T.TempDir doesn't work well with testing libraries #38850

Closed
rogpeppe opened this issue May 4, 2020 · 13 comments
Closed

testing: T.TempDir doesn't work well with testing libraries #38850

rogpeppe opened this issue May 4, 2020 · 13 comments

Comments

@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented May 4, 2020

The new T.TempDir method is documented to return the same path on every call within the same test:

Subsequent calls to t.TempDir return the same directory.

This means that it's easy for tests that use TempDir to shoot themselves in the foot by overwriting or reading content owned by testing libraries. To use TempDir correctly in the presence of multiple packages you must use a unique subdirectory name, which is hard to choose, so you'd need to use ioutil.TempDir which loses the whole point of having TempDir in the first place.

I propose that every call to TempDir return a new temporary directory; tests can save the result in a local variable if need be.

If it seems like the implied semantics of the name TempDir no longer match the actual behaviour, another possible name would be Mkdir.

@bcmills bcmills added this to the Go1.15 milestone May 4, 2020
@bcmills
Copy link
Member

@bcmills bcmills commented May 4, 2020

@bcmills
Copy link
Member

@bcmills bcmills commented May 4, 2020

Maybe? But each subtest has its own *testing.T, so if the test needs different subdirectories for different test cases, it seems straightforward enough to split those into separate subtests...

@bcmills
Copy link
Member

@bcmills bcmills commented May 4, 2020

… you'd need to use ioutil.TempDir which loses the whole point of having TempDir in the first place.

Does it? They seem complementary to me: (*T).TempDir constructs a temporary directory with the same lifetime as the test (which can be logged, and perhaps preserved with -work). ioutil.TempDir constructs a unique subdirectory within that directory.

For package-independent test helpers, I would recommend passing in the output directory as an explicit argument anyway. For example, the helper might be called from a subtest, but generate an output for a parent test (perhaps guarded by a sync.Once).

@rogpeppe
Copy link
Contributor Author

@rogpeppe rogpeppe commented May 4, 2020

Does it? They seem complementary to me: (*T).TempDir constructs a temporary directory with the same lifetime as the test (which can be logged, and perhaps preserved with -work). ioutil.TempDir constructs a unique subdirectory within that directory.

Preserving the temporary directory with -work is an interesting consideration, which is not done yet, would need some more work (AFAIK currently the test binary has no way of knowing whether the -work flag was provided or what that temporary directory is named), and seems to me to be orthogonal to this issue.

ISTM that using ioutil.TempDir within that directory is not enough to avoid clashes - a test could walk ioutil.TempDir assuming that its contents have all been created by itself, for example.

Let's put this another way: what's the advantage of always returning the same name from TempDir ?

FWIW I've been using this functionality for a long time now, and I've very often found it to be useful to create several independent temporary directories within a test. Having a handy function that returns a new scratch directory is great! (and makes it easy to avoid unintentional bleedthrough and extra bookkeeping).

@gopherbot
Copy link

@gopherbot gopherbot commented May 4, 2020

Change https://golang.org/cl/231958 mentions this issue: testing: make TempDir return new directory every time

@myitcv
Copy link
Member

@myitcv myitcv commented May 4, 2020

(*T).TempDir constructs a temporary directory with the same lifetime as the test (which can be logged, and perhaps preserved with -work).

Indeed, but I don't think this precludes a subsequent call returning a different directory that is similarly tied to the lifetime of the test. The similar naming of (*T).TempDir and ioutil.TempDir just seems a likely source of confusion (at least I was surprised 😄).

For a given *testing.T, subsequent calls to (*T).TempDir could return directories that share the same temporary root (making the cleanup trivial).

@rogpeppe
Copy link
Contributor Author

@rogpeppe rogpeppe commented May 18, 2020

@bcmills Any further input on this, please? The release approaches fast!

@rsc
Copy link
Contributor

@rsc rsc commented May 19, 2020

I'm skeptical about this. I think of it like os.TempDir, that it returns the "/tmp" for the test. How are libraries and tests colliding exactly? Don't they have different file names they'd use?

@rsc
Copy link
Contributor

@rsc rsc commented May 19, 2020

I talked to @bcmills about this. The argument is that if t.TempDir is like os.TempDir, it's hard to get multiple directories without calls to Mkdir or ioutil.TempDir. But if t.TempDir is like ioutil.TempDir, then it's easy to get a single directory - just call it once and remember the result. That suggests we should make the change that @rogpeppe has proposed.

I'll review the CL. Thanks.

@gopherbot gopherbot closed this in 6d6e482 May 19, 2020
@rogpeppe
Copy link
Contributor Author

@rogpeppe rogpeppe commented May 19, 2020

FWIW that's why I liked the name "Mkdir" which makes it clear that it's making a new directory each time with no confusion between ioutil.TempDir and os.TempDir.

@gopherbot
Copy link

@gopherbot gopherbot commented May 19, 2020

Change https://golang.org/cl/234582 mentions this issue: crypto/x509: save the temp dir in TestReadUniqueDirectoryEntries

gopherbot pushed a commit that referenced this issue May 19, 2020
In CL 231958, TempDir was changed to create a new temp directory on
each allocation, on the theory that it is easy to save in a variable
for callers that want the same directory repeatedly. Apply that
transformation here.

Updates #38850

Change-Id: Ibb014095426c33038e0a2c95303579cf95d5c3ba
Reviewed-on: https://go-review.googlesource.com/c/go/+/234582
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented May 19, 2020

Change https://golang.org/cl/234583 mentions this issue: testing: clean up remaining TempDir issues from CL 231958

gopherbot pushed a commit that referenced this issue May 19, 2020
Updates #38850

Change-Id: I33f48762f5520eb0c0a841d8ca1ccdd65ecc20c8
Reviewed-on: https://go-review.googlesource.com/c/go/+/234583
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@dmitshur dmitshur added NeedsFix and removed NeedsDecision labels May 20, 2020
@mvdan
Copy link
Member

@mvdan mvdan commented May 20, 2020

FYI, we split @rogpeppe's #38850 (comment) into #39169, since this issue is now closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.