testing: TestMain should not require os.Exit #34129

abuchanan-nr · 2019-09-05T22:01:51Z

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.

The text was updated successfully, but these errors were encountered:

ianlancetaylor · 2019-09-05T23:09:54Z

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 · 2019-09-05T23:15:16Z

#23404

ianlancetaylor · 2019-09-05T23:17:50Z

Ah, thanks.

rsc · 2019-09-12T21:19:21Z

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 · 2019-09-25T17:33:05Z

@robpike and @mpvl, any thoughts?

robpike · 2019-09-26T00:07:55Z

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

rsc · 2019-10-02T17:31:14Z

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:

Call m.Run() and record the result.
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 · 2019-10-09T17:52:59Z

This seems like a likely accept.

Leaving open a week for final comments.

bradfitz · 2019-10-23T17:09:20Z

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

gopherbot · 2020-02-16T13:54:08Z

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

gopherbot · 2020-02-28T01:22:47Z

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

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 · 2020-03-01T02:17:59Z

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

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>

Defers will not be run when using os.Exit (golang/go#34129) Signed-off-by: Alexander Obukhov <dev@sprql.space>

Based on golang/go#34129 and https://golang.org/cl/219639

gopherbot added this to the Proposal milestone Sep 5, 2019

gopherbot added the Proposal label Sep 5, 2019

andybons mentioned this issue Sep 12, 2019

proposal: review meeting minutes #33502

Open

rsc added the Proposal-FinalCommentPeriod label Oct 9, 2019

bradfitz added Proposal-Accepted and removed Proposal-FinalCommentPeriod labels Oct 23, 2019

bradfitz changed the title ~~proposal: TestMain should not require os.Exit~~ testing: TestMain should not require os.Exit Oct 23, 2019

bradfitz modified the milestones: Proposal, Backlog Oct 23, 2019

bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 23, 2019

bcmills mentioned this issue Feb 18, 2020

testing: when using a custom TestMain, m.Run does not return if one of the tests it runs panics #37206

Open

changkun mentioned this issue Feb 24, 2020

cmd/vet or golint: Catch TestMain without os.Exit #9917

Closed

gopherbot closed this as completed in 2f54081 Mar 17, 2020

sprql added a commit to sprql/mainflux that referenced this issue Apr 14, 2020

NOISSUE - Remove defers from TestMain

894488b

Defers will not be run when using os.Exit (golang/go#34129) Signed-off-by: Alexander Obukhov <dev@sprql.space>

sprql mentioned this issue Apr 14, 2020

NOISSUE - Remove defers from TestMain absmach/magistrala#1111

Merged

manuio pushed a commit to absmach/magistrala that referenced this issue Apr 14, 2020

NOISSUE - Remove defers from TestMain (#1111)

fbbf2c0

Defers will not be run when using os.Exit (golang/go#34129) Signed-off-by: Alexander Obukhov <dev@sprql.space>

shibumi mentioned this issue Jul 28, 2020

Test clean up does not work due to deferring functions not being called in-toto/in-toto-golang#60

Closed

manuio pushed a commit to absmach/magistrala that referenced this issue Oct 12, 2020

NOISSUE - Remove defers from TestMain (#1111)

7d16dfb

Defers will not be run when using os.Exit (golang/go#34129) Signed-off-by: Alexander Obukhov <dev@sprql.space>

farazdagi mentioned this issue Nov 10, 2020

Remove redundant calls to os.exit() in TestMain prysmaticlabs/prysm#7761

Merged

rabbbit mentioned this issue Nov 13, 2020

Support go1.15s non Os.Exit TestMain bazelbuild/rules_go#2716

Merged

jayconrod pushed a commit to bazelbuild/rules_go that referenced this issue Nov 17, 2020

go_test: don't require TestMain to call os.Exit (#2716)

b01b09e

Based on golang/go#34129 and https://golang.org/cl/219639

jayconrod pushed a commit to bazelbuild/rules_go that referenced this issue Dec 2, 2020

go_test: don't require TestMain to call os.Exit (#2716)

a59f6a1

Based on golang/go#34129 and https://golang.org/cl/219639

golang locked and limited conversation to collaborators Mar 17, 2021

gopherbot added the FrozenDueToAge label Mar 17, 2021

bcmills modified the milestones: Backlog, Go1.15 Dec 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

testing: TestMain should not require os.Exit #34129

testing: TestMain should not require os.Exit #34129

abuchanan-nr commented Sep 5, 2019

ianlancetaylor commented Sep 5, 2019

abuchanan-nr commented Sep 5, 2019

ianlancetaylor commented Sep 5, 2019

rsc commented Sep 12, 2019 •

edited

rsc commented Sep 25, 2019

robpike commented Sep 26, 2019

rsc commented Oct 2, 2019

rsc commented Oct 9, 2019

bradfitz commented Oct 23, 2019

gopherbot commented Feb 16, 2020

gopherbot commented Feb 28, 2020

gopherbot commented Mar 1, 2020

testing: TestMain should not require os.Exit #34129

testing: TestMain should not require os.Exit #34129

Comments

abuchanan-nr commented Sep 5, 2019

ianlancetaylor commented Sep 5, 2019

abuchanan-nr commented Sep 5, 2019

ianlancetaylor commented Sep 5, 2019

rsc commented Sep 12, 2019 • edited

rsc commented Sep 25, 2019

robpike commented Sep 26, 2019

rsc commented Oct 2, 2019

rsc commented Oct 9, 2019

bradfitz commented Oct 23, 2019

gopherbot commented Feb 16, 2020

gopherbot commented Feb 28, 2020

gopherbot commented Mar 1, 2020

rsc commented Sep 12, 2019 •

edited