search

barum / gomock

Automatically exported from code.google.com/p/gomock
Apache License 2.0
0 stars 0 forks source link

mockgen creates unusable mock in some circumstances. #9

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago 
What steps will reproduce the problem?
1. Acquire https://github.com/fluffle/goirc/blob/master/event/registry.go
2. Run mockgen -source registry.go -destination mock_registry.go -package event

What is the expected output? What do you see instead?

There are two problems with the generated mock, both related to variadics:

1. Line 35, an errant comma is produced before the single variadic argument.

2. Lines 101 and 109, the variadic is of type ...interface{} not ...string.

The former should be an easy fix. The latter causes problems with 
reflect.DeepEqual as the types differ between the argument saved with the 
recorded call and the argument saved with the expected call.

[]interface{}{[]interface{}{"arg1", "arg2"}} != []interface{}{[]string{"arg1", 
"arg2"}}

What version of the product are you using? On what operating system?

Latest hg tip.

Please provide any additional information below.

http://www.pl0rt.org/~alex/mock_registry.patch

In a related note, reflect.DeepEqual also caused some confusion for me when 
comparing a character pulled from a string with indexing (string[1]) and a 
character 'c'. The latter is an int naturally, but is often coerced to a byte 
when doing string[0] == 'c' in code. Unfortunately, if your mock function is 
e.g. Warn(fmt string, args ...interface{}), and you do something like:

switch c := string[0]; c {
  default:
    Warn("Unknown char %c", c)
}

Mocking this correctly requires object.EXPECT().Warn("Unknown char %c", 
byte('c'))

Original issue reported on code.google.com by a.bram...@gmail.com on 13 Nov 2011 at 1:28

GoogleCodeExporter commented 9 years ago 
I believe I already fixed the first issue 
(http://code.google.com/p/gomock/source/detail?r=9aff7dfd2ebbb8d9521fd9e5852c027
8e897397b). Apart from the method name, I have a test case with exactly the 
same signature as your Run method, and mockgen produces correct output for it.

I am confused by the second problem you are claiming. Your input interface is

type EventRegistry interface {
    AddHandler(name string, h Handler)
    DelHandler(h Handler)
    Dispatch(name string, ev ...interface{})
    ClearEvents(name string)
}

and the diff you reference has

-func (mr *_MockEventRegistryRecorder) AddHandler(arg0 interface{}, arg1 
...interface{}) *gomock.Call {
+func (mr *_MockEventRegistryRecorder) AddHandler(arg0 interface{}, arg1 
...string) *gomock.Call {

mockgen should not have produced that first line with a variadic argument, 
because the AddHandler method does not have a variadic argument. Are you sure 
you produced the output from that exact input?

Original comment by dsymo...@golang.org on 13 Nov 2011 at 11:17

GoogleCodeExporter commented 9 years ago 
Ahem. Apologies, there were two slight problems with my report.

Firstly, I was using the release tagged mockgen to create the problematic mock. 
I updated to tip for weekly, but I was sticking with release for the release 
branch, and hadn't re-run mockgen to update mock_registry.go for weekly. If the 
first problem above is fixed in tip, that's good :-)

Secondly, after I wrote this problem report, I spent the rest of the day 
re-organising my code, so that link now 404s. The default github branch for 
goirc is "release", which is rather old code now. The newer event code is now 
at:

https://github.com/fluffle/goevent/blob/master/event/registry.go

This should match up more with the generated mock that has problems.

Original comment by a.bram...@gmail.com on 14 Nov 2011 at 10:40

GoogleCodeExporter commented 9 years ago 
Yeah, I don't know what to do about this. The recorder methods are deliberately 
all interface{} so you can pass in the true type as well as matchers. That 
obviously gets in the way when it's variadic.

I don't know what we should expect for variadics there. I've successfully used 
GoMock with a variadic method, and in my test code I just use 
c.EXPECT.Varf("whatever %d", 7), but in that case the variadic is 
...interface{} and not ...string as in your case. If the generated code used 
...string then you wouldn't be able to use matchers.

Perhaps you need to write a custom matcher?

Original comment by dsymo...@golang.org on 15 Nov 2011 at 4:57

GoogleCodeExporter commented 9 years ago 
That's a plausible solution that'll work around both this problem and the other 
caused by the implicit boxing and reflect.DeepEqual. It's either that or 
leaving it manually edited as ...string and thus disallowing matchers, which is 
probably not wanted. Will see if I can make it generic enough to match 
variadics of any type.

Original comment by a.bram...@gmail.com on 23 Nov 2011 at 12:11

GoogleCodeExporter commented 9 years ago 
http://www.pl0rt.org/~alex/variadic.patch is a first hacky attempt.

Any chance of some instructions on how to set up for code review? I suspect 
you're using codereview.appspot.com. Couldn't seem to get that to play nice 
with 2 factor login.

Original comment by a.bram...@gmail.com on 23 Nov 2011 at 12:35

GoogleCodeExporter commented 9 years ago 
It follows the same setup as the Go repo.
  http://golang.org/doc/contribute.html#Code_review

Original comment by dsymo...@golang.org on 23 Nov 2011 at 4:04

GoogleCodeExporter commented 9 years ago 
Finally got some time to fiddle again. Needed to generate an app-specific 
password for the code review script, meh.

CL is up at http://codereview.appspot.com/5453052

I suspect the testing needs to be more thorough, but it uses the same kind of 
pattern as in the internals of reflect.deepValueEqual to test so it *should* 
cover at least variadics of basic types correctly.

Original comment by a.bram...@gmail.com on 5 Dec 2011 at 10:35