proposal: testing: add TB.CleanupErr

[mvdan]

https://github.com/golang/go/issues/32111 added TB.Cleanup, which I find really useful. However, more often than not I actually have a func() error rather than a func() - and I want the cleanup to fail the test if a non-nil error appears.

Right now you can sort of get away with this by either ignoring the error, e.g. t.Cleanup(func() { returnsErr() }), or something similar which checks the error and calls t.Error if it's not nil.

t.Cleanup(func() {
    if err := returnsErr(); err != nil {
        t.Error(err)
    }
})

I'm left wishing for t.CleanupErr taking func() error due to this being a relatively common scenario in my experience - then I could do e.g. t.CleanupErr(returnsErr), or t.CleanupErr(f.Close), or t.CleanupErr(cmd.Wait), or t.CleanupErr(proc.Kill), etc.

The API would call TB.Error instead of TB.Fatal if an error is returned, since we still want all cleanups to run even when some of them fail. If an early cleanup fails and makes others fail as well, getting more errors is better than skipping some cleanups and potentially not releasing/deleting resources.

In https://github.com/golang/go/issues/32111#issuecomment-535419156, @egonelbre suggested that TB.Cleanup should have taken func() error from the start, and I disagreed with that, funnily enough :) I've changed my mind since then per the above. It's worth noting that I still think the func() form is useful - plenty of use cases don't return an error, such as t.Cleanup(cancel) or t.Cleanup(func() { global = nil }).

Worth noting that this should also make cases where parameters are involved simpler, even if a func literal would still be needed. For example,

t.Cleanup(func() {
    if err := os.Chdir(orig); err != nil {
        t.Error(err)
    }
})

could become just t.CleanupErr(func() error { return os.Chdir(orig) }), avoiding the need for multiple lines.

I'm also reminded of the new sync.OnceX APIs too - OnceFunc takes func(), OnceValue takes func() T, OnceValues takes func() (T1, T2). I don't think generics make sense here, since the testing package can only do something reasonably useful with an error.

[mvdan]

As one data point, this morning I fixed a bug where a test did t.Cleanup(func() { os.Remove(tmpFile) }), and we weren't actually closing the file properly before the remove call. On Windows, this led to the os.Remove silently failing, since you can't remove a file while it's still open. We never noticed due to the lack of error checking, which I attribute to laziness, since doing the proper error check is five lines instead of one, like the last example above.

Instead we could have written t.CleanupErr(func() error { return os.Remove(tmpFile) }) from the start, which is just three more words.

[mvdan]

Happy to send a CL if this gets approved - the implementation is trivial. This is purely about reducing verbosity on the caller side.

[mvdan]

Yet another example is chaining of multiple cleanup steps while checking errors. Right now, I might write:

t.Cleanup(func() {
    if err := step1(); err != nil {
        t.Error(err)
    }
    if err := step2(arg); err != nil {
        t.Error(err)
    }
})

Whereas now I could write:

t.CleanupErr(func() error { return step2(arg) })
t.CleanupErr(step1)

[earthboundkid]

Devil's advocate: Isn't the obvious missing API here t.AssertNilErr? If we don't want to add that (and I think we don't), why not? Isn't this a step on the slippery slope to t.AssertNilErr?

Context: I wrote my own be.NilErr helper.

[mvdan]

Sorry, I don't see how that's an obvious conclusion, or how it relates to this proposal.

[earthboundkid]

If there were t.AssertNilErr, you would write this instead:

t.Cleanup(func() {
    t.AssertNilErr(os.Chdir(orig))
})

This feels a little bit too much like fixing one spot where writing if err != nil { t.Error(err) } is tedious so it ends up being skipped, leading to test mistakes, but there are lots of places where that is tedious to write, so a lot of people use testify etc. to get out of writing it.

[mvdan]

Personally, I disagree - I would rather solve the issue of if err != nil generally for all Go code, like with the former try proposal, and not just for test funcs taking testing.TB. AssertNilErr feels like a band-aid with a rather narrow scope.

If general error handling were to make TB.CleanupErr entirely unnecessary, I'd happily retract this proposal. For now it's hard to say; there isn't active work around it, as far as I'm aware.

[earthboundkid]

FWIW, with @dsnet's try package, you could do:

defer try.F(t.Fatal)

t.Cleanup(func() {
    try.E(os.Chdir(orig))
})

[apparentlymart]

Much moreso than the rest of the standard library, the testing.T API seems to prioritize concisely expressing common situations vs. aiming for orthogonality. There are already numerous ways to make a test fail even though t.Fail is all that is strictly needed, and I assume that's to help test code be relatively concise to avoid distracting from what's actually being tested.

I agree with the assessment that most times I have cleanup to do in a test it's something fallible, like deleting some temporary files or terminating a child process, and so I would agree that this seems like a common enough situation to deserve a concise way of writing it, and that having a concise way to write it is likely to help authors write correct tests that will fail when their cleanup steps fail.

---

Since the concept of cleanup is relatively new the codebases I spend most time in haven't universally adopted it yet, but there is some existing use of defer statements for similar purposes and those tend to end up ignoring errors too in my experience, as test authors prioritize being concise. I think the addition of something like this proposal would motivate a more proactive retrofitting of the new API instead of defer in those codebases, therefore making the test cases more robust without a significant increase in verbosity.