code review 55780043: testing: diagnose buggy tests that panic(nil)

testing: diagnose buggy tests that panic(nil)

Fixes issue 6546.

[rsc, 2014-01-22 20:18:59]

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

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

[dave_cheney.net, 2014-01-22 20:24:44]

LGTM. This needs an entry in the 1.3 release notes.


On Thu, Jan 23, 2014 at 7:19 AM, <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:
> testing: diagnose buggy tests that panic(nil)
>
> Fixes issue 6546.
>
> Please review this at https://codereview.appspot.com/55780043/
>
> Affected files (+7, -1 lines):
>   M src/pkg/testing/testing.go
>
>
> Index: src/pkg/testing/testing.go
> ===================================================================
> --- a/src/pkg/testing/testing.go
> +++ b/src/pkg/testing/testing.go
> @@ -376,10 +376,15 @@
>         // returned normally or because a test failure triggered
>         // a call to runtime.Goexit, record the duration and send
>         // a signal saying that the test is done.
> +       var finished bool
>         defer func() {
>                 t.duration = time.Now().Sub(t.start)
>                 // If the test panicked, print any test output before
> dying.
> -               if err := recover(); err != nil {
> +               err := recover()
> +               if !finished && err == nil {
> +                       err = fmt.Errorf("test executed panic(nil)")
> +               }
> +               if err != nil {
>                         t.Fail()
>                         t.report()
>                         panic(err)
> @@ -389,6 +394,7 @@
>
>         t.start = time.Now()
>         test.F(t)
> +       finished = true
>  }
>
>  // An internal function but exported because it is cross-package; part of
> the implementation
>
>
> --
> 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/groups/opt_out.
>

[rsc, 2014-01-22 20:25:34]

There is a seemingly interminable discussion on issue 6546 about this. My
goal here is primarily to end that discussion. It seems clear that a test
that calls panic(nil) should not result in a PASS, which is what happens
today.

[bradfitz, 2014-01-22 20:31:10]

LGTM



On Wed, Jan 22, 2014 at 12:19 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:
> testing: diagnose buggy tests that panic(nil)
>
> Fixes issue 6546.
>
> Please review this at https://codereview.appspot.com/55780043/
>
> Affected files (+7, -1 lines):
>   M src/pkg/testing/testing.go
>
>
> Index: src/pkg/testing/testing.go
> ===================================================================
> --- a/src/pkg/testing/testing.go
> +++ b/src/pkg/testing/testing.go
> @@ -376,10 +376,15 @@
>         // returned normally or because a test failure triggered
>         // a call to runtime.Goexit, record the duration and send
>         // a signal saying that the test is done.
> +       var finished bool
>         defer func() {
>                 t.duration = time.Now().Sub(t.start)
>                 // If the test panicked, print any test output before
> dying.
> -               if err := recover(); err != nil {
> +               err := recover()
> +               if !finished && err == nil {
> +                       err = fmt.Errorf("test executed panic(nil)")
> +               }
> +               if err != nil {
>                         t.Fail()
>                         t.report()
>                         panic(err)
> @@ -389,6 +394,7 @@
>
>         t.start = time.Now()
>         test.F(t)
> +       finished = true
>  }
>
>  // An internal function but exported because it is cross-package; part of
> the implementation
>
>
> --
> 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/groups/opt_out.
>

[r, 2014-01-22 20:51:26]

LGTM

[rsc, 2014-01-22 21:04:54]

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

testing: diagnose buggy tests that panic(nil)

Fixes issue 6546.

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