ssacher-tgm / mockito

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

anyMap causes unchecked cast warning in eclipse #208

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago
Using anyMap() in eclipse causes a warning. If the code is written like this:
verify(mockObject).methodWithMap(anyMap());

Then a warning will be issued of the following form:
"Type safety: The expression of type Map needs unchecked conversion to conform 
to Map<X,Y>"

If the code is written like this:
verify(mockObject).methodWithMap((Map<X,Y>)anyMap());

Then a warning will be issued of the following form:
"Type safety: Unchecked cast from Map to Map<X,Y>"

Please add a mechanism to allow anyMap() objects to be created with type 
safety. Possibilities may include adding generics to anyMap (making the format 
"anyMap<X,Y>()") or adding arguments to the method (making the format 
"anyMapOf(X.class, Y.class)").

Original issue reported on code.google.com by bhforumu...@gmail.com on 29 Jul 2010 at 12:35

GoogleCodeExporter commented 8 years ago
We could do this but we prefer to @SupressWarnings in your tests. Why don't you 
too? :)

Original comment by szcze...@gmail.com on 30 Jul 2010 at 7:23

GoogleCodeExporter commented 8 years ago
I consider it bad form to add @SuppressWarnings to an entire method; the 
purpose of warnings is to appear, and suppressing all of them when I am not the 
sole author of the code seems like an invitation to problems. I would be fine 
with adding it to a particular line, either across the line as a whole or as an 
attribute on a specific variable. I have not yet managed to do so.

I have tried adding the @SuppressWarnings before the 'verify' statement:
@SuppressWarnings("unchecked")
verify(mockObject).methodWithMap(anyMap())

I have also tried adding the @SuppressWarnings to the anyMap() argument:
verify(mockObject).methodWithMap(@SuppressWarnings("unchecked") anyMap())

Both of these result in compiler errors, rather than warnings, which is 
unfortunately a step in the wrong direction. I would greatly appreciate it if 
either the anyMap functionality could be changed or if someone could direct me 
to the proper usage of @SuppressWarnings in these cases.

Original comment by bhforumu...@gmail.com on 30 Jul 2010 at 12:33

GoogleCodeExporter commented 8 years ago
Hey

I put @SuppressWarnings on the entire test class usually and I avoid too much 
generic typing to keep the test more compact & readable. 

The purpose of warnings is to detect runtime errors earlier, at compilation 
time (e.g. CCE). Since you are running the test when you change it you detect 
the problem immediately anyway. Therefore the rationale for type-safety 
warnings in tests is pretty much null to me. Test readability wins.

More over, I believe that if you do TDD and reach coverage nearly ~100% it also 
deprecates the sense of most bug-spotting warnings in production code.

Do feel I want convince you :) we are entitled to have different opinions. I 
will run your idea through other maintainers. However bear in mind that most of 
us are freaks around test readability. The more explicit typing, the less 
essence is visible in the test. Hence I cannot guarantee we will change 
anyMap().

You can create your own anyMap() method and statically import it to the tests 
you like. 

Cheers ;)

Original comment by szcze...@gmail.com on 31 Jul 2010 at 9:11

GoogleCodeExporter commented 8 years ago
It would be good if "anyMap"could work in exactly the same way as 
Collections.emptyMap() and Collections.EMPTY_MAP.

In many situations, the compiler will be able to figure out which generic types 
are required. Where the compiler can't figure it out, you can use this syntax:

Collections.<String, String>emptyMap()

I don't propose that we need an ANY_MAP field seen as Mockito is strictly Java 
5 and above.
anyMap() gets changed to:

    public static <Key,Value> Map<Key,Value> anyMap() {
        return (Map<Key,Value>)reportMatcher(Any.ANY).returnMap();
    }

Because of Type Erasure 
(http://java.sun.com/docs/books/tutorial/java/generics/erasure.html) the same 
object can be used to represent all empty Maps. However, I notice that at the 
moment HandyReturnValues.returnMap returns a "new HashMap()" anyway.

Original comment by david.j....@googlemail.com on 10 Aug 2010 at 1:06

GoogleCodeExporter commented 8 years ago
Hey,

I'm afraid we cannot go for public static <Key,Value> Map<Key,Value> anyMap() 
because:

1. it will make some existing tests not-compiling
2. it will force the user to always use casting (or generifying) even if the 
user does not like it and prefers cleaner tests + @SuppressWarnings

Cheers

Original comment by szcze...@gmail.com on 14 Aug 2010 at 2:51

GoogleCodeExporter commented 8 years ago
Perhaps I will create a wiki or blog post on good patterns around naughty 
type-safety warnings.

Original comment by szcze...@gmail.com on 14 Aug 2010 at 2:54

GoogleCodeExporter commented 8 years ago
Thanks. Anyway, after reading this thread more fully, I'm pretty much sold on 
just adding @SuppressWarnings to the whole test class ;)

Original comment by david.j....@googlemail.com on 14 Aug 2010 at 6:32

GoogleCodeExporter commented 8 years ago
Generic friendly aliases (e.g. anyCollectionOf()) have been added in 1.8.

But anyMapOf() is still missing and has been requested in issue #78 as well.

Any chance of adding it? Should I submit a patch or you still reject the idea 
of (generic) checked tests?

Original comment by gfo...@gmail.com on 25 Aug 2010 at 12:39

GoogleCodeExporter commented 8 years ago
Ok here is the patch (with tests) against trunk.

Original comment by gfo...@gmail.com on 26 Aug 2010 at 12:40

Attachments:

GoogleCodeExporter commented 8 years ago
Thanks for the patch. I'll ask other maintainers how do they feel about this 
change.

>you still reject the idea of (generic) checked tests?

sure :)

Original comment by szcze...@gmail.com on 2 Sep 2010 at 5:34

GoogleCodeExporter commented 8 years ago
The patch still applies cleanly in head. Any news on this one?

Original comment by gfo...@gmail.com on 20 Jan 2011 at 10:23

GoogleCodeExporter commented 8 years ago
Done there : 
http://code.google.com/p/mockito/source/detail?r=bbefdd03ae7a353cae23f27722935fe
456880e68

Original comment by brice.du...@gmail.com on 9 May 2011 at 9:24

GoogleCodeExporter commented 8 years ago
Might be interesting to add anyVarargOf too.

Original comment by brice.du...@gmail.com on 9 May 2011 at 9:33

GoogleCodeExporter commented 8 years ago

Original comment by brice.du...@gmail.com on 5 Mar 2012 at 4:13

GoogleCodeExporter commented 8 years ago

Original comment by szcze...@gmail.com on 13 May 2012 at 3:37

GoogleCodeExporter commented 8 years ago

Original comment by szcze...@gmail.com on 3 Jun 2012 at 2:06