panicing code results in tests passing?

[Dieterbe]

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

[Dieterbe]

note: i have just removed the bogun panic in grafana/metrictank#917 , so to reproduce you need a version older than that, such as 0.9.0 (git checkout 0.9.0)

[benpaxton-hf]

duplicate of #98 - you're effectively calling panic(nil), which is not distinguishable from not calling panic() at all.

[mdwhatcott]

Go 2 Proposal for dealing with panic(nil): golang/go#25448

[Dieterbe]

wow, ok. thanks ! let's close this then.

[Dieterbe]

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.

[mdwhatcott]

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