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: TestMain should not require os.Exit #34129

Closed
abuchanan-nr opened this issue Sep 5, 2019 · 12 comments
Closed

testing: TestMain should not require os.Exit #34129

abuchanan-nr opened this issue Sep 5, 2019 · 12 comments

Comments

@abuchanan-nr
Copy link

@abuchanan-nr abuchanan-nr commented Sep 5, 2019

TestMain is expected to call os.Exit itself. This leads to subtle issues with defer and panic that leads to much headscratching. I'm proposing that if TestMain doesn't call os.Exit, the testing framework will call it for you after TestMain returns.

Let's say you start here:

func TestMain(m *testing.M) {
  setup()
  defer teardown()
  os.Exit(m.Run())
}

Subtle bug #1: your teardown isn't running; you're making a mess of your test environment. Try this:

func TestMain(m *testing.M) {
  var exitCode int
  defer os.Exit(exitCode)

  setup()
  defer teardown()
  exitCode = m.Run()
}

Oops, setup has a panic call. Now when you run go test, it's silent..

go test .
ok  	_/Users/abuchanan/scratch/testmain_bug	0.010s

Uh, headscratch. "Why isn't go running my tests?", you ask. Oh, os.Exit is hiding my panic call. Ok, finally:

func TestMain(m *testing.M) {
  // testMain wrapper is needed to support defers and panics.
  // os.Exit will ignore those and exit silently.
  os.Exit(testMain(m))
}

func testMain(m *testing.M) int {
  setup()
  defer teardown()
  return m.Run()
}

Let's save people some headscratching (and time and effort and silly wrapper code). The testing framework probably has enough information to call os.Exit appropriately for you after TestMain returns, right? Why require users to call it?

TestMain is really useful for writing end-to-end tests, where a single setup/teardown for all the tests is important. If I break those tests into packages for organization, it would be nice not to copy that TestMain wrapper code everywhere.

@gopherbot gopherbot added this to the Proposal milestone Sep 5, 2019
@gopherbot gopherbot added the Proposal label Sep 5, 2019
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 5, 2019

Related: #9917.

One possibility, that I thought was discussed before but I can't find an issue, is to change the behavior depending on the result of TestMain. If TestMain does not return a value, assume it calls os.Exit. If TestMain returns an int, then pass that int to os.Exit.

@abuchanan-nr
Copy link
Author

@abuchanan-nr abuchanan-nr commented Sep 5, 2019

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 5, 2019

Ah, thanks.

@rsc
Copy link
Contributor

@rsc rsc commented Sep 12, 2019

This mistake does happen a lot, and the testing package that called TestMain and prepared the m can certainly record the result of m.Run for use in its own outer os.Exit.

Letting TestMain simply call m.Run instead of calling os.Exit(m.Run) seems OK and makes defer more useful.

I'm not as confident about alternate TestMain signatures; that seems unnecessary to solve this specific problem.

/cc @mpvl @robpike

@rsc
Copy link
Contributor

@rsc rsc commented Sep 25, 2019

@robpike and @mpvl, any thoughts?

@robpike
Copy link
Contributor

@robpike robpike commented Sep 26, 2019

No real insight from me. The topic seems covered by @ianlancetaylor's and @rsc's points.

@rsc
Copy link
Contributor

@rsc rsc commented Oct 2, 2019

It seems like if we do the "automatic os.Exit" that will fix quite a few buggy tests, and it basically eliminates the need for a TestMain that returns int. We could always revisit that and add it later, if needed, but let's start with the automatic os.Exit.

To be clear, the proposal is this. Right now, the overall test func main calls TestMain(m), and the TestMain function is expected to do two things:

  1. Call m.Run() and record the result.
  2. Call os.Exit with the result of m.Run.

The proposal is that:

  • m.Run records its own result in an unexported field of m, and then
  • if TestMain(m) returns, the outer test func main harness calls os.Exit with that code.

This would mean that implementations of TestMain that forget to call os.Exit will have it called for them. And implementations that want to explicitly avoid os.Exit in order to make defer useful can do that.

If TestMain does not call m.Run (presumably for a good reason) and returns, then the outer func main should os.Exit(0) as it does today.

Does anyone object to this plan or think it would not address the necessary use cases?

@rsc
Copy link
Contributor

@rsc rsc commented Oct 9, 2019

This seems like a likely accept.

Leaving open a week for final comments.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Oct 23, 2019

No new comments in the past week, so marking as accepted.

@bradfitz bradfitz changed the title proposal: TestMain should not require os.Exit testing: TestMain should not require os.Exit Oct 23, 2019
@bradfitz bradfitz modified the milestones: Proposal, Backlog Oct 23, 2019
@bradfitz bradfitz added the NeedsFix label Oct 23, 2019
@gopherbot
Copy link

@gopherbot gopherbot commented Feb 16, 2020

Change https://golang.org/cl/219639 mentions this issue: testing: do not require os.Exit in TestMain

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 28, 2020

Change https://golang.org/cl/221477 mentions this issue: go/packages: drop imports of reflect in tests of import graph

gopherbot pushed a commit to golang/tools that referenced this issue Feb 28, 2020
In CL 219639 the generated testing package gets a dependency on reflect.
Trim that dependency in the go/packages tests, just as we trim
other dependencies of the generated testing package.

Updates golang/go#34129

Change-Id: I5e2578fc61cafb4d524d0a58525cf78a402c1143
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221477
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 1, 2020

Change https://golang.org/cl/221657 mentions this issue: go/packages: drop pruned packages in tests of import graph

gopherbot pushed a commit to golang/tools that referenced this issue Mar 10, 2020
Explicitly listing the pruned packages ties the test to the details of
how the testing package is implemented. That shouldn't matter, and it
changes in CL 219639. This change makes it possible to work whether or
not CL 219639 is committed.

Updates golang/go#34129

Change-Id: I45875dadeaac42a1002e1329a1a762e340c5352c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221657
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Changkun Ou <euryugasaki@gmail.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
@gopherbot gopherbot closed this in 2f54081 Mar 17, 2020
sprql added a commit to sprql/mainflux that referenced this issue Apr 14, 2020
Defers will not be run when using os.Exit (golang/go#34129)

Signed-off-by: Alexander Obukhov <dev@sprql.space>
manuio pushed a commit to mainflux/mainflux that referenced this issue Apr 14, 2020
Defers will not be run when using os.Exit (golang/go#34129)

Signed-off-by: Alexander Obukhov <dev@sprql.space>
manuio pushed a commit to mainflux/mainflux that referenced this issue Oct 12, 2020
Defers will not be run when using os.Exit (golang/go#34129)

Signed-off-by: Alexander Obukhov <dev@sprql.space>
jayconrod pushed a commit to bazelbuild/rules_go that referenced this issue Nov 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants