Assertj-core: #isEqualTo(Object other) takes invalid shortcut

[nioertel]

Summary

Before PR #1767 (specifically commit 76762a3664b3af2101d580355a05c6f0b3aa46cc) an object x was not automatically considered equal to itself when using BDDAssertions.assertThat(x).isEqualTo(x);
By switching from

    if (o1 == null) {
      return o2 == null;
    }
    if (o1.equals(o2)) {
      return true;
    }

to calling java.util.Objects.deepEquals(o1, o2); we end up with

    if (o1 == o2)
        return true;
    else if (o1 == null || o2 == null)
        return false;
    else
        // defaults to o1.equals(o2) if type is not an array
        return Arrays.deepEquals0(o1, 2);

This means when testing a custom equals implementation of a class, BDDAssertions doesn't actually do what we expect it to do and we may oversee bugs in the application code.

Example

// fails with assertj 3.16.1; succeeds with 3.13.2; probably broken since 13.15.1
@Test
void testEquals() {
    Object x = new Object() {
        @Override
        public boolean equals(Object other) {
            return false;
        }
    };
    BDDAssertions.assertThat(x).isNotEqualTo(x);
}

[scordio]

Thanks for the detailed explanation here and in #1948, I will look into them.

[scordio]

Before PR #1767 (specifically commit 76762a3664b3af2101d580355a05c6f0b3aa46cc) an object x was not automatically considered equal to itself when using BDDAssertions.assertThat(x).isEqualTo(x);

Right and I think the previous behavior was wrong because it could violate the contract of equals, where it is mentioned:

for any non-null reference value x, x.equals(x) should return true

The custom equals in the example is also violating that requirement.

Honestly, I would not restore the previous behavior. @joel-costigliola what do you think?

[joel-costigliola]

I do agree with @scordio, it is very difficult to argue that given o1 == o2 then o1.equals(o2) could return false.
I don't think the previous behaviour intended to allow that (might date from the Fest Assert library AssertJ is based upon).

If you want to have total control over isEqualTo, you can register a comparator:

// fails as isEqualTo checks is: comparator.compare(actual, other) == 0
assertThat(frodo).usingComparator((o1, o2) -> 1)
                 .isEqualTo(frodo);

Anyway @nioertel thanks for the detailed issue, appreciated!
I'm curious what was the exact use case that led you to raise this issue? Maybe there is something we can do.

[nioertel]

I disagree.
AssertJ should not make any assumptions about the code under test regarding compliance with any contracts from the JDK docs. It should give the developer a chance to test whether the code under test fulfils these contracts.
Until this change, assertThat(a).isEqualTo(b) for single objects behaved exactly like a.equals(b), except that it added null-safety on top. In my opinion this was the correct way, as explained above.

Niels

[scordio]

It should give the developer a chance to test whether the code under test fulfils these contracts.

I missed this point in the issue description. Do I understand correctly that you would like to use isEqualTo to test if the equals implementation fulfills the contract? I would suggest to use EqualsVerifier for this purpose. AssertJ might help with that, but supporting equals testing was never the main target.

Anyway, playing a bit more with this topic and the equals implementation above, another way to see the issue is with the following example:

assertThat(x.equals(x)).isFalse(); // succeeds
assertThat(x).isNotEqualTo(x); // fails

More specifically:

x.equals(x); // false
java.util.Objects.equals(x, x); // true
java.util.Objects.deepEquals(x, x); // true

The shortcut is in the java.util.Objects.equals implementation:

    public static boolean equals(Object a, Object b) {
        return (a == b) || (a != null && a.equals(b));
    }

which states in the javadoc:

Returns true if the arguments are equal to each other and false otherwise. Consequently, if both arguments are null, true is returned and if exactly one argument is null, false is returned. Otherwise, equality is determined by using the equals method of the first argument.

but the sentence in bold is actually in contrast with the result above.

Same for deepEquals. We could argue that this is a JDK issue.

I am still not sure how to proceed.

[joel-costigliola]

There is no obvious answer here, either AssertJ isEqualTo fully sticks to equals semantics and would not perform null check or reference checks or it performs them and the array comparison ones.

I would keep the current behaviour for now until we are presented a real life use case or more arguments in favor of the old behaviour.

The current behaviour does not prevent alternatives approaches like EqualVerifier or a simple assertThat(x.equals(x)).isFalse();

Let's leave that issue open for the sake of discussion, happy to reevaluate our position in light of new arguments.

[scordio]

This topic has been also discussed in JDK-8196069.

[pitschr]

Hello maintainers.

Summary

Breaking change detected in code coverage. Self-reference check in equals() not recognized by isEqualTo(..) which leads to less code coverage.

Works with AssertJ 3.14.0
Not working with AssertJ 3.18.1

Example

class EqualToTest {

    @Test
    void test() {
        final var myObj = new MyObject();
        assertThat(myObj).isEqualTo(myObj);
    }

    private static class MyObject {
        @Override
        public boolean equals(Object obj) {
            // simplified check for demo purposes
            return this == obj;
        }
    }

}

Example with AssertJ 3.14:

Example with AssertJ 3.18.1:

Hope that helps and hope it will be fixed soon and there won't be a forced need for a workaround using an extra dependency like EqualsVerifier as AssertJ should recover the most important parts OOTB 👍

[scordio]

@pitschr AssertJ isEqualTo is not designed to provide full coverage for custom equals implementations. We usually suggest EqualsVerifier to achieve this.

A similar example has been discussed in #2025.

To be noted that only the reference check is not covered with the custom equals invocation. This is due to Objects.deepEquals() which takes care of that.

[pitschr]

Yeah, EqualsVerifier is a good library. I agree that the EqualsVerifier is superior when we need to check equals and hashCode. But I still think that AssertJ should call the real equals method even if self-reference to avoid unexpected behavior.

The missing coverage through isEqualTo(..) was just accidentally discovered. Now I am in the situation that I am not sure if there might be other methods/scenarios which are "not designed" for AssertJ. Especially when it was covered before.

It is good to have it clarified that this drawback was done consciously. I still hope that AssertJ can be improved in this area in future.

[scordio]

@nioertel @pitschr we evaluated this issue again and decided to address it. Feel free to have a look at #2114.