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 TB.CleanupErr #63257

Open
mvdan opened this issue Sep 27, 2023 · 9 comments
Open

proposal: testing: add TB.CleanupErr #63257

mvdan opened this issue Sep 27, 2023 · 9 comments
Labels
Milestone

Comments

@mvdan
Copy link
Member

mvdan commented Sep 27, 2023

#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 #32111 (comment), @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 mvdan added the Proposal label Sep 27, 2023
@gopherbot gopherbot added this to the Proposal milestone Sep 27, 2023
@mvdan
Copy link
Member Author

mvdan commented Sep 27, 2023

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
Copy link
Member Author

mvdan commented Sep 27, 2023

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

@mvdan
Copy link
Member Author

mvdan commented Sep 27, 2023

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
Copy link
Contributor

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
Copy link
Member Author

mvdan commented Sep 27, 2023

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

@earthboundkid
Copy link
Contributor

earthboundkid commented Sep 27, 2023

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
Copy link
Member Author

mvdan commented Sep 27, 2023

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
Copy link
Contributor

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

defer try.F(t.Fatal)

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

@apparentlymart
Copy link

apparentlymart commented Sep 28, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

No branches or pull requests

4 participants