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

Catch panics in the cleanup #91

Closed
genesor opened this issue Mar 7, 2024 · 6 comments
Closed

Catch panics in the cleanup #91

genesor opened this issue Mar 7, 2024 · 6 comments

Comments

@genesor
Copy link
Contributor

genesor commented Mar 7, 2024

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
Copy link

nordicdyno commented Apr 8, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link

nordicdyno commented Jun 3, 2024

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: #91 (comment)

@genesor
Copy link
Contributor Author

genesor commented Jun 4, 2024

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
Copy link
Collaborator

zcolleen commented Jun 7, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants