ssacher-tgm / mockito

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

Generics problems with mocking parameterized types #204

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago
Supposing you have a class:

class Provider<T> {}

You can't write correctly without warnings:

Provider<String> unscoped = mock(Provider.class);

To deal with this, the mock method should have been written like this:

public static <T> T mock(Class<?> classToMock)

and not like this:

public static <T> T mock(Class<T> classToMock)

Original issue reported on code.google.com by mathieu....@gmail.com on 23 Jul 2010 at 2:12

GoogleCodeExporter commented 8 years ago
Interesting idea. Are you sure about this? Wouldn't it have any side effects?

Original comment by szcze...@gmail.com on 26 Jul 2010 at 9:00

GoogleCodeExporter commented 8 years ago
The side effect I see is that someone could write silly things like this:

Provider<String> unscoped = mock(Integer.class);

But he will get a class cast error. So i think it's quite acceptable ;-)

To my opinion, the signature with Class<T> is an error since you can't say 
Class<Provider<String>>. To fully support generics, the use of TypeLiteral like 
Guice would be necessary in this case:

public static <T> T mock(TypeLiteral<T> classToMock)

With such a signature, you would be able to write:

Provider<String> unscoped = mock(new TypeLiteral<Provider<String>>(){});

So if Classis directly used, Class<?> the way to go.

Original comment by mathieu....@gmail.com on 26 Jul 2010 at 2:07

GoogleCodeExporter commented 8 years ago
I have a feeling that refactorings might not work best if we go for Class<?>, 
e.g. extract field, extract local variable, etc. But maybe it is negligible?

Original comment by szcze...@gmail.com on 26 Jul 2010 at 3:19

GoogleCodeExporter commented 8 years ago
From a client point of view, I don't see where it could hurt since the change 
is transparent. The only thing that could happen and that could impact the test 
is when the class is renamed. If its parametrized type changes, the 
mock(Class<?>) won't be an issue, and it should be refactored correctly on the 
variable side (Provider<...> = )

But i think it's much more an IDE concern, and perhaps it shouldn't be 
considered. I think what should be considered is much more what the javac 
compiler outputs, and currently it's quite embarrassing to see a lot of 
warnings in tests because of this signature ;-)

Original comment by mathieu....@gmail.com on 26 Jul 2010 at 9:01

GoogleCodeExporter commented 8 years ago
Hey,

I looked at your suggestion once again to make sure it will play nicely. 
There're some gotchas:

1. The change will lead to compilation errors, for example this will no longer 
compile:
NeedsList tested = new NeedsList(mock(List.class));

2. The change will require extra casting (or generifying), eg:
NeedsList tested = new NeedsList((List) mock(List.class));

3. The code generation with TDD is not going to be as nice because IDE will not 
suggest correct type, eg:

mock(List.class); -> ctrl+f to extract field will not pick up the correct type.

Especially #1 is a kind of showstopper because new version should never make 
the existing code not compiling. Therefore I would probably suggest users to 
create their own mock() method in case they don't like so much suppressing 
warnings. Thoughts?

Original comment by szcze...@gmail.com on 13 Aug 2010 at 9:10

GoogleCodeExporter commented 8 years ago
Yes true. This is caused by type inference: to make it work, you would need in 
this case to do:

NeedsList tested = new NeedsList(Mockito.<List>mock(List.class));

But i agree this is not acceptable. I am wondering if the JDK 7 has something 
to avoid this...

The difficulty here comes from the use of the Class object, which does not 
support templating. 

I think that to get around this, you'll have no choice to implement the issue 
#137  (TypeLiterals): this is the only way to guaranty a type-safe compilation 
with generics.

Otherwise, if you still stick with the Class<T> parameter, it would force the 
users to accept having plenty of warnings on their test and also at compilation 
time, which is quite embarrassing also.

I really don't see other alternatives...

Hope it helped,

Mathieu.

Original comment by mathieu....@gmail.com on 13 Aug 2010 at 1:17

GoogleCodeExporter commented 8 years ago

Original comment by szcze...@gmail.com on 13 Aug 2010 at 2:32