jostyee / gomock

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

Modify callset.go callSet map indexing to correctly match receivers regardless of indirection or dereferencing the mock object before calling the interface method. #18

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
What steps will reproduce the problem?
1. Check out my clone with explanation, example interface and test file here: 
https://code.google.com/r/nickmab-callsetpatch
2. Running go test in the callsetpatch_sample folder (for package mypkg) fails 
without my changes and passes with them (it is checked in in a PASS state - 
simply change the import paths to make it break)

What is the expected output? What do you see instead?
A test that I expect to pass actually fails because the mock call matcher does 
not recognise the receiver. It doesn't recognise it specifically in cases where 
you dereference the pointer returned by NewMockIMyInterface() (generated by 
mockgen) before calling an interface method. It will make more sense when you 
see my code! 

What version of the product are you using? On what operating system?
Using the latest gomock on CentOS7, MacOSX 10.10.1, Windows 7. Fault appears on 
all 3 OSes, but I made and tested my changes only on Mac using go1.3 
darwin/amd64.

Please provide any additional information below.

I'm going to apologise in advance for any quality / convention / n00b mistakes 
or "you're not using interfaces the way you're meant to use interfaces" errors 
I've made here. I'm new to golang and to making open source contributions in 
general. I'm just looking to get a feature incorporated that would definitely 
help me in real life testing (I struck this at work just the other day).

Original issue reported on code.google.com by nick...@gmail.com on 23 May 2015 at 11:06

GoogleCodeExporter commented 9 years ago
Also maybe I'll elaborate a bit on the real life case where this struck me: 

I have a function that I wish to test that takes a ptr to a struct as its sole 
argument. 

It's basically an "InitializeThisThingForMe" function that uses reflection for 
convenience (saves me having to write a whole bunch of boilerplate NewThis() 
and NewThat() functions for any number of future struct types).

The function needs to call an interface method on any and all of the struct 
fields of types that implement a specific interface, regardless of whether the 
field is an interface type, a pointer to a struct that implements this 
interface or a struct value in-place. 

The last of these is currently untestable using a gomock generated mock 
interface.

Original comment by nick...@gmail.com on 23 May 2015 at 11:18

GoogleCodeExporter commented 9 years ago
Sorry, I can't really see what your probably is or proposed change is. It's 
hard to discover that by looking at a clone. Can you make a diff and attach it 
here?

Original comment by dsymo...@golang.org on 25 May 2015 at 3:31

GoogleCodeExporter commented 9 years ago
Hi, yep, no worries. 

You can see all changes I made (I made only one commit after cloning) here:
https://code.google.com/r/nickmab-callsetpatch/source/detail?r=afb265f8533d70b78
6252a6c6da6b0e63f53ed94 

3 of the changes are adding example files to demonstrate the point of the 
change.

1 change is the actual change to callset.go, which I've attached here too.

Original comment by nick...@gmail.com on 25 May 2015 at 6:18

Attachments:

GoogleCodeExporter commented 9 years ago
If I understand the diff correctly, it's making it so that you can copy mock 
objects, like what you do in callsetpatch_sample/mypkg_test.go?

If so, that's not a supported use case. In your sample, you should be calling 
NewMockIExample for each mock object, not copying an existing value. Your 
change would also break things if we ever needed to put something in a mock 
object that wasn't a legal map value, since your change uses a struct as a map 
key instead of a pointer to a struct.

Original comment by dsymo...@golang.org on 26 May 2015 at 1:48

GoogleCodeExporter commented 9 years ago
No worries at all, thanks for the patient explanation.

Original comment by nick...@gmail.com on 26 May 2015 at 2:22