code review 55790043: testing: fix SkipNow and FailNow to avoid panic(nil) check

testing: fix SkipNow and FailNow to avoid panic(nil) check

Sorry, too many windows in which to run all.bash.
Fixes build.

[rsc, 2014-01-22 21:34:00]

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

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

[rsc, 2014-01-22 21:34:05]

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

testing: fix SkipNow and FailNow to avoid panic(nil) check

Sorry, too many windows in which to run all.bash.
Fixes build.

TBR=r
CC=golang-codereviews
https://codereview.appspot.com/55790043

[bradfitz, 2014-01-22 21:36:42]

LGTM



On Wed, Jan 22, 2014 at 1:34 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: fix SkipNow and FailNow to avoid panic(nil) check
>
> Sorry, too many windows in which to run all.bash.
> Fixes build.
>
> Please review this at https://codereview.appspot.com/55790043/
>
> Affected files (+10, -8 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
> @@ -143,10 +143,11 @@
>  // common holds the elements common between T and B and
>  // captures common methods such as Errorf.
>  type common struct {
> -       mu      sync.RWMutex // guards output and failed
> -       output  []byte       // Output generated by test or benchmark.
> -       failed  bool         // Test or benchmark has failed.
> -       skipped bool         // Test of benchmark has been skipped.
> +       mu       sync.RWMutex // guards output and failed
> +       output   []byte       // Output generated by test or benchmark.
> +       failed   bool         // Test or benchmark has failed.
> +       skipped  bool         // Test of benchmark has been skipped.
> +       finished bool
>
>         start    time.Time // Time test or benchmark started
>         duration time.Duration
> @@ -275,6 +276,7 @@
>         // it would run on a test failure.  Because we send on c.signal
> during
>         // a top-of-stack deferred function now, we know that the send
>         // only happens after any other stacked defers have completed.
> +       c.finished = true
>         runtime.Goexit()
>  }
>
> @@ -338,6 +340,7 @@
>  // those other goroutines.
>  func (c *common) SkipNow() {
>         c.skip()
> +       c.finished = true
>         runtime.Goexit()
>  }
>
> @@ -376,13 +379,12 @@
>         // 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.
>                 err := recover()
> -               if !finished && err == nil {
> -                       err = fmt.Errorf("test executed panic(nil)")
> +               if !t.finished && err == nil {
> +                       err = fmt.Errorf("test executed panic(nil) or
> runtime.Goexit")
>                 }
>                 if err != nil {
>                         t.Fail()
> @@ -394,7 +396,7 @@
>
>         t.start = time.Now()
>         test.F(t)
> -       finished = true
> +       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 21:38:05]

LGTM