-
Notifications
You must be signed in to change notification settings - Fork 255
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Document that isEqualTo is not for testing equals(), etc. #268
Comments
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?:) |
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 If For years, Java was emphasising the difference between reference ( Remember that Comparing to |
The The idea is: If you want to test method |
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. |
Other points:
|
@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 assertThat(getStringOnOption(1)).isEqualTo("qwerty"); // passes! ...just like we would if we used JUnit 4's But if we have a new data class called 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? |
@jbduncan Well said! That's exactly right. |
Thanks for clarification! 👍 I misunderstood what was the intention. |
@nymanjens has been running into this with |
@cpovirk with all your SPI refactorings and documentings, maybe now's a good time to address this? |
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... |
We have some text for this in an internal doc for the TruthSelfEquals cleanup. I should also find what kevinb wrote long ago. |
More text available in a doc for... the SelfComparison cleanup :) kevinb had said:
|
Kevin added some docs in ffde169. 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!) |
We've decided that Truth isn't in the business of handling the "act" part of "arrange, act, assert." To test an implementation of equals(), users can call equals() themselves. But that was the case way back when we originally added this. What really prompted this now is that I realized this change would chip away at "Not true that..."-style messages :) Additionally, I'm looking at making a similar change to IterableSubject, and I figure this is as good a time as any to be consistent in not handling bad equals() methods (which we already don't handle in MultimapSubject). Also, avoid NPE on null actual value. Relevant: google#268 [] ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=253994206
We've decided that Truth isn't in the business of handling the "act" part of "arrange, act, assert." To test an implementation of equals(), users can call equals() themselves. But that was the case way back when we originally added this. What really prompted this now is that I realized this change would chip away at "Not true that..."-style messages :) Additionally, I'm looking at making a similar change to IterableSubject, and I figure this is as good a time as any to be consistent in not handling bad equals() methods (which we already don't handle in MultimapSubject). Also, avoid NPE on null actual value. Relevant: #268 [] ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=253994206
- Don't let users declare `class FooSubject extends ComparableSubject<Foo>` if `Foo` is a raw type. (But, as [the tests show](https://github.com/google/truth/blob/f1f0a9aad6e8fb4f5d68ad2e5850a2c5ca2478fa/core/src/test/java/com/google/common/truth/ComparableSubjectTest.java#L221), you can still write `Truth.assertThat(foo).isLessThan(...)` and similar for a raw type `Foo`.) - Don't let users pass a null value to `isGreaterThan` and similar. `null` is forbidden by the contract of `compareTo`, but some implementation may have tried to support it anyway. Anyone who wants to test `compareTo` should be using `ComparatorTester`, anyway, not `isGreaterThan` and friends. (Compare [our policy on using `isEqualTo` (rather than `EqualsTester`) for testing `equals`](#268).) RELNOTES=n/a PiperOrigin-RevId: 518568376
- Don't let users declare `class FooSubject extends ComparableSubject<Foo>` if `Foo` is a raw type. (But, as [the tests show](https://github.com/google/truth/blob/f1f0a9aad6e8fb4f5d68ad2e5850a2c5ca2478fa/core/src/test/java/com/google/common/truth/ComparableSubjectTest.java#L221), you can still write `Truth.assertThat(foo).isLessThan(...)` and similar for a raw type `Foo`.) - Don't let users pass a null value to `isGreaterThan` and similar. `null` is forbidden by the contract of `compareTo`, but some implementation may have tried to support it anyway. Anyone who wants to test `compareTo` should be using `ComparatorTester`, anyway, not `isGreaterThan` and friends. (Compare [our policy on using `isEqualTo` (rather than `EqualsTester`) for testing `equals`](#268).) RELNOTES=n/a PiperOrigin-RevId: 518896343
Another factor here is that, even if we change Now, it's fair to say that any custom 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 |
This addresses some https://errorprone.info/bugpattern/JUnit3FloatingPointComparisonWithoutDelta warnings, and it moves us off the bug-prone 3-`double`-arg `assertEquals` overload. In some cases, it results in a slightly awkward mix of JUnit and Truth assertions. I think it's still worthwhile for the benefits, but obviously we can consider migrating to even more Truth assertions in the future. (We would have to be a little careful about mass migration of arbitrary assertions, since migration might [replace `assertEquals` with `isEqualTo` for testing `equals` implementations](google/truth#268), etc. But we could look at migrating, say, all equality assertions on `int` and similar types—assuming that we never use those assertions to differentiate between, say, `Integer(0)` and `Long(0)`, which Truth treats as equivalent....) PiperOrigin-RevId: 648713259
This addresses some https://errorprone.info/bugpattern/JUnit3FloatingPointComparisonWithoutDelta warnings, and it moves us off the bug-prone 3-`double`-arg `assertEquals` overload. In some cases, it results in a slightly awkward mix of JUnit and Truth assertions. I think it's still worthwhile for the benefits, but obviously we can consider migrating to even more Truth assertions in the future. (We would have to be a little careful about mass migration of arbitrary assertions, since migration might [replace `assertEquals` with `isEqualTo` for testing `equals` implementations](google/truth#268), etc. But we could look at migrating, say, all equality assertions on `int` and similar types—assuming that we never use those assertions to differentiate between, say, `Integer(0)` and `Long(0)`, which Truth treats as equivalent....) PiperOrigin-RevId: 649135877
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: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.
The text was updated successfully, but these errors were encountered: