Are any... Matchers harmful?

[tibo...@googlemail.com]

Hi, 

Mockito provides matchers like any(Class c), anyString, anyInt, AnyList, anyListOf()...

Software developers rely on those to make useful assertions about method parameters, without having to specify actual values.

However, I find that all those matchers match null, and those that deal with Collections do not run typeChecks on the collection items.

The isA(Class c) method on the other hand does not accept null, which seems a much wiser choice in order to find likely NPE bugs at test time.

So it seems to me that developers should generally avoid those any... Matchers, and prefer matchers that also check against null and wrong collection members. It is possible to use isA(String.class), isA(List.class) etc. 

But I would prefer to have something like, to raise awareness that any() is generally a bad choice, since it tests less.

isAString, isAnInt, isAList, isAListOf

Do I miss something obvious?

regards,

  Thibault

[Francisco Olarte]

Hi Thibault:

On Fri, Mar 20, 2015 at 5:24 PM, <tibo...@googlemail.com> wrote:
> Mockito provides matchers like any(Class c), anyString, anyInt, AnyList,
> anyListOf()...
> Software developers rely on those to make useful assertions about method
> parameters, without having to specify actual values.
> However, I find that all those matchers match null, and those that deal with
> Collections do not run typeChecks on the collection items.
> The isA(Class c) method on the other hand does not accept null, which seems
> a much wiser choice in order to find likely NPE bugs at test time.

I do not grok the collection typechecks too well, but regarding
nullness, you use any() if nul is correct, isA if not, what is the
problem?

> So it seems to me that developers should generally avoid those any...
> Matchers, and prefer matchers that also check against null and wrong
> collection members. It is possible to use isA(String.class), isA(List.class)
> etc.

It is impossible to use them when null is an allowed value.

> But I would prefer to have something like, to raise awareness that any() is
> generally a bad choice, since it tests less.

and any(Object) is even worse, but you need them. The documentation is
explicit "Matches anything, including nulls", "Matches any object,
including nulls". Maybe a reference to use isA if null is allowed is
in order, but if people is not aware of it, it is not by lack of
advice, half of the four words in any() descriptions are for stating
it. I fail to see a problem with them.

> isAString, isAnInt, isAList, isAListOf
> Do I miss something obvious?

Do you mean you'll like isA* methods mirroring the any hierarchy? ( I
think they can be syntethised with AditionalMathers using and(any*(),
isNotNull()), but I see they could be useful.

Regards:
Francisco Olarte.

[tibo...@googlemail.com]

On Friday, March 20, 2015 at 6:32:37 PM UTC+1, Francisco Olarte wrote:

> isAString, isAnInt, isAList, isAListOf
> Do I miss something obvious?

Do you mean you'll like isA* methods mirroring the any hierarchy? ( I
think they can be syntethised with AditionalMathers using and(any*(),
isNotNull()), but I see they could be useful.

Yes, I think mirroring the any hierarchy would be useful. I believe that in general,
a check that does not accept nulls should be preferred over one that accepts null,
because I believe software should avoid nulls wherever possible (Java8 Optionals
show that trend in general).

There may be cases where null is valid and useful for an API, but that should be
the exception, rather than the rule. And for developers new to Mockito, the dominance
of the any...() API could make them lean towards using any...() when they should not.

cheers,
  Thibault

[Francisco Olarte]

Hi Thibault:

On Mon, Mar 23, 2015 at 10:47 AM, <tibo...@googlemail.com> wrote:
> Yes, I think mirroring the any hierarchy would be useful.

Well, me too.

> I believe that in
> general,
> a check that does not accept nulls should be preferred over one that accepts
> null,
> because I believe software should avoid nulls wherever possible (Java8
> Optionals
> show that trend in general).

Here I dissent with you. I believe that, when both types of checks are
available, a check with accepth nulls should be used when null is a
valid value, and one that doe not when it is not. No preferences
involved, using a null-accepting check where null is not valid is,
IMHO, incorrect. But bear in mind sometimes any() is not used for
checking, it's just used to pick the correct overload.

> There may be cases where null is valid and useful for an API, but that
> should be
> the exception, rather than the rule. And for developers new to Mockito, the
> dominance
> of the any...() API could make them lean towards using any...() when they
> should not.

Those are two unrelated things. Accepting nulls, or using
null-objects, or using Optionals, is one thing, and there may be
external forces driving it. Using any() when isA() is correct is
another one. Also, bear in mind Mockito is generally used in tests,
and many times you use any not because you want to check anything, you
know what is going in and do not care, you just use it to select one
method and make it return something, and the fluent API makes you use
any() if you do not know the exact value, or you do not care what it
is, and for you null is as incorrect as any arbitrary value of the
parameter class, i.e.,
when(myObject.setTittle(anyString()).thenReturn(true) when testing how
the user of the mock handles normall returns and .thenThrow(new
NullPointerException()) when testing how it handles NPE. I do not care
what it is passing, I'm testing its error handling.

Francisco Olarte.

[tibo...@googlemail.com]

On Monday, March 23, 2015 at 12:13:18 PM UTC+1, Francisco Olarte wrote:

> I believe that in
> general,
> a check that does not accept nulls should be preferred over one that accepts
> null,
> because I believe software should avoid nulls wherever possible (Java8
> Optionals
> show that trend in general).

Here I dissent with you. I believe that, when both types of checks are
available, a check with accepth nulls should be used when null is a
valid value, and one that doe not when it is not.

I guess that is what I meant. Or at least our positions are not that far from each other.

In any case, the many variants of any...() compared to just a single isA...() call led my team
to use any() in every place, even where null is not a valid value.

And I believe that is worse than if they had used isA() in a place where nulls are allowed,
because doing so would make the test fail when null is passed, so that wrong use of isA
defect would likely be found. The other way round is much harder to detect.

[Francisco Olarte]

On Mon, Mar 23, 2015 at 1:19 PM, <tibo...@googlemail.com> wrote:
> I guess that is what I meant. Or at least our positions are not that far
> from each other.

Understood. I agree with you there.

> In any case, the many variants of any...() compared to just a single
> isA...() call led my team
> to use any() in every place, even where null is not a valid value.

That is true.

> And I believe that is worse than if they had used isA() in a place where
> nulls are allowed,
> because doing so would make the test fail when null is passed, so that wrong
> use of isA
> defect would likely be found. The other way round is much harder to detect.

Correct. But what I was saying is some times isA == anyNonNull isn't
even correct. Following my invented example, if I'm testing with a
"TEST-TITLE" I should probably code
when(myObject.setTitle("TEST-TITLE")).thenReturn(true);
when(myObject.setTitle(any())).thenThrow(new
Assertionerror("WTF-internal test error, unexpected title"), as in
this case "OTHER-TITLE" is as bad as null ( I may say worse, in some
cases ).

In many cases test code is so short and mock usage is so constrained
that, IMO, is better to just code any() and lessen the complexity, let
the the brain center in the useful part, which in many cases is just
the stubbed method name and the thenXXX, as the parameters are just
noise ( If java admited things like 'method pointers' I would prefer
something like 'when(myObject.setTitle).thenXXXX' instead of the
any(), but that is not going to happen for a long time, until java 8+
and similars are widespread enough, and I think the new syntax in jdk8
is not going to be valid for this things. )

Francisco Olarte.

[Brice Dutheil]

I’d like to give additional info on this. The origin of these methods is they come from anything i.e. anything matches, later for shortness and cast avoidance the aliases grew, but the API naming thus became inconsistent with what a human would expect. So this behavior is being changed in mockito 2 beta, to be precise here’s the status on these API in the version 2.0.5-beta :

• any, anyObject, any(Class) won’t check anything (at first they were just aliases for anything and for cast avoidance)

• anyX like anyString will check the arg is not null and that has the correct type

• anyList will check the argument is not null and a List instance

• anyListOf (and the likes) at the moment are just aliases to their non generic counter part like anyList

Note this is work in progress (started here in #141), these new behavior can / will change in the beta phase. I’m especially wondering if the any familly should allow null and if not do a type check. For example with these matchers :

• any, anyObject stay the same, they currently allow null and don’t have to do type check anyway

• any(Class) currently allows null and doesn’t do type check => allows null and if not checks for the given type

• any<Collection>Of currently doesn’t allow null and does a type check of the collection, not elements => allows null, if not checks collection type, if not empty checks element type

Maybe extend the isA family that won’t allow any null arguments. Thoughts ?

— Brice

--
You received this message because you are subscribed to the Google Groups "mockito" group.
To unsubscribe from this group and stop receiving emails from it, send an email to mockito+u...@googlegroups.com.
To post to this group, send email to moc...@googlegroups.com.
Visit this group at http://groups.google.com/group/mockito.
For more options, visit https://groups.google.com/d/optout.

​

[David Wallace]

So your intention is that for many people, a whole lot of their tests will start failing when they upgrade from 1.x.x to 2.x.x?  These seem like a very odd set of changes to make.

[Brice Dutheil]

It's a major version, so to this question of backward compatibility, it's a yes. That's unfortunate, but it is needed.

This matcher API is broken anyway, time to fix it.

-- Brice

[Francisco Olarte]

Hi Brice..

On Sun, Apr 5, 2015 at 12:18 PM, Brice Dutheil <brice....@gmail.com> wrote:
> any<Collection>Of currently doesn’t allow null and does a type check of the
> collection, not elements => allows null, if not checks collection type, if
> not empty checks element type
>
> Maybe extend the isA family that won’t allow any null arguments. Thoughts ?

I like any() to be regular, all nulls or all not nulls, even if it
breaks some test on mayor upgrade. Regarding the isA, due to how it
works I would prefer them to be called anyNotNull*, or aNotNull*,
which IMO makes it clearer and also clarifies any*() usages by
contrast ( do not know how it will go for others, but if I see
anyNotNull* and any* in a chunk i would tend to assume any includes
null ).

Personally I prefer any() to accept nulls, as for me every 'object' in
Java is really a pointer an null is a valid pointer value, and also I
normally only use any where I really do not care what value is being
passed in. I doubt I have any place in my code where it does. When I
use any() I'm just using it for method selection to select the
returning value or exception, and when I need to test the value passed
in both any and isA are just too coarse and I need a concrete value or
to capture and test after it.

Francisco Olarte.

[Brice Dutheil]

Hey Francisco,

I agree, any family should be consistent on nulls, that’s my proposition. By the way I took my previous message and created an issue there #194.

I kind of like anyNotNull family, as you said it draws a clear behavior difference. There’s also the possibility to extend the notNull family.

— Brice

Francisco Olarte.

​

[Jim Mayer]

This is slightly off topic, but if Mockito 2.x is going to break compatibility I'd like to see Mockito matchers have distinct names from Hamcrest matchers. My project uses both and dealing with the name clashes is a pain!

Jim Mayer

[Francisco Olarte]

Hi Brice:

On Sun, Apr 5, 2015 at 8:48 PM, Brice Dutheil <brice....@gmail.com> wrote:
> I agree, any family should be consistent on nulls, that’s my proposition. By
> the way I took my previous message and created an issue there #194.

I've read it, and commented this ( I'm not familiar enough with gh to
know wether it needs to be done, so I repeat it here in case it's
useful to anyone ).

I see anyString() ( and friends ) checking for null and anyObject()
not doing it as an inconsistency potentially leading to user surprise.
Object is as much of a class as String ( or other ).

Other thing is I fail to see is the difference ( if there is any )
between isA(Klazz) and notNull(Klazz) ( or IsNotNull, but I see that
one is a forwarder ). To me they seem they do the same, given the
docs, but the code is different and I'm not familiar enough with it to
assert they do the same thing. When doing DSL helper libraries I've
found ( the hard way ) having slightly different names for the same
thing seems a good idea initially, but leads to problem along the way,
so I would vote for just having one of notNull/isNotNull ( and isA if
it is functionally equivalent ).

> I kind of like anyNotNull family, as you said it draws a clear behavior
> difference. There’s also the possibility to extend the notNull family.

You can assume I like anyNotNul ;->, but, after looking around a bit
and getting confused by the NotNul/isNotNull/isA stuff, I'll prefer to
have just one way to do it ( feel free to use the name, I still like
it, but the multitude of options puzzles me ). In fact, if it were my
call I would relegate anyString and maybe the non-primitive behaviour
of anyBoolean and friends to additionalMatchers. As I'm not a big fan
of overlloading I normally get along with any()/any(klass), and all
that synonims just confuse me ( I mean, let anyBoolean for a primitive
boolean, leave any(Booolean.class) and isA(Boolean.class) for the
object versions ). After time I've found kb chars are cheap and I'm
better served by a small set of orthogonal primitives, so TI!MTOWTDI (
of course, all that helpers can be useful for someone, and the only
problem for me is they make the JD page huge, so no biggie there ).

As someone has already commented, major version, api breakage, so I
think now is the time to expose my thoughts.


Francisco Olarte.

[Francisco Olarte]

Hi Jim:

On Mon, Apr 6, 2015 at 12:57 AM, Jim Mayer <j...@pentastich.org> wrote:
> This is slightly off topic, but if Mockito 2.x is going to break
> compatibility I'd like to see Mockito matchers have distinct names from
> Hamcrest matchers. My project uses both and dealing with the name clashes is
> a pain!

First, do not take the folowing personally.

Second, as I use Mockito but do not use Hamcrest, I would prefer to
have the 'good' names in Mockito ( for some definition of good, this
means I would use whichever ones sound better disregarding H. usage of
them ).

Francisco Olarte.

[Jim Mayer]

No problem Francisco!

Here's my reasoning, though:

1. Many people use Mockito in conjunction with JUnit.
2. The trend with JUnit seems to be to use "assertThat(value, matcher)" in preference to "assertTrue", "assertEquals", etc.
3. Anyone using "assertThat" is using Hamcrest matchers.

For example, "any" is commonly used and is present in both the Hamcrest and Mockito matcher classes:

• org.hamcrest.Matchers.any
  
• org.hamcrest.Matchers.any

This is particular problematic since not only is there a conflict on "any", but there's also a conflict on the class that defines "any" so we can't even reliably say "Matchers.any" and know what is means.

Hamcrest predates Mockito by quite a long time, so I doubt they'd want to change their names.

At the very least, it would be useful if the class names didn't conflict! Then one could say any of the following:

import static org.mockito.MockitoMatchers.somethingCool;

...

assertThat(result, Matchers.somethingCool(...));

or

import static org.hamcrest.Matchers.somethingCool;

...

verify(something).myMethod(MockitoMatchers.somethingCool(...));

or

assertThat(result, Matchers.somethingCool(...));

verify(something).myMethod(MockitoMatchers.somethingCool(...));

Jim Mayer

Francisco Olarte.

[Stephen Duncan Jr]

Actually, many people using assertThat might be using assertj: http://joel-costigliola.github.io/assertj/  Personally, I far prefer it, and avoid Hamcrest now.  My integration with Mockito & assertj mostly comes just via argumentCaptor.  Not sure there's really any better way, though I'd be interested in ideas.

I do agree that fixing the class names matching would probably be helpful, if we're proposing non-backwards-compatible changes.

[Francisco Olarte]

Hi Jim:

On Mon, Apr 6, 2015 at 9:25 PM, Jim Mayer <j...@pentastich.org> wrote:
> No problem Francisco!

Thanks.

> Here's my reasoning, though:
>
> Many people use Mockito in conjunction with JUnit.
> The trend with JUnit seems to be to use "assertThat(value, matcher)" in
> preference to "assertTrue", "assertEquals", etc.
> Anyone using "assertThat" is using Hamcrest matchers.

Well, in my case I use TestNg and assertThat, butnot the one from
Hamcrest but the one from fest.

> For example, "any" is commonly used and is present in both the Hamcrest and
> Mockito matcher classes:
> org.hamcrest.Matchers.any
> org.hamcrest.Matchers.any

Finger slip, I assume, but I see your point. This kinds of things
always remind me of why I hate Java as a language, too simple
compilers. I've always wondered why we it does not have a 'import
xxxxxxx as A, import yyyyyyy as B', so you do not have to choose
between a FULL and tiresome to type name or an naked and unqualified
one.

> This is particular problematic since not only is there a conflict on "any",
> but there's also a conflict on the class that defines "any" so we can't even
> reliably say "Matchers.any" and know what is means.

.... snipped.....

I completely see your point. Mine is a ( probably selfish ) different
one. I try to use as little libraries as posible ( not so much in
testing ), and try to pick ( what I consider ) best of breed. I then
want those ones to have the easier names, not in the sense of shorter
or easier to type, but the one which impose less memory load, as I'm (
presently ) overwhelmed but too many languages and libs.

Also, I think there is a way of renaming them, IIRC. Even the main
Mockito class does it IIRC. If, in some accesible place ( even at the
top of your test file ) you put a 'class MM extends
org.mockito.Matchers {} class HM extends org.hacrest.matchers {}' ( I
think this can be done in many ways, shared test helper class in a
file, private internal class, whatever ) you can use them as
namespaces and use MM.any()/HM.any() ( being in the tests you do not
care about their bloat, which sould be minimal ). I normally prefer to
use this kind of things ( which are orthogonal tricks which can be
used in any scenario ) than trying to invent new names ( I think it
was Phil Karlton who said 'There are only two difficult things in
computer science: cache coherence and naming things', and this is
without the added complexity of not duplicating the unqualified one
with another class. Also, any is IMO a good name. I would not give it
away, given Hamcrest users have easy ways to avoid clashes ).

There is also another possiblity, make a method mockitoXXXX for each
XXXX (or just moXXXX), implemented and documented as just a forwarder.
This ways the mockito user ( which is the one I'm caring about
presently ), can easily access them on name clashes ( and then tell
hamcrest to do the same. It does not solve your present problem, but
may be we can start a trend. Also, presently mockito collides with
Hamcrest today, but it can collide with anyone tomorrow, so the
renaming solution may impose a cost on user and be what we call 'pan
para hoy y hambre para mañana' here ( that means 'bread for today,
hunger for tomorrow' ).

Ok, drifting away. To close this, I understand your arguments and
think they have merit, but think having the nice names in mockito
vastly overweights them, and given collisions can be trivially solved
with many tricks my money is on on getting them.

Regards.
Francisco Olarte.

[rogerdpack]

I don't know too much about Hamcrest, but I would agree that

anyObject()

Matches anything, including null.

is very odd...anyObject but it matches null? Surprising.

Cheers!