panic in test is swallowed if there are failed expectations and t.Cleanup(ctrl.Finish)

[powerman]

Actual behavior A clear and concise description of what the bug is.

If test function setup some EXPECT() which wasn't called yet and then it panic(), the panic message won't be shown in test output, all I get is missing call(s) to … aborting test due to missing call(s) message.

Expected behavior A clear and concise description of what you expected to happen.

Panic has to be shown in test output. Mentioned above message about missing calls may or may not be shown, it's much less important than panic and often implicit because of panic.

To Reproduce Steps to reproduce the behavior

I can provide trivial example if needed, but I believe it's obvious.

Additional Information

• gomock mode (reflect or source): source
• gomock version or git ref: v1.4.3
• golang version: go version go1.14.2 linux/amd64

Triage Notes for the Maintainers

[codyoss]

Thanks for the feedback, I agree I think we could do more for the cases where the mocks are not quite setup right. I think there are other related issues here, will link when I re-come across them.

[lassekv]

I just ran into this issue today as well. This only happens from Go 1.14 and forward when an explicit defer ctrl.Finish() is no longer expected because it is registered through the testing.T interface. If one explicitly does a defer ctrl.Finish() then the panics are not swallowed.

[powerman]

This is still quite annoying - test just hangs without any output. @codyoss If you can provide some quick hints about how to implement possible fix then I can try to prepare PR.

In the meanwhile I'd like to share two workarounds:

1. Run go test -timeout=5s to work around hang and get some details about which test is broken/panics.
2. Handle panic manually inside tests to actually workaround this issue (but it's inconvenient because this code must be added inside every Test-function and also inside every t.Run-function):

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

[codyoss]

@powerman I did open up a PR for this yesterday. You can see the link above your comment 😸 I think that should solve the issue. Check out the PR and let me know if it does not though.

[powerman]

@codyoss Your PR didn't fix the issue. Here is minimal example to reproduce hang:

//go:generate mockgen -package=$GOPACKAGE -source=$GOFILE -destination=mock.$GOFILE T

package gomock428

import (
	"testing"

	"github.com/golang/mock/gomock"
)

type T interface {
	F(string) error
}

func Test428(t *testing.T) {
	ctrl := gomock.NewController(t)
	t.Cleanup(ctrl.Finish)

	o := NewMockT(ctrl)

	o.EXPECT().F("ok").Return(nil) // <--- replace with o.EXCEPT() to fix the hang

	t.Run("", func(t *testing.T) {
		panic("boom")
	})
}

[codyoss]

A couple things:

1. Are you trying to make use of add default calling of ctrl.Finish() in go1.14+ #422 on HEAD? If so you actually don't need to call t.Cleanup(ctrl.Finish) as this will be done for you. If you remove that line you will see the panic.

2. When using you are using subtests with mocks you should be creating the mock from within the subtest. If you don't the t context is not correct for any calls to the mock.

3. If you want to explicitly call ctrl.Finish it should be deferred, as a deferred call is the only wait to get context of a panic.

[powerman]

Sorry, I'm too sleepy now to answer 1 and 3, but this one surprises me:

2. When using you are using subtests with mocks you should be creating the mock from within the subtest. If you don't the t context is not correct for any calls to the mock.

I never bother about this and used t.Run a lot without any issues. Moreover, having to define mocks inside t.Run will be incredible inconvenient in a very usual case when we define EXPECT before loop over test cases (e.g. https://github.com/powerman/go-service-goswagger-clean-example/blob/46dc447e66267226c4432a448722ead4b7276adc/internal/srv/openapi/handlers_test.go#L23-L36) - because to call EXPECT within t.Run we'll need to include details about how to call EXPECT inside data structure which describes test cases.

[codyoss]

I never bother about this and used t.Run a lot without any issues.

It is an issue if there are errors. Here is an example:

gm428.go

package gm428

//go:generate mockgen -package=gm428 -source=gm428.go -destination=gm428_mock.go

type Foo interface {
	Bar() string
}

func Baz(f Foo) string {
	return f.Bar()
}

gm428_test.go

package gm428

import (
	"testing"

	gomock "github.com/golang/mock/gomock"
)

func TestBaz(t *testing.T) {
	ctrl := gomock.NewController(t)
	defer ctrl.Finish()
	mock := NewMockFoo(ctrl)

	t.Run("one", func(t *testing.T) {
		//mock.EXPECT().Bar().Return("Bar")
		Baz(mock)
	})

	// This will never run
	t.Run("two", func(t *testing.T) {
	})
}

Run tests go test -v:

=== RUN   TestBaz
=== RUN   TestBaz/one
=== CONT  TestBaz
    gm428.go:10: Unexpected call to *gm428.MockFoo.Bar([]) at /tmp/gm428_mock.go:39 because: there are no expected calls of the method "Bar" for that receiver
=== CONT  TestBaz/one
    testing.go:1033: test executed panic(nil) or runtime.Goexit: subtest may have called FailNow on a parent test
--- FAIL: TestBaz (0.00s)
    --- FAIL: TestBaz/one (0.00s)
FAIL
exit status 1
FAIL    example.com/gm428       0.189s

[powerman]

@codyoss

It is an issue if there are errors. Here is an example:

I don't see how t.Run affect your example:

• if we remove t.Run then it still works the same way (calls t.FailNow or panic on first call to Baz(mock) and never reach second call)
• if we move EXCEPT() from within t.Run to before t.Run then it still works the same way (fail on second call)

If I get you right and you wanna continue test to second t.Run even if mock fails in first t.Run your example should be changed to include all mock-related setup inside each t.Run - this works, but it looks overkill and not needed in most cases to me:

func TestBaz(t *testing.T) {
	t.Run("one", func(t *testing.T) {
		ctrl := gomock.NewController(t)
		defer ctrl.Finish()
		mock := NewMockFoo(ctrl)
		// mock.EXPECT().Bar().Return("Bar")
		Baz(mock)
	})
	// This will run too
	t.Run("two", func(t *testing.T) {
		ctrl := gomock.NewController(t)
		defer ctrl.Finish()
		mock := NewMockFoo(ctrl)
		Baz(mock)
	})
}

[powerman]

1. Are you trying to make use of add default calling of ctrl.Finish() in go1.14+ #422 on HEAD? If so you actually don't need to call t.Cleanup(ctrl.Finish) as this will be done for you. If you remove that line you will see the panic.

This is a cool feature.

3. If you want to explicitly call ctrl.Finish it should be deferred, as a deferred call is the only wait to get context of a panic.

Hmm… I see what you mean, but:

• I don't see how your PR changes this behaviour - 1.4.4 works in same way.
• I suppose there a lot of existing code which already does t.Cleanup(ctrl.Finish), and while you right about this may result in omitting ctrl.Finish() call in case of panic (so we'll lost messages about missing calls to mock in case of panic - which is perfectly fine), this should not turn panic into hang!

I mean, this is what this issue about (but we should update the issue title to mention it happens only with t.Cleanup(ctrl.Finish) - will do now) and it's not fixed by your PR.

[cvgw]

@codyoss

It is an issue if there are errors. Here is an example:

I don't see how t.Run affect your example:

• if we remove t.Run then it still works the same way (calls t.FailNow or panic on first call to Baz(mock) and never reach second call)
• if we move EXCEPT() from within t.Run to before t.Run then it still works the same way (fail on second call)

If I get you right and you wanna continue test to second t.Run even if mock fails in first t.Run your example should be changed to include all mock-related setup inside each t.Run - this works, but it looks overkill and not needed in most cases to me:

func TestBaz(t *testing.T) {
	t.Run("one", func(t *testing.T) {
		ctrl := gomock.NewController(t)
		defer ctrl.Finish()
		mock := NewMockFoo(ctrl)
		// mock.EXPECT().Bar().Return("Bar")
		Baz(mock)
	})
	// This will run too
	t.Run("two", func(t *testing.T) {
		ctrl := gomock.NewController(t)
		defer ctrl.Finish()
		mock := NewMockFoo(ctrl)
		Baz(mock)
	})
}

There are many cases where gomock will "work", but because the Controller maintains a reference to *testing.T it needs to be the instance of *testing.T in the current test scope (IE the current subtest). The golang testing pkg requires that all operations on the *testing.T instance be called on the instance belonging to the current test scope.

The new t.Cleanup stuff is a bit unfamiliar still so apologies for the issues here; I do think that calling t.Cleanup(ctrl.Finish) is something which should be avoided. The ctrl.Finish() method wasn't designed to be used in that way; I would only use it with defer ctrl.Finish() in the current test scope (IE current subtest).

If for some reason we decided to support t.Cleanup(ctrl.Finish) you would still need to call it in every test scope, at which point I'm not sure what benefit there would be to using t.Cleanup rather than defer.

[codyoss]

As of right now we don't plan to support calling t.Cleanup for calling ctrl.Finish for the reasons outlined above. I think we have it pretty well documented that calling ctrl.Finish() should be deferred.

[powerman]

I agree that in general case we need to create controller inside t.Run. But!

t.Run is often used not to run test at different point in time (e.g. because of t.Parallel inside t.Run), but just to name subtests executed in a loop on table test cases, within normal test flow - as recommended by https://blog.golang.org/subtests. In this case t.Run never contains full setup both because this is done before loop on test cases and because it's impossible to write test this way without injecting large anonymous "init" functions into table with test cases itself (which hurt readability of table tests a lot and thus should be avoid).

To me it looks like it's worth to support this use case.

As for t.Cleanup vs defer - sure, to catch and correctly handle panic we must use defer… or testing package's implementation of Cleanup must ensure it'll run in case of panic by using defer inside testing package. But this isn't the point - I can live with swallowed messages from gomock about missing calls if I prefer Cleanup to defer to have less boilerplate code in tests. The point is using Cleanup shouldn't result in hang or swallowed panic message.

[cvgw]

@powerman I agree that for a canonical, table-driven golang test there is some unpleasant overhead with gomock.

I typically do what you mentioned and use an anonymous function as my table case EG

type testCase struct {
    expect int
    mock Fooer
}

testCases := map[string]func(ctrl *gomock.Controller) testCase {
    "when foo is bar": func(ctrl *gomock.Controller) testCase {
      // setup and return testCase
    },
}

for desc, tcFunc := range testCases {
    t.Run(desc, func(t *testing.T) {
       ctrl := gomock.NewController(t)
       tc := tcFunc(ctrl)
    
       result := FunctionToTest(tc.mock)

       if result != tc.expect {
           // do some more test stuff
       }
    }
}

This could definitely be improved and we are happy for suggestions, as long as they are not breaking changes and don't require the use of the *testing.T instance from a different test scope.

[powerman]

More details and possible fix is in golang/go#41355 (comment)