equalTo() matcher incorrectly optimises out the call to the real equals() method

[GoogleCodeExporter]

(This bug report applies to Hamcrest 1.1)

I've just been writing a unit test for my class's implementation of
equals() and realised that my equals method blew up if it was given a null
object to compare with (it should return false instead). My (JUnit) unit
test contained a line like:

assertThat(myObject, not(equalTo(null)));

I was surprised to discover that the unit test 'passed' before I had fixed
my equals() implementation.  It turned out that Hamcrest was not actually
calling my equals() method as it contains an optimised equals test that
skips call to my equals() implementation:

private static boolean areEqual(Object o1, Object o2) {
        if (o1 == null || o2 == null) {
            return o1 == null && o2 == null;
        } else if (isArray(o1)) {
            return isArray(o2) && areArraysEqual(o1, o2);
        } else {
            return o1.equals(o2);
        }
    }

This means that if either object is null, the call to o1.equals(Object) is
skipped completely as Hamcrest thinks it knows better.  This skips my code,
and makes my broken code look ok. 
(Code coverage didn't show this up as I had already covered the equals()
method in another non-null test.)

Original issue reported on code.google.com by Royst...@gmail.com on 16 Oct 2007 at 3:51

[GoogleCodeExporter]

You're misusing the assertThat method, really.  It's not intended to be used to 
test
the methods that it itself is calling.  Rather, it assumes that they work and 
are
being used to test something else.

A test for equals should call equals directly.  E.g.

 assertFalse(myObject.equals(null));

Original comment by nat.pr...@gmail.com on 16 Oct 2007 at 5:13

[GoogleCodeExporter]

I know what you're saying, but the difference in syntax is going to be a bit 
subtle
for most people. I'm now aware of the problem and will (have to) write my test 
code
differently, but most people won't realise that equalTo doesn't necessarily call
equals().

The other reason that this behaviour susprised me was that JUnit's 
implementation of
assertEquals() doesn't contain the same optimisation as Hamcrest's equalTo() 
method.
 So when translating existing logic, there's a big gotcha.

The only reason I found the problem was that I did the Right Thing(tm) of 
running my
tests [to ensure that they failed] before I fixed the problem.

Apart from the obvious (very tiny) performance benefit, is there a reason not to
change the behaviour of the Hamcrest equalTo matcher so that it's a) less 
subtle and
b) closer to the behaviour of the existing JUnit logic?

Original comment by Royst...@gmail.com on 22 Oct 2007 at 12:22

[GoogleCodeExporter]

Fixed in trunk.

Original comment by joe.wal...@gmail.com on 22 Oct 2007 at 7:12

• Changed state: Fixed