If the mock is shared across goroutines, any error/fatal messages will get swallowed

[rancerbeta]

I had some code like this

m := MockInterface{}
l := LogEntry{}
m.Expect().Fire(l)
AsyncWrapper(m).Fire(l)

AsyncWrapper.Fire adds to a channel that a goroutine is listening too
Then that goroutine calls mock.Fire(l)

If the signature is mismatching what was in Expect, a fatalf is normally called.
see: https://github.com/golang/mock/blob/master/gomock/controller.go#L150

The problem is the fatalf is called on a branch goroutine that is not the main test goroutine. This swallows the fatalf message and kills the branch goroutine. If the main goroutine has a wait on the async fire finishing, the test will just hang.

Let me know if you need more details.

[balshetzer]

Hi Michael,

When the controller is created you pass in a TestReporter. You are most likely just passing in the testing.T, which implements TestReporter. The problem is that the implementation of Fatalf in testing.T has issues like the one you describe.

This leaves us with two options:

1. gomock could try to handle this somehow. Maybe gomock could figure out that a call to Fatalf wasn't truly fatal or that we're running in a different goroutine than the main test. I don't actually know if these are possible though.

2. The user can pass in a different implementation of TestReporter. For example, the implementation of Fatalf could call os.Exit. Or one that cancels a context when Fatalf is called. An implementation of the second option exists here: https://github.com/golang/mock/blob/master/gomock/controller.go#L103

I'm concerned that option one would be out of scope and too much for gomock to take on. It seems like if these things are possible then the testing package itself would or should take care of that. It might also require trying to read the user's mind about their intention, which is not usually a good idea.

Unfortunately, most people don't think of implementing their own TestReporter because all the examples just pass in testing.T. Maybe there is a way to make this option clearer. gomock could use better documentation in general. That might be the best solution here.

Hesky

[prateek]

@balshetzer how would you feel about including something equivalent to (2) in the codebase/documentation? I use a panic instead of an os.Exit because it logs the stacktrace, example.

The default hanging behaviour in these situations is really annoying for users who don't know about this workaround. I'd go as far as suggesting that gomock.Controller use such a Reporter by default if it's provided a *testing.T during construction

[poy]

@rancerbeta I am having trouble recreating this, do you have an example somewhere?

[codyoss]

Bumping above comment @rancerbeta

[codyoss]

Closing, please reopen if you have an example to share.