code review 5651081: testing: do not squelch panics

testing: use runtime/debug to format panics

Among other things, this avoids putting a testing.go:nnn:
prefix on every line of the stack trace.

[rsc, 2012-02-13 03:32:39]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[r2, 2012-02-13 03:40:13]

i don't understand the purpose of this change.

-rob

[dsymonds, 2012-02-13 03:42:36]

On Mon, Feb 13, 2012 at 2:40 PM, Rob 'Commander' Pike <r@google.com> wrote:

> i don't understand the purpose of this change.

Nor I. The code you're disabling markedly improved the test output if
code panics.


Dave.

[rsc, 2012-02-13 04:18:58]

On Sun, Feb 12, 2012 at 22:40, Rob 'Commander' Pike <r@google.com> wrote:
> i don't understand the purpose of this change.

The panics it is printing right now are impossible to read,
because they say testing.go:123: and testing.go:124:
at the beginning of alternating lines, for some value of 123.
I fixed that by using debug.Stack, which should have been
used anyway.

However, even then the panics are not very good.
The built-in panic does a much better job of showing
useful output and stopping the program.  I think that
should be the default, and that the CL that put this
recover in was a mistake.  Even so, I left the code with
if false in case people wanted it.

The recover is catching a panic in the main test goroutine
but not in goroutines that the test itself starts.  Why should
that one goroutine be different?  Why not just let it panic
and get the message everyone is used to?

Russ

[r2, 2012-02-13 04:22:23]

i leave this to you. the panics were caught when people wanted them caught. they
were printed when people wanted more output. now we're back to doing nothing. i
don't know what people want.

but i do know that i would prefer not to add a poor dependency to the testing
package under ordinary circumstances, let alone inside code that is optimized
away.  please make this a comment or delete it altogether, don't put it in an if
false clause.

-rob

[dsymonds, 2012-02-13 04:24:58]

On Mon, Feb 13, 2012 at 3:18 PM, Russ Cox <rsc@golang.org> wrote:

> On Sun, Feb 12, 2012 at 22:40, Rob 'Commander' Pike <r@google.com> wrote:
>> i don't understand the purpose of this change.
>
> The panics it is printing right now are impossible to read,
> because they say testing.go:123: and testing.go:124:
> at the beginning of alternating lines, for some value of 123.
> I fixed that by using debug.Stack, which should have been
> used anyway.

We were deliberately avoiding the dependency on runtime/debug, which
is where I copied the code from in the first place.

> However, even then the panics are not very good.
> The built-in panic does a much better job of showing
> useful output and stopping the program.  I think that
> should be the default, and that the CL that put this
> recover in was a mistake.  Even so, I left the code with
> if false in case people wanted it.

It also throws away any t.Log/t.Error messages that had been buffered
up to that point, which are often valuable in tracking down why the
code is panicking. Worse, it allows the test to pass if there were no
t.Error calls before the time of the panic.

> The recover is catching a panic in the main test goroutine
> but not in goroutines that the test itself starts.  Why should
> that one goroutine be different?  Why not just let it panic
> and get the message everyone is used to?

Most test code doesn't start separate goroutines; this isn't trying to
be a 100% solution, but a solution that helps in the vast majority of
cases.


Dave.

[dsymonds, 2012-02-13 04:29:28]

On Mon, Feb 13, 2012 at 3:24 PM, David Symonds <dsymonds@golang.org> wrote:

> Worse, it allows the test to pass if there were no
> t.Error calls before the time of the panic.

Actually, forget this sentence. I was misremembering.

[rsc, 2012-02-13 04:36:03]

PTAL

I put the recover back for now, but I left the use of runtime/debug.
It's a useful package and it leaves the stack trace formatting
(which is subtle and might improve) in a single place.

[r, 2012-02-13 04:37:55]

LGTM
but officially annoyed by new dependency

[dsymonds, 2012-02-13 04:38:49]

LGTM

okay

[rsc, 2012-02-13 04:39:43]

*** Submitted as http://code.google.com/p/go/source/detail?r=1a565b406af7 ***

testing: use runtime/debug to format panics

Among other things, this avoids putting a testing.go:nnn:
prefix on every line of the stack trace.

R=golang-dev, r, dsymonds, r
CC=golang-dev
http://codereview.appspot.com/5651081