argThat incompatible with Hamcrest 1.2 and upwards for generic classes

[GoogleCodeExporter]

Note the following code.
Most matchers in Hamcrest 1.2 and newer return <? extends T>, as opposed to 1.1 
matchers which returned T.

As a result, argThat, which expects a Matcher<T> and returns T, doesn't compile.

See the attached project for a full example including imports and a maven 
project file.

public class Hamcrest121Generics {

    public void stubAndVerify() {

        Collection<String> col = new ArrayList<String>();
        col.add("Abe");
        col.add("Bob");

        assertThat(col, hasItems("Abe", "Bob"));

        Container container = mock(Container.class);

        verify(container).setNames(argThat(hasItem("momo")));
    }

    static class Container {

        public void setNames(Iterable<String> name) {}
    }

}

Original issue reported on code.google.com by shai.yal...@gmail.com on 8 Dec 2011 at 9:42

Attachments:

• mockito-hamcrest-bug.zip

[GoogleCodeExporter]

I think I considered upgrading to hamcrest 1.2 but doing that leads to problems 
because junit still ships with hamcrest 1.1 (AFAIR)

Original comment by szcze...@gmail.com on 8 Dec 2011 at 9:51

[GoogleCodeExporter]

JUnit 4.10, released recently, removed the dependency on Hamcrest.

Original comment by yoa...@gmail.com on 8 Dec 2011 at 9:54

[GoogleCodeExporter]

Cool. Still there are people using older versions of JUnit with mockito and we 
don't want to introduce problems in their environments.

Maybe there's a way to implement it safely - just mentioning that it's not just 
the matter of bumping hamcrest version. Plus, we're not very keen to distribute 
different versions of mockito paired with some specific version of dependency.

Original comment by szcze...@gmail.com on 8 Dec 2011 at 10:03

[GoogleCodeExporter]

Agreed, we can't make those changes, maybe it can be considered for a later 
mockito 2.0.

Original comment by brice.du...@gmail.com on 8 Dec 2011 at 10:20

[GoogleCodeExporter]

I really disagree. How about creating another method, "argThatMatches" that 
either adapts (using casting, if necessary) Hamcrest >=1.2 matchers to an 
argument matcher or uses a separate hierarchy that supports generic variance. 
Currently no one using Mockito can move to Hamcrest 1.2; v1.1 is almost 3 years 
old and lacks many of the features and matchers of 1.2. 

Original comment by shai.yal...@gmail.com on 8 Dec 2011 at 11:03

[GoogleCodeExporter]

>How about creating another method, "argThatMatches"

I think that would require 2 separate distributions because mockito has compile 
dependency on hamcrest.

I think that Mockito should simply drop the compile dependency on hamcrest. 
Instead of that we could offer some adapter methods that would have only 
runtime dependency.

Original comment by szcze...@gmail.com on 9 Dec 2011 at 12:04

[GoogleCodeExporter]

Seriously, all it takes is this:
    public static <T> T argThatMatches(Matcher<? extends T> matcher) {
        return reportMatcher(matcher).<T>returnNull();
    }

You can even keep the compile-time dependency on 1.1 for now, users will be 
able to drop in 1.2 or higher as a replacement, as it should be fully 
backwards-compatible in runtime as long as Mockito itself doesn't define any 
Hamcrest matcher, which it does. 

Original comment by shai.yal...@gmail.com on 9 Dec 2011 at 8:27

[GoogleCodeExporter]

Can you fork & spike it? Just want to make sure it's possible. Still not sure 
if I like a new method (e.g. would prefer to drop hamcrest)

Original comment by szcze...@gmail.com on 9 Dec 2011 at 9:05

[GoogleCodeExporter]

Already on it, will commit shortly

Original comment by shai.yal...@gmail.com on 9 Dec 2011 at 9:19

[GoogleCodeExporter]

I couldn't figure out if Google Code has a "pull request" feature like Github 
does, but here's my clone with the change committed:
http://code.google.com/r/shaiyallin-mockito/

Original comment by shai.yal...@gmail.com on 9 Dec 2011 at 10:46

[GoogleCodeExporter]

I think you need to use  the "ask for review" fearture.

Original comment by brice.du...@gmail.com on 9 Dec 2011 at 10:50

[GoogleCodeExporter]

Can't seem to find it... care to elaborate, or just review it manually?

Original comment by shai.yal...@gmail.com on 9 Dec 2011 at 11:11

[GoogleCodeExporter]

I just looked and in fact it doesn't seem to be working in similar way to 
github's pull requests.

As for the review : 
 - The method name argThatMatches isn't really good : I really don't want to write argThatMatches(hasProperty("a"))
 - Just to be sure, once packaged did you test this code with a project set with hamcrest 1.1, and another set with hamcrest 1.2 ?

Original comment by brice.du...@gmail.com on 9 Dec 2011 at 11:23

• Added labels: Type-Enhancement
• Removed labels: Type-Defect

[GoogleCodeExporter]

Can you suggest a better name?

I did run it with 1.1 and 1.3RC2. I'm not sure if I did it right, as your build 
configuration took me some time to understand (I'm not used to Ant + IntelliJ, 
I use Maven) but it definitely builds correctly with 1.1, which is all that 
matters for backwards compatibility. If I screwed up >1.2 support, we can apply 
another fix later.

Original comment by shai.yal...@gmail.com on 9 Dec 2011 at 1:29

[GoogleCodeExporter]

I agree with Brice on this one; argThatMatches is not a good name.

I loathe the idea of having two versions of this method for the two different 
cases.  I shudder to think of a
comparatively inexperienced user trying to work out whether a particular 
Matcher method is returning a 1.1-style 
or a 1.2-style Matcher, and choosing whether to write argThat or argThatMatches 
accordingly.

I think the person working on this should spend some time trying to work out 
whether it's possible to write one 
argThat method that will deal correctly with both cases.  Off the top of my 
head, it ought to be possible - after 
all, Matcher<T> is a subtype of Matcher<? extends T> - or is there something 
I'm not understanding here?

If it turns out not to be possible to deal with both cases with the existing 
syntax, then my vote would be to 
introduce an optional extra parameter to argThat().  So we'd have a method 
argThat( Matcher<? extends T> matcher, boolean newStyle ).  Supplying false for 
the second parameter should have
the same effect as using the existing argThat method.

But in my opinion, this solution should be considered a last resort.

Regards,
David Wallace.

Original comment by dmwallace.nz on 10 Dec 2011 at 6:33

[GoogleCodeExporter]

David's got a point. Please check out my latest push: 
http://code.google.com/r/shaiyallin-mockito/source/detail?r=d782a69555847b4bf98c
fe983e104662cc7a1a9d

I reverted argThatMatches and added an upper type bound to argThat. This 
compiles against Hamcrest 1.1 and 1.3RC2.

Original comment by shai.yal...@gmail.com on 11 Dec 2011 at 6:34

[GoogleCodeExporter]

Cool.

Not really related, but that would be great if we could setup some tests with 
different classpaths, eg with different versions of libraries.

Original comment by brice.du...@gmail.com on 12 Dec 2011 at 9:18

[GoogleCodeExporter]

So.... merge it?

Original comment by shai.yal...@gmail.com on 12 Dec 2011 at 3:08

[GoogleCodeExporter]

I'm hitting exactly this issue. I have harmcrest 1.2 because it is required by 
rest-assured library and having hard times with argThat() :(

Original comment by ar...@identified.com on 29 Feb 2012 at 5:45

[GoogleCodeExporter]

I looked at this issue a bit further and I actually experience the same capture 
error.

setNames(java.lang.Iterable<java.lang.String>) in Hamcrest121Generics.Container 
cannot be applied to (java.lang.Iterable<capture#566 of ? super 
java.lang.String>)

The only option I had was to introduce a cast anyway, same with Hamcrest 1.1, 
1.2.1, 1.3.RC2.

Original comment by brice.du...@gmail.com on 30 Mar 2012 at 9:00

[GoogleCodeExporter]

So, how about updating the dependency to Hamcrest 1.3? 

Original comment by shai.yal...@gmail.com on 11 Jul 2012 at 11:43

[GoogleCodeExporter]

Issue 361 has been merged into this issue.

Original comment by brice.du...@gmail.com on 1 Aug 2012 at 3:24

[GoogleCodeExporter]

Issue 361 has been merged into this issue.

Original comment by brice.du...@gmail.com on 1 Aug 2012 at 4:31

[GoogleCodeExporter]

Guys, can you please merge my fix into the main trunk? it is the only way I can 
see of supporting Hamcrest 1.2+. I also can't supply my own argThat() 
alternative from outside Mockito since I don't have access to the 
implementation details

Original comment by shai.yal...@gmail.com on 13 Aug 2012 at 12:49

[GoogleCodeExporter]

you can always do the following in the meantime:

@SuppressWarnings("unchecked")
public static <T> T argThat(Matcher<? super T> matcher) {
    return (T) Mockito.argThat(matcher);
}

Original comment by xavier.d...@gmail.com on 13 Aug 2012 at 12:58

[GoogleCodeExporter]

I'd add, although it's probably obvious, that even with it compiling ok, using 
argThat as it is currently (unless I'm doing something wrong) you lose the very 
useful diagnostic mismatch descriptions that hamcrest 1.2+ offers (and a huge 
part of what it's trying to achieve).  you just get "Actual invocation has 
different arguments" and toString output on what was passed.  So the only way 
to make sure the failure includes the diagnostics is to use ArgumentCaptor, 
then assertThat. (MatcherAssert.assertThat if you're on junit < 4.11).  

I realise it'd be pretty hard to do that and maintain compatibility with 1.1  
but would be great if this could work in eventual Mockito 2.0
(unless perhaps to check for matching args when within a verify you actually 
just called MatcherAssert.assertThat and trapped the AssertionException instead 
of directly calling matcher.matches(..) ?)

Original comment by nic.infa...@gmail.com on 23 Nov 2012 at 3:15

[GoogleCodeExporter]

Original comment by brice.du...@gmail.com on 2 Dec 2012 at 10:16

• Added labels: Matchers

[GoogleCodeExporter]

The compilation problem in example class Hamcrest121Generics is not really 
related to the version of hamcrest. It's a problem with java generics. Any 
factory method with a signature like this:

public static <T> org.hamcrest.Matcher<java.util.List<? super T>> foo(T element)

yields compilation error when applied to argThat()

There are a couple workarounds, the easiest one is an extra cast:

argThat((List) foo(element));
//or:
verify(container).setNames((Iterable) argThat(hasItem("momo")));

The workaround is easy and cheap enough that I don't see a point of adding a 
new method to the api that accommodates Matchers with generic subtype. I'm 
thinking of closing this issue as "won't fix". If someone has any interesting 
idea how to improve interoperability with certain kinds of Matchers, please 
submit a new enhancement request. For example, printing mismatch might be a 
candidate for an enhancement request.

Thanks everyone for participation in the discussion!

Original comment by szcze...@gmail.com on 8 Dec 2012 at 10:15