Catch panics in the cleanup

[genesor]

Hi,

Currently if a panic happens before all expected calls to mocks are made the panic output is not shown in the test output, there is only the missing expected calls.

Do you think it's possible to output both the missing expected calls and the panic ?

[nordicdyno]

The issue was introduced in v3.3.0 after adding t.Cleanup(c.Finish) to controller's constructor. Why using t.Cleanup was not a good idea easy to illustrate with code here https://github.com/nordicdyno/minimock-panic

Only workaround I've found for previous versions it's not to call c.Finish in defer and don't use t.Cleanup either, the only option to avoid such "debugging surprises" I know is always explicitly call of Finish method at the end of the test.

[zcolleen]

Hi! This happens because when expected call is not called, then FailNow is called https://github.com/gojuno/minimock/blob/master/template.go#L353 which suppresses panic output. @hexdigest do we really need to call FailNow here?

[hexdigest]

Hey @genesor

It's not the responsibility of minimock to handle panics in your code. In fact it's not even possible because in order for the minimock to output information about the panic the panic needs to be recovered somewhere within minimock code but since it's not the minimock that's panicking it can't be caught there.

Here is the example of how panic in your code can be handled within your test:

func TestStringer(t *testing.T) {
    mc := minimock.NewController(t)

    mock := NewStringerMock(mc)
    mock.StringMock.Return("Hello, World!")

    defer func() {
        if err := recover(); err != nil {
            t.Error(err)
        }
    }()

    panic("panic is good for you")
}

Output:

go test ./...
--- FAIL: TestStringer (0.00s)
    main_test.go:17: panic is good for you
    safe_tester.go:35: Expected call to StringerMock.String
FAIL

[nordicdyno]

Sorry, but problem is not "minimock doesn't catch panics", but "minimock hides panics" (after 3.3.0).

Code that illustrates the problem:

gist:

type GetterCaller struct {
    Getter
}

func (gc GetterCaller) Run() {
    if err := gc.Getter.Get(); err != nil {
        panic(err)
    }
    gc.Getter.Get2()
}

func TestPanic(t *testing.T) {
    mc := minimock.NewController(t)

    mock := NewGetterMock(mc)
    mock.GetMock.Return(fmt.Errorf("fail"))
    mock.Get2Mock.Return()
    gc := GetterCaller{mock}
    gc.Run()

    t.Log("test finalized")
}

Output after 3.3.0:

    safe_tester.go:19: Expected call to GetterMock.Get2

Output before 3.3.0:

-- FAIL: TestPanic (0.00s)
panic: fail [recovered]
    panic: fail

goroutine 34 [running]:
testing.tRunner.func1.2({0x10495ff80, 0x140001382b0})
    /Users/aorlovskiy/sdk/go1.22.2/src/testing/testing.go:1631 +0x1c4
testing.tRunner.func1()
    /Users/aorlovskiy/sdk/go1.22.2/src/testing/testing.go:1634 +0x33c
panic({0x10495ff80?, 0x140001382b0?})
    /Users/aorlovskiy/sdk/go1.22.2/src/runtime/panic.go:770 +0x124
github.com/nordicdyno/minimock-panic/pre-3.3.0/minipanic.GetterCaller.Run({{0x104984608?, 0x140001525a0?}})
    /Users/aorlovskiy/w/my-gitlab/minimock-panic/pre-3.3.0/minipanic/minipanic.go:15 +0x60
github.com/nordicdyno/minimock-panic/pre-3.3.0/minipanic.TestPanic(0x14000118680)
    /Users/aorlovskiy/w/my-gitlab/minimock-panic/pre-3.3.0/minipanic/minipanic_test.go:17 +0x11c
testing.tRunner(0x14000118680, 0x1049830a0)
    /Users/aorlovskiy/sdk/go1.22.2/src/testing/testing.go:1689 +0xec
created by testing.(*T).Run in goroutine 1
    /Users/aorlovskiy/sdk/go1.22.2/src/testing/testing.go:1742 +0x318

reasons I always call mc.Finish at the end of my tests with minimock (and don't upgrade to 3.3.0+) are:

• (2) it's not very convenient to write catch panic in every test
• (1) catching panic in test with minimock doesn't work, cause on panic t.Cleanup in tests goes first and minimock calls on this stage t.Failnow() by Finish method, so it goes to runtime.Goexit() – end the game here, no any panic defers will be executed after

You can play with code from above here: https://github.com/gojuno/minimock/issues/91#issuecomment-2042759583

[genesor]

Hey @hexdigest

Thanks for the reply even if I don't agree with you.

As you know people are using minimock to save time writing tests and mocks. I'm pretty sure you would agree with me if I tell you that having to have a 5 line snippet in every unit test func won't be a scalable solution.

I totally agree with you with "It's not the responsibility of minimock to handle panics in your code" no debate in this but minimock should not HIDE panics in my code either.

Currently with the FailNow call in the MinimockFinish it overrides the default behaviour of testing.T that outputs a panic in the underlying layers of the tested code.

I'm not asking for minimock to hide or even catch a panic as I know it's not possible, I just want minimock to avoid hiding panics when they occur and let the testing.T handle it for us as it should.

I tested locally by changing m.t.FailNow() to m.t.Fail() in MinimockFinish of a generated mock and it works like a charm. We would also need to add Fail() func to minimock.Tester for this, I'd happily open a PR for this if you allow me to do 😉

EDIT: I might understand that my original issue was not correctly worded, sorry about that

[zcolleen]

Hey guys, fix is available in v3.3.12 , thanks to everyone