smartystreets
panicing code results in tests passing?
Hello, using latest goconvey (just updated from master)
$ go version ⏎
go version go1.10.1 linux/amd64
on this code base https://github.com/grafana/metrictank :
$ pwd
/home/dieter/go/src/github.com/grafana/metrictank
$ grep -R MustMKeyFromString .
./test/key.go:func MustMKeyFromString(id string) schema.MKey {
./idx/memory/memory_test.go: So(test.MustMKeyFromString(series[0].Id), ShouldBeIn, deletedIds)
./idx/memory/memory_test.go: So(test.MustMKeyFromString(series[1].Id), ShouldBeIn, deletedIds)
$
note that the function is question is buggy, the definition looks like so:
func MustMKeyFromString(id string) schema.MKey {
mkey, err := schema.MKeyFromString(id)
if err != nil {
panic(err)
}
panic(err)
return mkey
}
due to the extra panic statement, there is no way this function returns without panicing.
yet, when invoking a test method that uses this function, it passes:
$ go test -v -run=TestMixedBranchLeafDelete .
=== RUN TestMixedBranchLeafDelete
when deleting mixed leaf/branch ✔✔
2 total assertions
when deleting from branch ✔✔
deleted series should not be present in the metricDef index ✔
deleted series should not be present in searches ✔✔✔✔
9 total assertions
when deleting mixed leaf/branch ✔✔
11 total assertions
when deleting from branch ✔✔
deleted series should not be present in the metricDef index ✔
deleted series should not be present in searches ✔✔✔✔
18 total assertions
--- PASS: TestMixedBranchLeafDelete (0.00s)
PASS
ok github.com/grafana/metrictank/idx/memory 0.004s
I can trivially prove the panic is being triggered by changing the definition to
func MustMKeyFromString(id string) schema.MKey {
mkey, err := schema.MKeyFromString(id)
if err != nil {
panic(err)
}
fmt.Println("PANIC HERE!!!")
panic(err)
return mkey
}
the "PANIC HERE" messages show in the output and the test passes:
$ go test -v -run=TestMixedBranchLeafDelete .
=== RUN TestMixedBranchLeafDelete
when deleting mixed leaf/branch ✔✔PANIC HERE!!!
2 total assertions
when deleting from branch ✔✔
deleted series should not be present in the metricDef index ✔
deleted series should not be present in searches ✔✔✔✔
9 total assertions
when deleting mixed leaf/branch ✔✔PANIC HERE!!!
11 total assertions
when deleting from branch ✔✔
deleted series should not be present in the metricDef index ✔
deleted series should not be present in searches ✔✔✔✔
18 total assertions
--- PASS: TestMixedBranchLeafDelete (0.00s)
PASS
ok github.com/grafana/metrictank/idx/memory 0.004s
I tried to reproduce this in a standalone case, but
package main
import (
. "github.com/smartystreets/goconvey/convey"
"testing"
)
func getInt() int {
panic("panic")
return 1
}
func TestHelloWorld(t *testing.T) {
list := []int{1}
Convey("1", t, func() {
Convey("2", func() {
So(getInt(), ShouldBeIn, list)
})
})
}
this correctly shows the panic and fails the test. i thought maybe it was because i use a custom struct type, but also the following example shows the panic and fails like it should:
package main
import (
. "github.com/smartystreets/goconvey/convey"
"testing"
)
type myType struct {
b byte
}
func getInt() myType {
panic("panic")
return myType{1}
}
func TestHelloWorld(t *testing.T) {
list := []myType{myType{1}}
Convey("1", t, func() {
Convey("2", func() {
So(getInt(), ShouldBeIn, list)
})
})
}
so I can't provide a simple example case, however I can say that if you checkout github.com/grafana/metrictank, look at the defintion of MustMKeyFromString and the TestMixedBranchLeafDelete test method which calls that function, it should be able to reproduce
note: i have just removed the bogun panic in https://github.com/grafana/metrictank/pull/917 , so to reproduce you need a version older than that, such as 0.9.0 (git checkout 0.9.0)
duplicate of #98 - you're effectively calling panic(nil), which is not distinguishable from not calling panic() at all.
Go 2 Proposal for dealing with panic(nil): https://github.com/golang/go/issues/25448
actually, i imagine the answer will probably be no, but would it be possible for go-convey not to recover, that way if user code calls panic(nil) the failure will at least be obvious.
Well, 10 years later with the release of Go 1.21, we finally have closure!
Go 1.21 now defines that if a goroutine is panicking and recover was called directly by a deferred function, the return value of recover is guaranteed not to be nil. To ensure this, calling panic with a nil interface value (or an untyped nil) causes a run-time panic of type *runtime.PanicNilError.
https://go.dev/doc/go1.21