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 Cleanup method #32111

Open
rogpeppe opened this issue May 17, 2019 · 26 comments

Comments

@rogpeppe
Copy link
Contributor

commented May 17, 2019

It's a common requirement to need to clean up resources at the end of a test - removing temporary directories, closing file servers, etc.

The standard Go idiom is to return a Closer or a cleanup function and defer a call to that, but in a testing context, that can be tedious and add cognitive overhead to reading otherwise simple test code. It is useful to be able to write a method that returns some domain object that can be used directly without worrying the caller about the resources that needed to be created (and later destroyed) in order to provide it.

Some test frameworks allow tear-down methods to be defined on "suite" types, but this does not compose well and doesn't feel Go-like.

Instead, I propose that we add a Defer method to the testing.B, testing.T and testing.TB types that will register a function to be called at the end of the test.

The implementation could be something like this:

type T struct {
	// etc
	mu sync.Mutex
	deferred func()
}

// Defer registers a function to be called at the end of the test.
// Deferred functions will be called in last added, first called order.
func (t *T) Defer(f func()) {
	t.mu.Lock()
	defer t.mu.Unlock()
	oldDeferred := t.deferred
	t.deferred = func() {
		defer oldDeferred()
		f()
	}
}

// done calls all the functions registered by Defer in reverse
// registration order.
func (t *T) done() {
	t.mu.Lock()
	deferred := t.deferred
	t.mu.Unlock()

	deferred()
}

The quicktest package uses this approach and it seems to work well.

@gopherbot gopherbot added this to the Proposal milestone May 17, 2019
@gopherbot gopherbot added the Proposal label May 17, 2019
@natefinch

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

I would use the heck out of this in my tests.

@ChrisHines

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

Can you provide an example of when this is better than a basic defer?

@natefinch

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

Because it means you can do the defer from inside a helper function.

So you get this:

func TestSomething(t *testing.T) {
    dir := mkDir(t)
    // use dir
}

func mkDir(t *testing.T) string {
    name, err := ioutil.TempDir("", "")
    if err != nil {
        t.Fatal(err)
    }
    t.Defer(func() {
        err := os.RemoveAll(name)
        if err != nil {
            t.Error(err)
	})
    return name
}

Without Defer, you have to return a cleanup function from mkDir that you then defer:

func TestSomething(t *testing.T) {
    dir, cleanup := mkDir(t)
    defer cleanup()
    // use dir
}

func mkDir(t *testing.T) (string, func()) {
    name, err := ioutil.TempDir("", "")
    if err != nil {
        t.Fatal(err)
    }
    return name, func() {
        err := os.RemoveAll(name)
        if err != nil {
            t.Error(err)
	})
}

If it's just one thing, like a single directory you're creating, it's not that bad. But when it's a whole bunch of things, it adds a lot of noise for little benefit.

@cespare

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

If there are many things, then you can create a test helper wrapping type that initializes them all for you at once and the close method can clean up/close/delete all of them.

This proposal adds an API that explicitly overlaps with a language feature, making tests look yet a bit more different from normal code. There is a nice symmetry (common in test and non-test code) where the creation and deferred cleanup of a thing are paired:

t := newThing()
defer t.cleanup()

(think files or mutexes); where it exists, this pattern is clear and obvious, and t.Defer breaks it by putting the cleanup elsewhere.

In the end, this is all to save a single line of code per test, which feels like an un-Go-like tradeoff to me. So initially I'm mildly opposed to this feature.

@rogpeppe

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2019

In the end, this is all to save a single line of code per test, which feels like an un-Go-like tradeoff to me. So initially I'm mildly opposed to this feature.

I understand where you're coming from, so let me try to explain one use case with reference to some existing tests that use this feature.

Firstly, tests already look different from normal code. In tests, unlike production code, we can concern ourselves with happy path only. There's always an easy solution when things don't happen the way they should - just abort the test. That's why we have T.Fatal in the first place. It makes it possible to have a straightforward sequence of operations with no significant logic associated with branches.

When testing more complex packages, it becomes useful to make test helper functions that set up or return one or more dependencies for the test. Sometimes helper functions run several levels deep, with a high level function calling various low level ones, all of which can call T.Fatal.

Here's an test helper function from some real test code:

https://github.com/juju/charmstore-client/blob/799c23e4ad134e4115837060a3f7831401a9097e/internal/ingest/testcs_test.go#L40-L76

It's building a *testCharmstore instance, but to do that, it has to put together various dependencies, any of which might fail. We want to guarantee that things are cleaned up even if a later dependency cannot be acquired. For example, if the call to charmstore.NewServer fails, we'd like to close the database connection and cs.discharger (another server instance).

Without the Defer method, that's not that easy to do. Even if you define a Close method on the returned *testCharmstore type, you'd have to write some code to clean up the earlier values if the later ones fail. Of course, that's entirely possible to do, as we do in production code.

However, given Defer, we have an easy option: we can write straight-line initialization code, failing hard if anything goes wrong, but still clean up properly if it does.

@beoran

This comment has been minimized.

Copy link

commented May 20, 2019

A simple way to solve this problem is to use a helper that opens and closes the resource and that takes the real test as a closure function parameter. This is a common idiom in Ruby, which I also use often in Go. Something like this:

func TestSomething(t *testing.T) {
    dir := withResourceHelper(t, func (r Resource) {
       // perform test in here, can use resource
    })
}

func withResourceHelper(t *testing.T, testFunc func(r Resource)) {
    resource, err := NewResource()
    if err != nil {
        t.Fatal(err)
    } else {
      defer resource.Close()
      testFunc(resource)
   }
}
@rogpeppe

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

A simple way to solve this problem is to use a helper that opens and closes the resource and that takes the real test as a closure function parameter.

Yes, that is a possible way of working around this, but as a personal preference in Go, I prefer to avoid this callback-oriented style of programming - it leads to more levels of indentation than I like, and I don't find it as clear. Rather than just asking for a resource to use, you have to give up your flow of control to another function which makes this style less orthogonal to other use cases. For example, callback style doesn't play well with goroutines - if you wanted to create two such resources concurrently, you'd need to break the implied invariant that you shouldn't use the resource outside of the passed function. It also means that if you do want to add some cleanup to an existing test helper, significant refactoring can be required.

@bouk

This comment has been minimized.

Copy link

commented Jul 9, 2019

I expect Defer would get widely adopted by various test helpers, as checking for conditions at the end of a test is common pattern. For example: gomock has a Finish method that you’re supposed to defer in your test, but this is commonly overlooked. A built-in t.Defer would allow gomock to automatically register this Finish method to ensure all the conditions are met at the end of the test.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

Because the argument f to t.Defer(f) does not run at the end of the function in which that call appears, t.Defer is probably not the right name. Perhaps t.Cleanup(f) would be clearer.

Except for the name, it does seem like a test case does have a well-defined "everything is done now" moment that could be worth attaching a blocking cleanup to, so this proposal seems plausible.

(We have declined to add an os.AtExit, but completing a test is different from exiting a process. You really do want to get everything cleaned up before starting the next test. AtExit is too often abused for things that the OS is going to clean up anyway, clean exit or not.)

/cc @robpike @mpvl (past testing API designers)

@rogpeppe

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2019

Because the argument f to t.Defer(f) does not run at the end of the function in which that call appears, t.Defer is probably not the right name. Perhaps t.Cleanup(f) would be clearer.

FWIW Cleanup is similar to the name I first gave to this functionality (I actually used AddCleanup), but I changed it to defer because I thought that it's a more obvious name. The fact that you're calling it on the testing value and that it's not the usual defer keyword is, I think, sufficient clue that it won't be called at the end of the current function. You're still "deferring" the function, even if it's deferred at test level rather than function level. To me, "Cleanup" sounds like it's actually doing the cleanup there and then, rather than scheduling something to happen later, and "AddCleanup" is more accurate but clumsier than "Defer".

@rsc

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2019

@robpike and @mpvl, any thoughts? Thanks.

@robpike

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2019

I like the idea but agree (slightly) that Defer might not be the best name.

@egonelbre

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2019

@rogpeppe any reason not to use a func that expects an func() error?

I would think that closing something that implements io.Closer or returns an error is more common. Or when you close something then it will return an error.

An example for handling a temporary file:

func TestFile(t *testing.T) {
    file, err := ioutil.TempFile("", "example")
    if err != nil {
        t.Fatal(err)
    }
    t.Defer(func() error { return os.Remove(file.Name()) })
    t.Defer(file.Close)

    // ...
}
@mvdan

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

@egonelbre what would we do if we encounter an error? t.Fatal? t.Error? Something else?

The current approach just lets the user decide, and it's more general. For example:

t.Defer(func() {
    if err := os.Remove(file.Name()); err != nil {
        t.Fatal(err) // t obtained via closure
    }
})
@egonelbre

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2019

@mvdan since it's intended for deferred code, and would always need to run, so it shouldn't make a difference whether t.Fatal or t.Error would be called. However, given it's cleanup code, t.Error would probably make more sense... since all the other cleanups need to run as well.

@rsc rsc changed the title proposal: testing: add Defer method proposal: testing: add Cleanup method Oct 2, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2019

I spoke to @robpike, who said he was fine with the functionality and had a slight preference against Defer (because it's not exactly what that keyword does).

AddCleanup is longer and more precise, but it doesn't seem like people would get too confused by seeing t.Cleanup taking a func(). (That is, t.Cleanup() would look confusing, but that's not what the calls will look like.)

Are there any objections to adding this as t.Cleanup?

@muirrn

This comment has been minimized.

Copy link

commented Oct 2, 2019

What about t.Finally(myCleanup) seeing as "finally" has a similar connotation from languages like Java? As mentioned above "Cleanup" is a bit inconsistent since all other imperative sounding testing.T methods happen right away.

@rogpeppe

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2019

FWIW I still think that Defer is better than Cleanup (and Finally). The execution of the function is still being deferred. "Defer" is a nice word for that. Yes, it's deferred at test scope rather than at function scope, but I believe that's clear from the fact that it's a method on *testing.T and from the fact that there's no way to add a function-scoped defer without an explicit defer statement.

There are other useful semantic implications from calling it "Defer", by analogy with the defer statement:

  • that deferred functions are run LIFO order
  • that deferred functions are still run even on panic, or when a previous defer panics

From personal experience, having used the Defer name for a while now for this functionalty, I can say that it feels very natural.

@bvisness

This comment has been minimized.

Copy link

commented Oct 3, 2019

I think Cleanup is the best name presented so far. It makes the intended use of the function clear (more so than Defer), and there are already other methods like Helper and Parallel on T that follow a similar naming scheme.

@rogpeppe expressed concerns about Cleanup sounding like it does the cleanup then and there. I don't share those concerns, for a couple reasons. First, if execution happened then and there, it would be pointless to call Cleanup in the first place. Second (and this is a little bit obtuse of me), I would then expect the function to be called CleanUp instead of Cleanup (because instead of registering a cleanup function, it is cleaning up).

I was also considering the name After, based on my experiences with other test frameworks, but I think Cleanup is more explicit, and doesn't leave the user wondering where Before is.

@iand

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2019

I think Defer is quite abstract even though it is familiar as a keyword to Go programmers. Cleanup has an implication that it should only be used for one purpose.

I propose we simply be direct and call it AfterTest

@mvdan

This comment has been minimized.

Copy link
Member

commented Oct 8, 2019

I propose we simply be direct and call it AfterTest

Don't mean to be nitpicky, but this would run at the end of a test, not after it. AfterTest could thus be even more misleading.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

The emoji on #32111 (comment) suggest that Cleanup is at least not terrible - 5up 1down. The comments since then are mixed and veering into bikeshedding that is unlikely to converge. But we appear to have converged on adding the function, just not on the name. I suggest we move forward with Cleanup in the absence of a consensus about a better name.

This seems like a likely accept.

Leaving open a week for any further comments.

@mvdan

This comment has been minimized.

Copy link
Member

commented Oct 10, 2019

I agree that we don't have the perfect name, but I still think Defer is the best one we have for the reasons Roger laid out in #32111 (comment). I've voted down the decision to converge on Cleanup, to make my position clearer :)

@narqo

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2019

While I can't say I like the method t.Defer() name, I wanted to say, that it feels being in align with x/sync/errgroup.Go().

It's true that the method won't work exactly as defer, but semantically (at least in my head) it will do the same thing.

Another point for names like t.Defer(), t.After(), t.Finally() is that the names don't add extra meaning or specific assumptions about what the passed function must do (in contrast to cleanup, that implies the function must do exactly cleaning).

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Oct 16, 2019

Cleanup voting is now 9 to 3. No name is perfect. Accepting for Cleanup.

-- for @golang/proposal-review

@gopherbot

This comment has been minimized.

Copy link

commented Oct 16, 2019

Change https://golang.org/cl/201359 mentions this issue: testing: implement Cleanup method

@andybons andybons changed the title proposal: testing: add Cleanup method testing: add Cleanup method Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.