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 M.Cleanup #42161

Closed
matttproud opened this issue Oct 23, 2020 · 10 comments
Closed

testing: add M.Cleanup #42161

matttproud opened this issue Oct 23, 2020 · 10 comments

Comments

@matttproud
Copy link
Contributor

matttproud commented Oct 23, 2020

This is a feature request/proposal for testing.M. I was doing a Readability Review today of user code. The code for one reason or another was using a custom main entrypoint. I noticed how bare the support in testing.M is for users to report and handle setup/teardown failures as well as conduct cleanups. The setup, execution, and teardown of the custom test main needed to do both of these tasks. It dawned on me that there no advice is given in the documentation about where setup error messages should go and how to correctly abort (e.g., what exit value).

I would like to propose that for similar reasons of testing.T having (*testing.T).Cleanup and (*testing.T).Fatal, testing.M should have the same.

Between the two of them, the cleanup is less important. But how to report errors and setup failures looks very relevant.

For instance, if a test main fails, should the user call log.Fatal or one of its variants, hand emit to stderr or stdout? I have seen the whole zoo of behavior before. Is any non-zero exit value OK or only some? I get that custom test mains are an advanced feature and should be seldom used, but it seems potentially like we are not providing the users an adequately documented journey. Maybe this is intentional (e.g., Go tests to support custom test harness and executor requirement contracts)? It does deserve some Schrift at the very least.

@gopherbot gopherbot added this to the Proposal milestone Oct 23, 2020
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Oct 23, 2020
@ianlancetaylor
Copy link
Contributor

ianlancetaylor commented Oct 23, 2020

It seems to me that testing.M.Cleanup can be implemented simply using defer. This isn't true for testing.T.Cleanup, because of parallel subtests, which can continue to run after the test function completes. So I don't see a strong argument for adding testing.M.Cleanup.

@matttproud
Copy link
Contributor Author

matttproud commented Oct 23, 2020

@rsc
Copy link
Contributor

rsc commented Jul 14, 2021

It sounds like we don't need to worry about an M.Cleanup until/unless we have M.Fatal etc.
And I don't know why we would have M.Fatal - is that a separate proposal?
M is intentionally low-level. It's not supposed to be something with a rich API.

@rsc rsc changed the title Proposal: Give testing.M both a Cleanup and Failure Logging Function testing: add M.Cleanup Jul 14, 2021
@matttproud
Copy link
Contributor Author

matttproud commented Jul 14, 2021

@rsc
Copy link
Contributor

rsc commented Jul 14, 2021

It's certainly easy to add a sentence or two to testing.M.

@rsc
Copy link
Contributor

rsc commented Jul 14, 2021

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Likely Decline in Proposals (old) Jul 14, 2021
@gopherbot
Copy link

gopherbot commented Jul 14, 2021

Change https://golang.org/cl/334649 mentions this issue: testing: clarify in docs that TestMain is advanced

@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Jul 21, 2021
@rsc
Copy link
Contributor

rsc commented Jul 21, 2021

No change in consensus, so declined.
— rsc for the proposal review group

steeve pushed a commit to znly/go that referenced this issue Aug 19, 2021
Beginner and intermediate Go users periodically use TestMain when
requirements do not necessitate TestMain (exceeding least-mechanism
design). This commit expands package testing's documentation to convey
that the TestMain feature itself is somewhat low-level and potentially
unsuitable for casual testing where ordinary test functions would
suffice.

Fixes golang#42161
Updates golang#44200

Change-Id: I91ba0b048c3d6f79110fe8f0fbb58d896edca366
Reviewed-on: https://go-review.googlesource.com/c/go/+/334649
Trust: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
@AndrewDK
Copy link

AndrewDK commented Mar 24, 2022

Reviving an old issue, the use case here is that deferred functions that are called in TestMain do not run if there's a panic in m.Run, even if there's a deferred recover on the stack in TestMain. Any processes started in TestMain don't get cleaned up when the test exits in this case.

@sunny-b
Copy link

sunny-b commented Oct 14, 2022

Adding my hat to this. The documentation on testing.M is a bit confusing and contradictory to being able to cleanup after functions. The main example is that m.Run returns an exit code that is meant to be used in os.Exit.

It even says so in the documentation

However, were you to actually use os.Exit, it would bypass any defer statements you made in order to cleanup.

The best way I found to get around this was to only call os.Exit if the code was not a successful exit (i.e. not 0). But that's a bit wonky IMO. The documentation should probably be a bit more clear about that.

retCode := m.Run()

if retCode != 0 {
  os.Exit(retCode)
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

No branches or pull requests

6 participants