Mockito-mocked methods returning primitive types other than "int" throw ClassCastException: java.lang.Integer

joepadde / mockito

Automatically exported from code.google.com/p/mockito

0 stars 0 forks source link

Mockito-mocked methods returning primitive types other than "int" throw ClassCastException: java.lang.Integer #352

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago

Mockito is returning a boxed Integer when it should be returning a boxed Long. 
Similarly for other types. I'm preparing a fix and will have a pull request 
very soon. It would be great to get this fixed before 1.9.5 final is cut.

See dexmaker issue 10
http://code.google.com/p/dexmaker/issues/detail?id=10

Original issue reported on code.google.com by limpbizkit on 29 Jun 2012 at 8:14

GoogleCodeExporter commented 8 years ago

Fix attached. I couldn't get hg to work the way I wanted it to so I'm doing 
this the lame manual way.

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

Attachments:

fix352.patch

GoogleCodeExporter commented 8 years ago

Cool, I will integrate the patch.
Thx

Original comment by brice.du...@gmail.com on 29 Jun 2012 at 9:30

Changed state: Accepted

GoogleCodeExporter commented 8 years ago

Thanks!

Does Mockito assume that MockHandler instances have to return the correct boxed 
type or not? It looks like cglib's proxy class bytecode adjusts the type if 
necessary. Thus, this issue is not visible on cglib. See 
http://code.google.com/p/dexmaker/issues/detail?id=10 for context (especially 
comments #11 and #16.

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

GoogleCodeExporter commented 8 years ago

Yes, the previous code assumed MockHandler should return the correct boxed type.
Though since the MockMacker is now plugable, I don't think this be assumed 
anymore, I don't know how Szczepan feels about that.

I'm not sure at this hour, it 1am here in paris, but there might be some code 
of our own involved like MethodInfo, Primitives as well.

I'll take a look tomorrow anyway.

Original comment by brice.du...@gmail.com on 29 Jun 2012 at 11:36

GoogleCodeExporter commented 8 years ago

Thanks! No rush.

To me it seems that not requiring MockMaker to return the correct boxed types 
is not ideal. Firstly, all clients of MockHandler need to handle the various 
conversion cases -- lots of duplication. Secondly, some of the conversions lose 
precision (e.g.,  Long to Integer, Integer to Byte, Double to Float) and can 
cause hard to find bugs -- it's probably best to prohibit these types of 
conversions.

However, if the existing clients of MockMaker rely on the above conversions, 
then fixing the situation might be harder, especially if higher layers of 
Mockito or lots of Mockito users implicitly rely on this feature.

In the end, please take my opinions with a pinch of salt -- I don't know much 
about the inner workings of Mockito.

Original comment by klyu...@google.com on 30 Jun 2012 at 12:53

GoogleCodeExporter commented 8 years ago

I suspect it is only dexmaker that'll be impacted.

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

GoogleCodeExporter commented 8 years ago

Hi,

I took a deeper look at the situation here, it seems that the code that Jesse 
patched was indeed incorrect, ReturnsEmptyValues returned an int (then 
autoboxed as an Integer). It slipped through because CGLIB's magic I suspect. I 
don't know where yet, probably in the code generation area.

Anyway it worked fine because we only had CGLIB bytecode engine. Now as said I 
think Mockito's code should really return correct boxed types. I think the 
issue you mention on conversions is related to answer implementation, and 
ReturnsEmptyValues was incorrect on that matter.

For custom answers, it might be worth checking at runtime the type returned by 
these answers.

Anyway I integrated the patch, though I made some changes to use Primitives 
instead. It'll return the correct boxed types, let me know if you have issues 
with this code.

Cheers,
Brice

Original comment by brice.du...@gmail.com on 30 Jun 2012 at 2:34

Changed state: Started

GoogleCodeExporter commented 8 years ago

Original comment by brice.du...@gmail.com on 30 Jun 2012 at 2:55

Changed state: Fixed

GoogleCodeExporter commented 8 years ago

Thanks for the fast turnaround!

Original comment by limpbizkit on 30 Jun 2012 at 6:15

GoogleCodeExporter commented 8 years ago

You're welcome :)

Anyway this is interesting stuff, as I forked myself CGLIB but I'm kind of a 
newbie on this really low level stuff, I need to work more on it.

Cheers,
Brice

Original comment by brice.du...@gmail.com on 30 Jun 2012 at 6:21

GoogleCodeExporter commented 8 years ago

Thank you very much!

Original comment by klyu...@google.com on 2 Jul 2012 at 5:38

GoogleCodeExporter commented 8 years ago

Issue 374 has been merged into this issue.

Original comment by brice.du...@gmail.com on 6 Sep 2012 at 3:38

GoogleCodeExporter commented 8 years ago

Original comment by szcze...@gmail.com on 6 Oct 2012 at 7:38

Added labels: Milestone-Release1.9.5

GoogleCodeExporter commented 8 years ago

Original comment by brice.du...@gmail.com on 19 Dec 2012 at 10:59

Added labels: Dexmaker-Android