Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

testing: should panic(nil) cause a test to fail? #6546

Closed
gopherbot opened this issue Oct 8, 2013 · 25 comments

Comments

Projects
None yet
5 participants
@gopherbot
Copy link

commented Oct 8, 2013

by elazarl:

package main_test

import "testing"

func TestMe(t *testing) {
    panic(nil)
    t.FailNow()
}

What is the expected output?

Test should fail or panic


What do you see instead?

Test pass


Which compiler are you using (5g, 6g, 8g, gccgo)?

8g


Which operating system are you using?

Mac OS X


Which version are you using?  (run 'go version')

1.1, same in 1.2

Please provide any additional information below.

panic(nil) makes little sense actually, since you cannot distinguish whether a recover()
has no panic, or a nil panic. At any rate, we should make sure panic(nil) never occurred
during testing, in case someone mistakenly throw a nil panic during the test.
@cznic

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2013

Comment 1:

panic(nil) makes perfect sense, ie. the behavior of panic(nil) wrt recover() is well
defined in the specs. (I actually used it, although not often, to run finalizing code
common to the normal and panic paths - instead of eg. a goto.)
#WorkingAsIntended
@gopherbot

This comment has been minimized.

Copy link
Author

commented Oct 8, 2013

Comment 2 by elazarl:

Regardless of the question whether or not panic(nil) makes sense. The behavior of
panic(nil) inside testing does not make sense.
I see zero value with having panic(nil) mark test as successful. If you really want to
enable making tests successful, have a t.SucceedNow() function that would internally
throw "type succeedNowPanic bool", and parse that.
I think it's obvious that:
    if expected == actual {
        panic(nil)
    }
is not readable, so I see no value with supporting that, and I think it's obvious that
if someone panics with nil by mistake, he wants the tests to catch that.
Therefor I see little value with "testing" regarding panic(nil) as "SuccessNow()". I
don't think the docs imply this behavior should be supported.
@cznic

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2013

Comment 3:

Testing doesn't know anything whatsoever about panic(nil). Testing wraps (guessing) the
test in a defer statement like
        if whatever := recover(); whatever != nil {
                // handle test failed due to panic
        }
The specification mandates that the "handle ..." part is not executed if the test
"returns" by panic(nil).
IOW, package testing does the right thing and an intentional use of panic(nil)
(orthogonal to if in test or elsewhere) does also the right thing.
PS: Readability is subjective, but IMHO, your example is clearly readable for a person
knowing how the combo panic/recover is specified to work. And that it can make sense
even in tests was discussed previously. Such use is correct (in accordance with the
specs) and is semantically valid (does what it's demanded to do).
@adg

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2013

Comment 4:

If I ever saw panic(nil) I would think "That code is busted," or, if I were feeling more
charitable, "This code is doing something highly unusual, I'd better read carefully and
check my assumptions." If you're the author of the line "panic(nil)" then you'd better
have a good reason for writing it, and should not be surprised when things behave
strangely (like making a test pass).
> if someone panics with nil by mistake, he wants the tests to catch that.
There is no reasonable for package testing to check for this. Just don't do it.
Maybe we could write a module for "go vet" that finds and warns about "panic(nil)", but
I've never seen it before and doubt it's a widespread problem.

Status changed to WorkingAsIntended.

@gopherbot

This comment has been minimized.

Copy link
Author

commented Oct 8, 2013

Comment 5 by elazarl:

@adg,
Just to clarify, I didn't mean someone writing panic(nil) by mistake. I meant someone
doing something 
like:
    e, err := foo()
    if err != nil {
        panic(e) //oops
    }
In application code.
Then his tests pass by mistake, and it's tricky to understand what have happened.
Of course, go vet cannot really find that.
I really see no reason not safeguarding against this behavior. panic'ing on the wrong
value is definitely possible, and I see no reason to assume it's not common enough to
guard against.
Note that this is a high cost mistake, as tests would silently pass.
@adg

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2013

Comment 6:

> I really see no reason not safeguarding against this behavior.
How do you propose we do it?
@gopherbot

This comment has been minimized.

Copy link
Author

commented Oct 8, 2013

Comment 7 by elazarl:

Before running a test set a canary value that must be set in order to mark this test as
successful, to make sure no panics were thrown, check for recover()'s return value and
for the canary value.
Something like:
    - defer func() {
    -     if recover() != nil {
    -         failTest()
    -     }
    - }()
    - runTest()
    + var noPanic = false
    + defer func() {
    +     if recover() != nil && noPanic {
    +         failTest()
    +     }
    + }()
    + runTest()
    + noPanic = true
any panic in the actual test will prevent noPanic's value from changing and would
eventually fail the test.
I can look at the actual code and propose an actual patch.
@adg

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2013

Comment 8:

Also, your example only fits when e is an interface value, which is not often the case.
Can you give a real world example of this happening? It seems like a very, very minor
issue.
@adg

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2013

Comment 9:

Also, your example only fits when e is an interface value, which is not often the case.
Can you give a real world example of this happening? It seems like a very, very minor
issue.
@cznic

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2013

Comment 10:

@7: Ad "I can look at the actual code and propose an actual patch."
I object to changing the existing testing package behavior. AFAICS, the patch would
clearly breach the compatibility promise.
@adg

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2013

Comment 11:

Look at the actual code. The reason I am pushing back is because it's already pretty
complex, and I don't see a compelling reason to make it more so.
@gopherbot

This comment has been minimized.

Copy link
Author

commented Oct 8, 2013

Comment 12 by elazarl:

@adg, have a look here https://golang.org/cl/14535044 very simple, and almost
identical to the pseudo-code I've written.
Your second point is a good point (though I tend to have nil interfaces in my code that
can find their way to a panic...). But my point is, the probability of error is smaller,
but the damage and frustration are big, so overall the benefit expectation from the
change is positive IMHO. (all this issue reminds me a bug in an embedded system I worked
on, where someone mistakenly written to 0x3, and the bug stayed undetected for years
until I ported parts of code to run on a PC). It would definitely make the testing
package more in line with the principle of least surprise for a new user.
If my simple patch is indeed good, I think it has a good cost/benefit ratio.
@Jan,
Re, compatibility promise, I believe the behavior of the testing library when throwing
nil interface is not specified anywhere, and I really doubt it that someone relies on
this behavior. If you know otherwise - do let us know.
@cznic

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2013

Comment 13:

@12: I think that sending a CL after the issue was closed is not the way things are
supposed to work.
Wrt recover() != nil, once again, it works _everywhwere_ as specified. Compatibility
promise is not about "if you know about code it can break". It's about not changing
behavior of existing things that are not bugs (not broken), as one can never know where
a code, that would get broken, can exists and if it exists at all. But this is _not_ a
bug. Not in the compiler, not in package testing.
Example of panic(nil) being used to error-lessly return from an outer function from
within a local function:
https://github.com/cznic/tmp/blob/7cdc7c6d85664e1fa120dde4a2b6c008a73a56f0/q/tests.go#L1401
@gopherbot

This comment has been minimized.

Copy link
Author

commented Oct 8, 2013

Comment 14 by elazarl:

@Jan,
CL sent per adg's request.
Discussion of general panic(nil) mechanism is out of the scope of this bug.
If I understand you correctly there's no specific promise for supporting this behavior
of the testing package, however you believe that regarding to panic on nil interface as
indication of a successful test is an implicit contract of the testing package. Thanks.
@cznic

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2013

Comment 15:

I cannot see the any such adg's request anywhere in this issue.
panic(nil) has nothing to do with specially only testing. It's a correct and valid
statement and it is actually used by at least one  person on this planet. (Note: the
previous example if mine predates this issue.) The behavior of it is well defined. If
someone somewhere wrote a function which exits, for whatever reason, in the non-error
case by panic(nil) (hopefully it's a non exported function in that case), then your
patch would break the tests of that person's tests of that function.
The current behavior is not a bug, ie. that would clearly mean a clear breach of the
compatibility promise.
Please feel free to comment further, but I think I've said everything worth of it
already. Twice ;-)
@adg

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2013

Comment 16:

@Jan: I don't think you should be relying on panic(nil) to exit a test successfully.
@cznic

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2013

Comment 17:

@adg: The earlier linked example function is not called directly from package testing.
The panic(nil) is just a way how to stop the function from further executing (when
executing a nested function). IOW, the discussed example doesn't/cannot run into the
same problem as Elazar's code is.
The panic(nil) in the example is used like a fast-return-path from a recursive descent
parser sometimes is. It can be done w/o panic (or exceptions in some other language),
but "bubbling" the stop message through all the levels up is a PITA.
I've checked all my code, I have no other panic(nil) in any tested directly by package
testing, function, so no, I'm fortunately not relying on panic(nil) to exit a test
successfully ;-)
@adg

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2013

Comment 18:

The change is indeed pretty simple. Let's discuss it further once Go 1.2 is out the door.

Labels changed: added priority-later, removed priority-triage.

Status changed to Thinking.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2013

Comment 19:

Labels changed: added go1.3maybe.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2013

Comment 20:

Labels changed: added release-none, removed go1.3maybe.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2013

Comment 21:

Labels changed: added repo-main.

@btracey

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2014

Comment 22:

For what it's worth, I just encountered this issue in writing my own tests. If it hadn't
been for the cover profile tool, I don't think I would have ever noticed it. Here is a
proxy for the code I wrote, and has misplaced the if throwPanic clause
http://play.golang.org/p/qfE_NPcNAK
@rsc

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2014

Comment 23:

This issue was closed by revision ae56210.

Status changed to Fixed.

@cznic

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2014

Comment 24:

Some of our tests are now broken and must be rewritten.
@adg

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2014

Comment 25:

Sorry for the inconvenience, Jan.

@golang golang locked and limited conversation to collaborators Jun 25, 2016

This issue was closed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.