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: add TB.TempDir() string #35998

Closed
bradfitz opened this issue Dec 5, 2019 · 28 comments
Closed

testing: add TB.TempDir() string #35998

bradfitz opened this issue Dec 5, 2019 · 28 comments

Comments

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Dec 5, 2019

It's pretty usual to have tests make temp files.

And it's pretty usual to have tests leak temp files. (#23619 tracks detecting it for Go's own tests, as it keeps coming up)

Doing things properly also gets kinda boilerplatey.

I propose adding a method to testing.TB, *testing.T, and *testing.B something like this:

// TempDir returns a temporary directory for the test to use.
// It is lazily created on first access, and calls t.Fatal if the directory
// creation fails.
// Subsequent calls to t.TempDir return the same directory.
// The directory is automatically cleaned up when the test exits.
func (t *T) TempDir() string {
	t.tempDirOnce.Do(func() {
		t.tempDir, t.tempDirErr = ioutil.TempDir("", t.Name())
		if t.tempDirErr == nil {
			t.Cleanup(func() {
				if err := os.RemoveAll(t.tempDir); err != nil {
					t.Errorf("TempDir RemoveAll cleanup: %v", err)
				}
			})
		}
	})
	if t.tempDirErr != nil {
		t.Fatalf("TempDir: %v", err)
	}
	return t.tempDir
}
@gopherbot gopherbot added this to the Proposal milestone Dec 5, 2019
@bontibon
Copy link
Contributor

@bontibon bontibon commented Dec 5, 2019

Is it possible that the artifacts in the temporary directory would be useful in the event of a test failure? In which case, the directory should not be removed. Or should it be expected that any t.Error calls contain all the necessary information to debug the failure?

Loading

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Dec 6, 2019

Yeah, the artifacts could be useful for debugging. If there is an explicit os.RemoveAll, I can just comment that out when debugging. With the new function I guess I can do the same by digging into the testing package and commenting out that line, but that is more complicated. Maybe add a new helper that keeps the artifacts that can be used for debugging? t.Keep()? t.NoCleanup?

Loading

@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Dec 6, 2019

@cherrymui, no need. You can write:

   defer func() { if t.Failed() { os.Exit(1) } }

Or since you know it's already failing:

    defer log.Fatalf("check out the failing files in %v", t.TempDir())

... then no cleanup will happen due to the os.Exit(1) (which log.Fatalf also does).

Loading

@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Dec 6, 2019

Why not have a flag which prevents tempdir deletion, something like -test.keeptmp? I'd think that'd be simpler than manually inserting defers (for most uses), and within this proposal the testing framework is already in charge of managing the directory's lifetime.

Loading

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Dec 6, 2019

@bradfitz Good to know. Thanks!

Loading

@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Dec 6, 2019

@zikaeroh, flag/knob fatigue. Too much crap can be overwhelming. Especially for stuff that's rarely needed. "It might be useful sometimes" is not the bar to add stuff. You also have to consider the fatigue costs for more docs and more -help spew.

Loading

@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Dec 6, 2019

Since I’m not seeing it mentioned, have you considered a more verbose and explicit API variation that returns a cleanup closure?

Somewhat inspired by context.WithCancel, it may look like:

func (*T) TempDir() (dir string, cleanup func())

It would make idiomatic first usage require two extra lines:

func Test(t *testing.T) {
	// ...
	tempDir, cleanup := t.TempDir()
	defer cleanup()
	// ... use tempDir
}

The advantage would be that it’s easier to temporarily modify not to clean up, more readable that there’s no leak to first-time readers, possible to detect when cleanup isn’t called, but at the cost of some additional verbosity. But maybe two lines isn’t bad compared to the 4+ it requires now.

Throwing it out there for consideration, but I’m not convinced this would be better.

Loading

@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Dec 6, 2019

@dmitshur, if I wanted to write verbose code, I already can today? 🤷‍♂️

Loading

@bradfitz

This comment has been hidden.

@dmitshur

This comment has been hidden.

@bradfitz

This comment has been hidden.

@cespare
Copy link
Contributor

@cespare cespare commented Dec 6, 2019

I wrote some tempdir helpers that we use for a lot of tests at work. A trick I used (which I took from @tv42) is to detect whether the test is being run under go test and, if it is, locate the tempdir inside the go tool's own temp directory. (In this case the deferred td.Remove() does nothing.) Then if you want to inspect the tempdir, you use -work to print the directory and not delete it.

Loading

@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Dec 6, 2019

@cespare, @tv42, cute! (maybe a little fragile and thus gross, though?)

Loading

@tv42
Copy link

@tv42 tv42 commented Dec 7, 2019

Would love to have that possible without hacks like that!

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Mar 11, 2020

Now that we have t.Cleanup, this seems like a very nice API. I'd be in favor of trying this. Anyone else have any thoughts?

Loading

@mvdan
Copy link
Member

@mvdan mvdan commented Mar 18, 2020

I've personally written a very similar helper three times in three separate modules, so I agree it would be useful. Also CC @rogpeppe @myitcv who participated in the Cleanup proposal.

Loading

@egonelbre
Copy link
Contributor

@egonelbre egonelbre commented Mar 18, 2020

We've been using:

dirname := t.TempDir("a", "b")
filename := t.TempFile("a", "b", "c.txt")

// internally these would do `filepath.Join` and `os.MkdirAll`

Since it's quite common to want a filename or a subdirectory, rather than just the "root directory".

Loading

@bcmills
Copy link
Member

@bcmills bcmills commented Mar 18, 2020

The manual version also doesn't work well when one or more of the directories ends up read-only. (In that case, the obvious RemoveAll call doesn't succeed in removing the temp directory.)

That could be addressed in RemoveAll (see also #26295, #29983), but it's further witness to the fact that cleaning up a temporary directory is not entirely trivial.

Loading

@rogpeppe
Copy link
Contributor

@rogpeppe rogpeppe commented Mar 18, 2020

In general, I think this is very useful functionality and probably worth adding to the standard library.

Having been using a similar thing for quite a few years (named Mkdir rather than TempDir), I can say that I've never once had an issue with removing read-only directories. I think it would probably be fine if the test fails when the directory can't be removed.

FWIW, two other similar helpers I've found enduringly useful over time are Setenv and Patch, in case those might be considered for adding too.

Loading

@nightlyone
Copy link
Contributor

@nightlyone nightlyone commented Mar 18, 2020

@rogpeppe the other helpers are not compatible with t.Parallel().

@rsc please make sure the Tempdir method has a well defined behaviour for t.Parallel() and subtests before ending the proposal process.

Loading

@narqo
Copy link
Contributor

@narqo narqo commented Mar 19, 2020

Does it have to be a part of testing.TB interface? The current API of testing package feels very general, while use-cases specific helpers are moved into sub-packages, e.g iotest, httptest.
Wouldn't it be better to move this into a separate package, specific for testing FS-related cases?

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Mar 25, 2020

It seems like temporary directories do come up in a large enough variety of tests to be part of testing proper.
Note that iotest and httptest are for testing io and http (and related implementations); it wouldn't make sense to put other I/O helpers in iotest.

Setenv and Patch seem less generally necessary, and now they can be implemented easily using Cleanup. (So can TempDir but it seems more widely applicable.)

The directory should be named with the test binary name and the test function name.
(The example above only used the test function name.)

Based on the discussion above, it sounds like this is a likely accept.

Loading

@rsc rsc moved this from Active to Likely Accept in Proposals Mar 25, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Apr 1, 2020

No change in consensus, so accepted.

Loading

@rsc rsc moved this from Likely Accept to Accepted in Proposals Apr 1, 2020
@bradfitz bradfitz self-assigned this Apr 1, 2020
@bradfitz bradfitz removed this from the Proposal milestone Apr 1, 2020
@bradfitz bradfitz added this to the Go1.15 milestone Apr 1, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 1, 2020

Change https://golang.org/cl/226877 mentions this issue: testing: add TB.TempDir

Loading

@rsc rsc changed the title proposal: testing: add TB.TempDir() string testing: add TB.TempDir() string Apr 1, 2020
@gopherbot gopherbot closed this in 888a0c8 Apr 2, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 3, 2020

Change https://golang.org/cl/226983 mentions this issue: doc: document testing.TB.TempDir in release notes

Loading

gopherbot pushed a commit that referenced this issue Apr 3, 2020
Updates #35998

Change-Id: I93784e9a9efdd1531e3c342aa0899bf059da0ae1
Reviewed-on: https://go-review.googlesource.com/c/go/+/226983
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@Deleplace
Copy link

@Deleplace Deleplace commented May 5, 2020

What's the guideline for making sure the package testing respects the Go 1 compatibility promise? Adding methods like Cleanup and TempDir to the interface testing.TB breaks custom implementations.

The test code is not explicitly excluded from the document, and it's unclear to me if we're in the "it may be necessary to add methods to types" case, which seems to be more concerned about structs and embedding.

Loading

@mvdan
Copy link
Member

@mvdan mvdan commented May 5, 2020

@Deleplace note that the docs already say TB is the interface common to T and B, and the implementation explicitly forbids any other type from ever implementing TB:

https://golang.org/src/testing/testing.go?s=19485:20043#L536

	// A private method to prevent users implementing the
	// interface and so future additions to it will not
	// violate Go 1 compatibility.
	private()

If someone wants to use a subset of the interface that's also implemented by other types, they should declare their own interface with the few methods they are interested in. Backwards compatibility will apply there, because we will only ever add methods to TB, not modify or remove existing ones.

Loading

@Deleplace
Copy link

@Deleplace Deleplace commented May 5, 2020

Thanks Daniel, that's exactly the info I was looking for! Indeed I was not aware of TB.private.

Loading

wojtekmach added a commit to wojtekmach/elixir that referenced this issue May 15, 2020
@golang golang locked and limited conversation to collaborators May 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

None yet