code review 148770043: cmd/go, testing: add TestMain support

cmd/go, testing: add TestMain support

Fixes issue 8202.

[rsc, 2014-09-18 23:39:27]

Hello r (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[r, 2014-09-19 00:01:42]

https://codereview.appspot.com/148770043/diff/40001/src/testing/testing.go
File src/testing/testing.go (right):

https://codereview.appspot.com/148770043/diff/40001/src/testing/testing.go#ne...
src/testing/testing.go:137: //	func TestMain(m *testing.M) { os.Exit(m.Run()) }
i don't see how this helps much. the fields of m are private so i can't, for
instance, loop over the tests doing setup and teardown for each test.

[rsc, 2014-09-19 00:21:16]

On Thu, Sep 18, 2014 at 8:01 PM, <r@golang.org> wrote:

>
> https://codereview.appspot.com/148770043/diff/40001/src/testing/testing.go
> File src/testing/testing.go (right):
>
> https://codereview.appspot.com/148770043/diff/40001/src/
> testing/testing.go#newcode137
> src/testing/testing.go:137: //  func TestMain(m *testing.M) {
> os.Exit(m.Run()) }
> i don't see how this helps much. the fields of m are private so i can't,
> for instance, loop over the tests doing setup and teardown for each
> test.
>

Setup and teardown for each test can be inside the test.
What people have been asking for is setup and teardown for the whole
executable run:

func TestMain(m *testing.M) {
    initDBServer()
    code := m.Run()
    cleanupDBServer()
    os.Exit(code)
}

The other thing that has come up repeatedly is being able to go back and
run something on the main thread, because some bad C libraries need that.
This lets them move the test loop off the main thread:

func init() { runtime.LockOSThread() } // locks main goroutine to main
thread

func TestMain(m *testing.M) {
    // on main thread. run testing elsewhere
    go func() {
        os.Exit(m.Run())
    }()
    // do whatever it is that has to be available on the main thread during
the tests...
    ...
}

[josharian, 2014-09-19 00:58:33]

https://codereview.appspot.com/148770043/diff/40001/src/cmd/go/test.go
File src/cmd/go/test.go (right):

https://codereview.appspot.com/148770043/diff/40001/src/cmd/go/test.go#newcod...
src/cmd/go/test.go:1068: len(fn.Type.Params.List[0].Names) > 1 {
To be thorough, should check fn.Recv as well.

[rsc, 2014-09-19 01:17:08]

https://codereview.appspot.com/148770043/diff/40001/src/cmd/go/test.go
File src/cmd/go/test.go (right):

https://codereview.appspot.com/148770043/diff/40001/src/cmd/go/test.go#newcod...
src/cmd/go/test.go:1068: len(fn.Type.Params.List[0].Names) > 1 {
On 2014/09/19 00:58:33, josharian wrote:
> To be thorough, should check fn.Recv as well.

fn.Recv was checked by the caller

[josharian, 2014-09-19 01:31:23]

Just gave this a whirl, hit a compilation failure (see below). That makes me
think this could use a test--maybe an internal package containing just a test,
perhaps along the lines of http://play.golang.org/p/LtI2opTsoR.

https://codereview.appspot.com/148770043/diff/40001/src/cmd/go/test.go
File src/cmd/go/test.go (right):

https://codereview.appspot.com/148770043/diff/40001/src/cmd/go/test.go#newcod...
src/cmd/go/test.go:1068: len(fn.Type.Params.List[0].Names) > 1 {
On 2014/09/19 01:17:08, rsc wrote:
> On 2014/09/19 00:58:33, josharian wrote:
> > To be thorough, should check fn.Recv as well.
> 
> fn.Recv was checked by the caller

Acknowledged.

https://codereview.appspot.com/148770043/diff/40001/src/cmd/go/test.go#newcod...
src/cmd/go/test.go:1237: "os"
Only import os when there's no TestMain, on pain of compilation failure.

[rsc, 2014-09-19 01:39:53]

https://codereview.appspot.com/148770043/diff/40001/src/cmd/go/test.go
File src/cmd/go/test.go (right):

https://codereview.appspot.com/148770043/diff/40001/src/cmd/go/test.go#newcod...
src/cmd/go/test.go:1237: "os"
On 2014/09/19 01:31:23, josharian wrote:
> Only import os when there's no TestMain, on pain of compilation failure.

Done, and added test.

[r, 2014-09-19 03:16:45]

LGTM

i don't like it but during several hours of comcast outage i couldn't think of a
better approach.

i expect pressure to add features to testing.M. i will push back on them very
hard.

[r, 2014-09-19 03:17:40]

also go1.4.txt

On Thu, Sep 18, 2014 at 8:16 PM,  <r@golang.org> wrote:
> LGTM
>
> i don't like it but during several hours of comcast outage i couldn't
> think of a better approach.
>
> i expect pressure to add features to testing.M. i will push back on them
> very hard.
>
> https://codereview.appspot.com/148770043/

[bradfitz, 2014-09-19 15:03:27]

LGTM


On Thu, Sep 18, 2014 at 7:39 PM, <rsc@golang.org> wrote:

> Reviewers: r,
>
> Message:
> Hello r (cc: golang-codereviews@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> cmd/go, testing: add TestMain support
>
> Fixes issue 8202.
>
> Please review this at https://codereview.appspot.com/148770043/
>
> Affected files (+93, -7 lines):
>   M src/cmd/go/test.go
>   M src/testing/testing.go
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[rsc, 2014-09-19 17:51:13]

*** Submitted as https://code.google.com/p/go/source/detail?r=804cc55d6b47 ***

cmd/go, testing: add TestMain support

Fixes issue 8202.

LGTM=r, bradfitz
R=r, josharian, bradfitz
CC=golang-codereviews
https://codereview.appspot.com/148770043

[pcc, 2014-11-06 16:01:11]

On 2014/09/19 17:51:13, rsc wrote:
> *** Submitted as https://code.google.com/p/go/source/detail?r=804cc55d6b47 ***
> 
> cmd/go, testing: add TestMain support
> 
> Fixes issue 8202.
> 
> LGTM=r, bradfitz
> R=r, josharian, bradfitz
> CC=golang-codereviews
> https://codereview.appspot.com/148770043

This broke "go test -compiler gccgo" with llgo (and presumably gccgo):

[...]/_test/_testmain.go:52:7: MainStart not declared by package testing

Could we perhaps add some version checks to this code for compatibility with
go1.3 compilers?

Over in https://code.google.com/p/go/issues/detail?id=8851 we were discussing
how "go" should learn the compiler's Go release tags, which may be useful for
this.

[rsc, 2014-11-06 17:29:01]

I'm sorry about the breakage, but llgo and gccgo should use a go command
built for the same version of Go that they support.


This is the Go 1.4 go command. It expects Go 1.4 underneath it. I don't
want to start supporting all older versions in each go command.

Russ