Fix Panics and PanicMatches checks to work with go1.21 nil panics

[liggitt]

go1.21 changes panic to propagate an instance of &runtime.PanicNilError{} if panic(nil) is called. See golang/go#25448 and https://go.dev/cl/461956 for details.

This library exercises behavior of panic(nil) in tests, which means this library no longer passes unit tests at go tip.

This PR adjusts the Panics and PanicMatches tests to work with both nil and &runtime.NilPanicError{} propagation for nil panics.

[liggitt]

cc @niemeyer

[liggitt]

Result before this PR:

go version devel go1.21-f9da938614 Tue Feb 7 17:17:35 2023 +0000 darwin/arm64

----------------------------------------------------------------------
FAIL: checkers_test.go:229: CheckersS.TestPanicMatches

checkers_test.go:251:
    // Verify a nil panic
    testCheck(c, check.PanicMatches, false, "Panic value is not a string or an error", func() { panic(nil) }, "")
checkers_test.go:33:
    c.Fatalf("%s.Check(%#v) returned (%#v, %#v) rather than (%#v, %#v)",
        info.Name, params, result_, error_, result, error)
... Error: PanicMatches.Check([]interface {}{"panic called with nil argument", ""}) returned (false, "") rather than (false, "Panic value is not a string or an error")


----------------------------------------------------------------------
FAIL: checkers_test.go:199: CheckersS.TestPanics

checkers_test.go:225:
    // Verify a nil panic
    testCheck(c, check.Panics, true, "", func() { panic(nil) }, nil)
checkers_test.go:33:
    c.Fatalf("%s.Check(%#v) returned (%#v, %#v) rather than (%#v, %#v)",
        info.Name, params, result_, error_, result, error)
... Error: Panics.Check([]interface {}{(*runtime.PanicNilError)(0x102ebaef8), interface {}(nil)}) returned (false, "") rather than (true, "")

OOPS: 136 passed, 2 FAILED
--- FAIL: Test (0.08s)
FAIL
FAIL    gopkg.in/check.v1       0.220s
FAIL

Result with this PR:

=== RUN   Test
OK: 138 passed
--- PASS: Test (0.09s)
PASS
ok  	gopkg.in/check.v1	0.404s

[jhenstridge]

Is this still necessary? Looking at the Go 1.21 release notes, they ended up making the behaviour dependent on the go version in the main package's go.mod file.

Using Go 1.22, go-check's tests pass for me. If I edit the go.mod to bump the minimum Go version to 1.21 and run go mod tidy, the tests fail in the way you demonstrate above. I could fix the test suite at this point by comparing against &runtime.PanicNilError{} instead of nil. Presumably any other package testing nil panics with go-check could do the same independently.

Or are there cases where one module's tests will be run against another module's go.mod?

[liggitt]

Yes, this is still needed. This is fixing the checker to work properly with nil panics, not just fixing unit tests.

[jhenstridge]

What I mean is that in a Go 1.21+ module, why wouldn't you just write the following?

c.Check(func() { panic(nil) }, Panics, &runtime.PanicNilError{})

That is correctly testing the behaviour of the closure under the semantics the module has opted into.

The only case I can think of where you could run into problems is where you have a test suite in one module call a helper from a second module that performs a c.Check call like the one above. But that seems like a fairly odd way to structure a test suite.

Are there other cases where there's ambiguity that I'm not thinking of?

[liggitt]

This change was trying to accomplish two things:

1. prepare this module for running with go1.21 defaults without breaking its behavior under go1.20, so that test code would pass as-is regardless of go version
2. avoid making test code in dependent modules be go-version-specific (or have to introspect use of GODEBUG=panicnil=1 that could flip behavior back despite the go version)

[jhenstridge]

The release notes say that the old behaviour "is enabled automatically when compiling a program whose main package is in a module that declares go 1.20 or earlier". In the case of testing the "main package" is going to be the package containing the tests, so it is in control of the behaviour it will see when it's tests are run.

If I write a program that uses go-check for testing (or a library that uses go-check for testing), the go-check tests are not run in the context of my go.mod file.

And looking at it from the other end: if I write a test that asserts that some code I've written causes a nil panic and then upgrade the minimum Go version in my go.mod file, isn't it a good thing that the test starts failing? Having the test lie could hide other problems if I depended on the old behaviour.

[dmitshur]

Or are there cases where one module's tests will be run against another module's go.mod?

To answer this question specifically: yes, it is possible to test packages contained in modules other than the main module, although it's likely less frequently done than testing only the packages contained in the main module.

For example, as documented at https://pkg.go.dev/cmd/go#hdr-Package_lists_and_patterns, the package pattern "all" expands to all packages in the main module and their dependencies, including dependencies needed by tests of any of those. So if a new module is created with a go directive at 1.21 or higher and has gopkg.in/check.v1 as a dependency, running go test -short all will show the two tests in gopkg.in/check.v1 that fail because they weren't intended to run with go1.21+ semantics:

$ go version
go version go1.22.4 darwin/arm64
$ cd $(mktemp -d)
$ go mod init example
go: creating new go.mod: module example
$ echo 'package example_test

import "testing"
import . "gopkg.in/check.v1"
 
func Test(t *testing.T) { TestingT(t) }; func init() { Suite(MySuite{}) }

type MySuite struct{}
func (MySuite) TestHello(c *C) { c.Check(123, Equals, 123) }' > example_test.go
$ go get -t
go: added github.com/kr/pretty v0.2.1
go: added github.com/kr/text v0.1.0
go: added gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c
$ go list -m all
example
github.com/kr/pretty v0.2.1
github.com/kr/pty v1.1.1
github.com/kr/text v0.1.0
gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c

$ go test -short all 
[…]
ok      example 0.185s
[…]
OOPS: 136 passed, 2 FAILED
--- FAIL: Test (0.26s)
FAIL
FAIL    gopkg.in/check.v1   0.456s
[…]
FAIL
$ echo $?
1

[liggitt]

@dmitshur, thanks for the example

And looking at it from the other end: if I write a test that asserts that some code I've written causes a nil panic and then upgrade the minimum Go version in my go.mod file, isn't it a good thing that the test starts failing? Having the test lie could hide other problems if I depended on the old behaviour.

That's a good point.

I reworked this PR to limit the change to checkers_test.go, rather than changing the behavior of the checkers themselves.

This ensures the checkers won't mask assertions callers are relying on, but makes it so that internal gocheck tests pass with go 1.21+ panicnil on and off.

go version
go version go1.22.4 darwin/arm64

GODEBUG=panicnil=0 go test . -count=1
ok  	gopkg.in/check.v1	0.259s

GODEBUG=panicnil=1 go test . -count=1
ok  	gopkg.in/check.v1	0.222s

[liggitt]

@jhenstridge, any feedback on #136 (comment)?