Open GoogleCodeExporter opened 8 years ago
Interesting. I think it's a combination of problems regarding generics, bridge
method (that are generated because of bridge method), and interface.
Thanks for reporting. I'll be looking into it, but still I won't be fast (busy
with so many other things), so don't hesitate to deal with it before me if
diving into mockito code don't bother you.
Cheers,
Brice
Original comment by brice.du...@gmail.com
on 26 May 2013 at 2:11
Hello
First of all I would like to start contributing to this project because it's
awesome.
Backing to issue: You are right, problem is related to the bridge methods. I
spent some time investigating this issue ane there are my notes:
I write additional main mehtod for B.class which do exactly the same thing like
test:
public static void main (String[] args){
B b = new B();
b.getId();
((I)b).getId();
}
Next i decompile compiled sources and get result:
public class B extends A<java.lang.String> implements I {
public B();
Code:
0: aload_0
1: invokespecial #1 // Method A."<init>":()V
4: return
public static void main(java.lang.String[]);
Code:
0: new #2 // class B
3: dup
4: invokespecial #3 // Method "<init>":()V
7: astore_1
8: aload_1
9: invokevirtual #4 // Method getId:()Ljava/lang/Object;
12: pop
13: aload_1
14: invokeinterface #5, 1 // InterfaceMethod I.getId:()Ljava/lang/String;
19: pop
20: return
public java.lang.String getId();
Code:
0: aload_0
1: invokespecial #6 // Method A.getId:()Ljava/lang/Object;
4: checkcast #7 // class java/lang/String
7: areturn
}
As you can see when you invoke ((I)b).getId(); in fact you invoke bridge method
from B.class
What is also important in this part during proxy creation class some filter is
setted to ignore bridge methods proxying:
At class ClassImposterizer lines 45-49
private static final CallbackFilter IGNORE_BRIDGE_METHODS = new
CallbackFilter() {
public int accept(Method method) {
return method.isBridge() ? 1 : 0;
}
};
As a result invocation of ((I)b).getId(); cause that real implementation is
used instead of mock one.
Original comment by albers...@gmail.com
on 6 Jan 2014 at 12:47
[deleted comment]
Probably I found potential solution but I'm not sure whether it will not cause
any problems:
I suggest to remove NoOp interceptor from callback types at enhancer. As a
result Callback filter will not be necessary and thanks to it all invocations
would be intercepted by MethodInterceptor class.
What do you think about this solution?
I attached patch with proposed changes(probably deletion of Noop will be
necessary)
Original comment by albers...@gmail.com
on 6 Jan 2014 at 1:28
Attachments:
Did you run all tests with this patch? All good?
Original comment by szcze...@gmail.com
on 6 Jan 2014 at 11:15
Unfortunately after aplying patch some tests are failing(related to compareTo).
I'm still investigating problem in order to provide better solution. It is
really complicated.
Original comment by albers...@gmail.com
on 6 Jan 2014 at 4:07
I have one question:
Why bridge methods are treated in special way? I mean: they are not intercepted
by cglib proxy. Everything is ok as produced byte code use INVOKE_VIRTUAL
operation code. As a result proper methods is invoked which is proxied by cglib
class instance. Situation in which delegation is invoked by INVOKE_SPECIAL
operation code does not work as expected.
Bridge methods only delegates invocation to the method written in src code so
why do not intercept them also in cglib proxy? This kind of change require also
small modification of
org.mockito.internal.util.ObjectMethodsGuru.isCompareToMethod(...) (look for
also for the bridge instance of compareTo)
What is also important this modification cause that test
BridgeMethodPuzzleTest.shouldVerifyCorrectlyWhenBridgeMethodCalled will not
have sense.
Maybe I'm wrong but proposed solution will work correctly. Please let me know
what do you think about it. I'm aware that this solution is not perfect.
Some others solution comes to my mind but they will consume much more time.
Original comment by albers...@gmail.com
on 6 Jan 2014 at 10:32
Interesting. Please explain what you have in mind for the other solutions.
Also could you create a pull request on github with your changes, with a
referring link to this issue ? ;)
Thanks for your investigation
Original comment by brice.du...@gmail.com
on 8 Jan 2014 at 12:34
Actually we set two callback for Enhancer:
- ClassImposterizer - which use cglib proxy for checking whether given method
was mocked. When it is mocked returns defined answer or "null"
- Noop - for bridge methods which invoke proper implementation
Maybe it will be worth to write another callback insted of Noop which will work
in that way:
- if bridge method was mocked it will return defined answer otherwise delegate
invokation to the proper implementation
What do you think: which solution is better?
When it comes to pull request - of course I will provide it tomorrow or on
friday
Original comment by albers...@gmail.com
on 8 Jan 2014 at 1:01
I think the first solution is probably better, ie to mock the real method,
because the java platform will invoke the bridge method or the real method
depending on the callsite.
Otherwise this would mean that if mockito only mock the bridge method, some
other compiled code might use the real method instead of the bridge method and
hence see the default answer instead of the stubbed one.
Original comment by brice.du...@gmail.com
on 8 Jan 2014 at 1:28
Actually I' m working on providing first solution - I mean preparing Pull
Request. I find something that seems to be really desirable:
after introducing first solution test: report_why_this_exception_happen() in
class ParentClassNotPublicVeryWeirdBugTest starts failing.
I think ttat this test is no longer needed as proposed solution solve another
issue.
Am i wright?
Original comment by albers...@gmail.com
on 9 Jan 2014 at 8:41
Pull request: https://github.com/mockito/mockito/pull/33
Please take have a look, execute code review and return comments
Original comment by albers...@gmail.com
on 9 Jan 2014 at 9:14
[deleted comment]
This change probably also solved issue 212
Original comment by albers...@gmail.com
on 9 Jan 2014 at 9:20
Original issue reported on code.google.com by
dlaze...@gmail.com
on 24 May 2013 at 11:49