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! :+1: 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...

[cpovirk]

We have some text for this in an internal doc for the TruthSelfEquals cleanup. I should also find what kevinb wrote long ago.

[cpovirk]

More text available in a doc for... the SelfComparison cleanup :)

kevinb had said:

A test invokes the code under test and gets a result. Truth helps the test check that result. The idea of assertThat(Predicate) goes against that: the test hands the code under test off to something else to invoke it. That makes it similar to our *Tester classes and, imho, out of scope for Truth.

[cpovirk]

Kevin added some docs in https://github.com/google/truth/commit/ffde1698e226375a5fe90883bbd1a4849f9c05f1. I think we could still do more here, including putting something in the FAQ, but I think Kevin's commit is enough to remove this from the 1.0 blockers. (Thanks, Kevin!)

[cpovirk]

Here's a Twitter thread (scroll up, too) with some discussion of using assertion libraries' "equal to" methods for testing equality.

[cpovirk]

Another factor here is that, even if we change isEqualTo in as many Subject implementations as we can, we won't be able to change every custom Subject in the wild. As a result, a given assertThat(foo).isEqualTo(foo) might test Foo.equals today, but it might stop doing so as soon as you import FooSubject.assertThat(Foo) (or as soon as someone adds an assertThat(Foo) overload to a class whose assertThat methods you import already).

Now, it's fair to say that any custom Subject can have any arbitrary bug, so the sorts of changes I'm talking about can always cause problems. But our policy is that a Subject implementation doesn't need to check that equals works, so a custom Subject that omits such checking isn't "buggy." The only principled way to go here would be to state that every Truth subject should check the implementation of equals (which might mean arbitrarily complex things, like EqualsTester or MapEqualsTester). (Note that we can't even make such a change today without inconveniencing people, since plenty of tests have come to rely on the exact current implementations of isEqualTo!)

We don't have to be principled: We could still check things opportunistically. But that will encourage people to depend on those checks, and those checks might stop working (or at least switch to working in a different set of cases than they used to), and then people may silently lose test coverage. (This has happened already!)

So we just call equals testing out of scope. That at least lets us give clear advice to users and place no further burdens on authors of custom Subject implementations.