Document that isEqualTo is not for testing equals(), etc.

[cpovirk]

Our principle is:

When you call assertThat(foo), you're not testing any of the methods of foo. Rather, you're testing the result of some previous call:

UserId id =
    employeeService.manager("cpovirk");
assertThat(id).isEqualTo(
    UserId.of("jeremymanson"));

So if you use assertThat(foo).containsExactly("a", "b"), you're not testing foo.contains(), foo.containsAll(), or foo.iterator(). With isEqualTo(), it's the same: You're not testing equals(). The only difference between containsExactly() and isEqualTo() is that there's only one way to test equality but many ways to test collection contents. As a result, people assume that they can guess how isEqualTo() is implemented and then rely on it.

(Of course, it turns out that there's more than one way to test equality: You can check == first, as we do.)

We could make this "work." But I fear muddying the waters, encouraging users to depend on more implementation details (even implementation details that aren't what users guess :)). I could also make the case that we'll mildly hurt performance by always calling equals(), but I don't see that as a major concern in the absence of evidence.

[torbenbru]

Hey, newbie here who'd love to contribute. Rather seems like you're listing pros and cons of a change to me. Any other way I could contribute?:)

[shtratos]

I'm a simple library user, but the whole premise "isEqualTo is not for testing equals()" sounds strange to me.

Tl;DR IMO it'll be extremely confusing for many users to change isEqualTo not to use foo.equals() for equality check.

If .isEqualTo() is not calling foo.equals() that would be the least expected behaviour to me.
What other possible implementation it could have?

For years, Java was emphasising the difference between reference (==) equality and .equals() one.
And for tests, testing object equality with .equals() seems most sensible default choice unless you want to verify object identity.

Remember that Truth exists not by its own, but alongside libraries like Mockito and Hamcrest (btw we use both in our projects along with the Truth). I might be too old, but I can't unlearn the conventions set in those libraries that have sameInstance/same and equalTo/eq matchers to differentiate between 2 types of equality checks.

Comparing to contains check, I rely on Collection contract to guarantee consistent implementation of container concept i.e. it should not matter how exactly I check if my collection "contains" an object: with contains(), iterator(), stream() or containsAll(). Similarly, I rely on hashCode/equals contract to guarantee implementation of equality concept. Using some other contract for equality checks would be extremely surprising for me.

[cpovirk]

The contains example is a good one: Truth has to pick some way to check containment. If you're using a collection with a bad implementation of contains or iterator, Truth's contains might or might not catch that, since it might use some other method whose implementation is correct. Similarly, if you're using an object with a bad implementation of equals, Truth isEqualTo might or might not catch it.

The idea is: If you want to test method foo, then call method foo, rather than infer which assertions are likely to cause Truth to call foo.

[cpovirk]

It is certainly true that the name "isEqualTo" helps to encourage people to use the method in the precise way that I'm discouraging here. We had some good reasons for the name we chose, but I don't think we considered this problem at the time, and we may have made a suboptimal decision. Sorry about that.

[kevinb9n]

Other points:

• I don't think anyone was ever suggesting changing isEqualTo to be based on == instead of equals
• The idea that Truth doesn't test things, but just makes assertions about the results of the methods you called yourself, is the very center of its design.
• Testing whether equals is working correctly is a more complex job; we provide EqualsTester as part of guava-testlib for that.

[jbduncan]

@cpovirk @kevinb9n So am I right to understand that, for example, if we have the following method,

static String getStringOnOption(int option) {
  switch (option) {
    case 1: return "qwerty";
    ...
  }
}

...then we very well should use Truth's isEqualTo to "test"/assert that we get the result we expect, such as in the following example,

assertThat(getStringOnOption(1)).isEqualTo("qwerty"); // passes!

...just like we would if we used JUnit 4's assertEquals("qwerty", getStringOnOption(1));?

But if we have a new data class called NewClass with a custom equals/hashCode,

final class NewClass {
  String f1;
  int f2;
  
  @Override public boolean equals(Object o) {
    ...
  }
  
  @Override public int hashCode() {
    ...
  }
}

...then we shouldn't use...

assertThat(new NewClass()).isEqualTo(new NewClass());

...in a naive attempt to rigorously check that it meets the equals/hashCode contracts, and instead use something like guava-testlib's EqualsTester?

[kevinb9n]

@jbduncan Well said! That's exactly right.

[shtratos]

Thanks for clarification! 👍 I misunderstood what was the intention.

[dimo414]

@nymanjens has been running into this with MultimapSubject recently, where we use .equals() to check for equality, but .containsExactlyEntriesIn() to actually generate the failure message. If the multimap in question is mis-implemented this can lead to surprising results. It would definitely be good to document this more clearly somewhere, though I'm generally of the opinion that there's no hope in the face of classes that don't obey their own contracts.

[dimo414]

@cpovirk with all your SPI refactorings and documentings, maybe now's a good time to address this?

[cpovirk]

I am trying to focus on the things that require a bigger chunk of time while I have a bunch of time officially allocated to Truth, so this may continue to slip :( But eventually...