qdrzwd / dexmaker

Automatically exported from code.google.com/p/dexmaker
0 stars 0 forks source link

Mockito-mocked methods of classes returning primitive types other than int, char, or boolean throw ClassCastException: java.lang.Integer #10

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago
1. Create a mock of a class (rather than an interface) using Mockito 1.9.5-rc1 
+ DexMaker.
2. On the mock, invoke a method that returns a primitive type other than "int".

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

EXPECTED: The method invocation succeeds and returns either a default value or 
the stubbed value (e.g., set using doReturn).

ACTUAL: The method invocation throws ClassCastException: java.lang.Integer 
from the byte code generated by DexMaker.

Please use labels and text to provide additional information.

What appears to happen is that org.mockito.invocation.MockHandler provided to 
DexMaker's DexmakerMockMaker returns java.lang.Integer for methods that return 
primitive types. The byte code generated by Mockito then (correctly) attempts 
to convert this result to another class (e.g., java.lang.Long) and this fails.

SAMPLE CODE

public class A {
  public long getValue() {
    return 1;
  }
}

A a = Mockito.mock(A.class);
System.out.println(a.getValue());

Original issue reported on code.google.com by klyu...@google.com on 28 Jun 2012 at 7:50

GoogleCodeExporter commented 8 years ago
Doing the same but using plain vanilla Mockito 1.9.5-rc1 outside of Android 
(i.e., no DexMaker involved), works just fine.

Original comment by klyu...@google.com on 28 Jun 2012 at 7:52

GoogleCodeExporter commented 8 years ago
It looks like Mockito assumes that MockHandler can, for primitive values, 
return a boxed type that doesn't completely match the primitive type. For 
example, in the above example the MockHandler returns the java.lang.Integer 
generated by the code below:

https://code.google.com/p/mockito/source/browse/src/org/mockito/internal/stubbin
g/defaultanswers/ReturnsEmptyValues.java?r=f08f5d7b948bc3eb6310f86fd17105e0d80ee
b22#123

Original comment by klyu...@google.com on 28 Jun 2012 at 8:52

GoogleCodeExporter commented 8 years ago
Thanks for the perfect bug report. I've filed it upstream and will prepare a 
pull request.
https://code.google.com/p/mockito/issues/detail?id=352

Original comment by limpbizkit on 29 Jun 2012 at 8:15

GoogleCodeExporter commented 8 years ago
I'm not sure it's a bug in Mockito. The above example works fine with Mockito 
outside of Android land. I wonder if Mockito's design/expecation is that 
MockHandler can return boxed primitive types that the proxy class needs to 
convert to the correct type (if necessary).

Original comment by klyu...@google.com on 29 Jun 2012 at 3:46

GoogleCodeExporter commented 8 years ago
It's a bug in Mockito. Dexmaker's behavior is consistent with 
java.lang.reflect.Proxy. It also throws if you return the wrong boxed primitive.

This code:

  interface A {
    long returnLong();
  }

  public void testDynamicProxy() {
    InvocationHandler invocationHandler = new InvocationHandler() {
      @Override public Object invoke(Object proxy, Method method, Object[] args) {
        return Integer.valueOf(5);
      }
    };
    A a = (A) Proxy.newProxyInstance(getClass().getClassLoader(),
        new Class[]{A.class}, invocationHandler);
    System.out.println(a.returnLong());
  }

yields this exception:

    java.lang.ClassCastException: java.lang.Integer cannot be cast to java.lang.Long
        at $Proxy6.returnLong(Unknown Source)
        at Test.testDynamicProxy(Test.java:53)

Original comment by limpbizkit on 29 Jun 2012 at 4:10

GoogleCodeExporter commented 8 years ago
However, if you use Mockito + DexMaker to create a mock of the above interface 
A, then it works just fine despite MockHandler returning a java.lang.Integer 
(example below). 

interface A {
  long returnLong();
}

A a = Mockito.mock(A.class)
a.returnLong();

Original comment by klyu...@google.com on 29 Jun 2012 at 4:38

GoogleCodeExporter commented 8 years ago
@klyubin: did you misspeak? I assume you meant "Mockito + cglib" ? Otherwise we 
wouldn't be having this discussion!

Cglib does a lot of unnecessary type-munging work that probably has a 
performance cost. I don't think that motivates us to do the same work in 
dexmaker.

Original comment by limpbizkit on 29 Jun 2012 at 5:34

GoogleCodeExporter commented 8 years ago
No, I didn't misspeak. If you run the above example (from comment #6) on 
Android using Mockito 1.9.5-rc1 + DexMaker, then it will work just fine. Behind 
covers, the MockHandler provided to DexMaker by Mockito still returns a 
java.lang.Integer as the result of the invocation, which in turn makes 
DexMaker's InvocationHandlerAdapter return the same java.lang.Integer from its 
"invoke" method. I'm puzzled myself as to why this does not blow up inside the 
proxy object generated by java.lang.reflect.Proxy inside DexmakerMockMaker.

Original comment by klyu...@google.com on 29 Jun 2012 at 5:43

GoogleCodeExporter commented 8 years ago
A bit of experimenting seems to suggest that the proxy generated by 
java.lang.reflect.Proxy behaves differently between Dalvik and Java VMs. On 
Dalvik it seems to be fine with accepting an Integer where Long needs to be 
returned, whereas on JVM it isn't. This explains why Mockito + DexMaker works 
fine on Android for mocking interfaces, but not for classes: DexMaker's proxy 
for classes is not as lenient as the Dalvik VMs java.lang.reflect.Proxy. In 
particular, Dalvik VM's proxy class seems to permit conversions that don't lose 
precision (e.g., Integer -> Long, Float -> Double, Short -> Integer), but still 
doesn't permit the reverse conversions.

Original comment by klyu...@google.com on 29 Jun 2012 at 6:04

GoogleCodeExporter commented 8 years ago
Weird. That's a bug in Dalvik. Regardless, I think we want to be consistent 
with the JVM's Proxy class. Conversions of any kind make me anxious.

Original comment by limpbizkit on 29 Jun 2012 at 6:10

GoogleCodeExporter commented 8 years ago
To sum up, when using Mockito to mock a method returning a primitive long, the 
MockHandler passed by Mockito to MockMaker instances returns a 
java.lang.Integer by default. This:
1. Works fine on JVM both for classes and interfaces. This suggests that 
Mockito uses an extra layer somewhere between the MockHandler and proxy classes.
2. Works fine on Dalvik VM for interfaces, because proxies generated by 
DexMaker using java.lang.reflect.Proxy seem to upscale boxed primitive values 
if necessary (e.g., Integer -> Long) -- this appears to be a "feature/bug" of 
Dalvik's java.lang.reflect.Proxy.
3. Does not work on Dalvik VM for classes, because the proxy generated by 
DexMaker's ProxyBuilder does not upscale boxed primitive values.

Now to mitigation:
* It's best to make DexMaker's ProxyBuilder generate proxies that work similar 
to those generated by java.lang.reflect.Proxy on Dalvik VM. This means adding 
upscaling, unless that's a bug in Dalvik VM.
* We need to find out why Mockito's MockHandler returning a java.lang.Integer 
for a method that is supposed to return a java.lang.Long does not blow up on 
the JVM. We can then see if that's a bug or not. This, in turn, will tell us 
how/whether to fix DexMaker.

Original comment by klyu...@google.com on 29 Jun 2012 at 6:26

GoogleCodeExporter commented 8 years ago
One workaround until this is resolved properly, would be to add upscaling to 
DexMaker's InvocationHandlerAdapter. What do you think?

Original comment by klyu...@google.com on 29 Jun 2012 at 6:38

GoogleCodeExporter commented 8 years ago

Original comment by klyu...@google.com on 29 Jun 2012 at 6:52

GoogleCodeExporter commented 8 years ago
P.S. To complicate matters, Mockito + DexMaker doesn't work for mocking 
interfaces whose methods return primitive byte or short. This is because the 
java.lang.reflect.Proxy on Dalvik does not downscale java.lang.Integer to 
java.lang.Byte or java.lang.Short.

So, in addition to the differences between the java.lang.reflect.Proxy behavior 
on Delvik and JVM, there's a fundamental mismatch between what Mockito's 
MockHandlers are returning and what DexMaker is expecting them to return. I'm 
still wondering whether it's a bug in Mockito or whether it expects (but does 
not declare) that MockHandler's results may need to be converted (up- or 
down-scaled) for primitive types.

Original comment by klyu...@google.com on 29 Jun 2012 at 8:19

GoogleCodeExporter commented 8 years ago
Here's a patch against ebb25ef16a03f478946593d2d2ada043e0007220. The patch 
fixes all of these issues by making DexMaker's InvocationHandlerAdapter adjust 
the boxed primitive values it returns. What do you think?

Original comment by klyu...@google.com on 29 Jun 2012 at 9:15

Attachments:

GoogleCodeExporter commented 8 years ago
I suspect (but haven't confirmed) that, on the JVM, Mockito does not experience 
the issues because it uses the following code (from cglib) in its proxy (used 
for interfaces and classes): 
https://code.google.com/p/mockito/source/browse/cglib-and-asm/src/org/mockito/cg
lib/core/CodeEmitter.java?r=f08f5d7b948bc3eb6310f86fd17105e0d80eeb22#733.

The code casts the boxed value to java.lang.Number and then invokes the 
to-primitive-value method (e.g., intValue, byteValue). The behavior is very 
similar to that in the patch attached to comment #15.

Original comment by klyu...@google.com on 29 Jun 2012 at 9:30

GoogleCodeExporter commented 8 years ago
We should just get this fixed in Mockito. We've already got a patch and it 
shouldn't be much trouble for them to integrate it. The casting to Number stuff 
is wacky, especially because the conversion may be lossy.

Original comment by limpbizkit on 30 Jun 2012 at 2:07

GoogleCodeExporter commented 8 years ago
Fixed in Mockito.

Original comment by limpbizkit on 1 Jul 2012 at 6:09

GoogleCodeExporter commented 8 years ago
Thanks! Good call.

Original comment by klyu...@google.com on 2 Jul 2012 at 8:25