Re: [EqualsVerifier] Confused behaviour with/without usingGetClass

[Jan Ouwens]

Hi Lars,

First of all, I have to admit, the use case of a class that doesn't override equals/hashCode is probably not handled optimally in EqualsVerifier. After all, why would you test the equals method of a class that doesn't have one? You have a point, maybe EqualsVerifier should simply fail when equals/hashCode aren't overridden.
On the other hand, the equals method that is inherited from Object is not wrong, and why should EV fail if it's not wrong? Also, I have to consider backward compatibility; I don't want existing tests to suddenly start failing because of this. So, this is something that I will have to think about.


Now for your actual question.

The difference comes from the way instanceof and getClass() behave when inheritance comes into the mix. In its default behaviour, EqualsVerifier will use some bytecode manipulation trickery to construct an "anonymous" subclass of the class you're testing, to see if they can be equal to each other. The subclass basically looks like this:

  public class SubDummy extends Dummy {
    public Dummy(String name) {
      super(name);
    }
  }

Then, EV instantiates two objects, essentially like this:

  Dummy a = new Dummy("myname");
  Dummy b = new SubDummy("myname");

Then, when a.equals(b) is called, of course this returns false, because the equals method inherited from Object simply checks if the hashCodes are equal, and they aren't. But unless you specify usingGetClass(), EV assumes that you use an instanceof check in your equals method. This implies that a Dummy object should be equal to an instance of SubDummy, if their fields have the same values, which they do. (This is necessary, for example, when you use Hibernate, which also uses some bytecode manipulation to construct anonymous subclasses of your entity classes. You still want those to be able to be equal to the objects that you instantiate yourself.) In other words, EqualsVerifier assumes that a and b should be equal, and they're not. That's why your second test fails.

When you use getClass() in your equals method, you basically say that subclasses can never be equal. EqualsVerifier knows this, and won't even try to check the subclass--and that's why your first test succeeds.


Also, the call to allFieldsShouldBeUsed() has no effect here. You haven't overridden equals, so every Dummy object is un-equal to every other Dummy object, and therefore you have tricked EV into believing that changing the value of a field also influences the outcome of equals and hashCode().


Hope this helps!


Regards,
Jan

On 7 September 2012 15:56, Lars Briem <briem...@googlemail.com> wrote:

Hello,

I am a bit confused about the effects of usingGetClass. In the following class equals and hashCode are not overridden.

public class Dummy {

    private final String name;

    public Dummy(final String name) {
        this.name = name;
    }
}

When I now write tests for this class I expect different behaviour in two tests which I am not able to explain.

Test 1:
EqualsVerifier.forClass(Dummy.class).allFieldsShouldBeUsed().usingGetClass().verify();

Test 2:
EqualsVerifier.forClass(Dummy.class).allFieldsShouldBeUsed().verify();

The first test runs without any failure, but the second one fails with this error:

java.lang.AssertionError: Subclass: object is not equal to an instance of a trivial subclass with equal fields:
 test.equalsverifier.Dummy@f0c85e
Consider making the class final.
For more information, go to: http://code.google.com/p/equalsverifier/wiki/ErrorMessages
    at nl.jqno.equalsverifier.util.Assert.fail(Assert.java:96)
    at nl.jqno.equalsverifier.EqualsVerifier.handleError(EqualsVerifier.java:359)
    at nl.jqno.equalsverifier.EqualsVerifier.verify(EqualsVerifier.java:336)
    at test.equalsverifier.DummyTest.failingTest(DummyTest.java:15)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
    at java.lang.reflect.Method.invoke(Method.java:597)
    at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:44)
    at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:15)
    at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:41)
    at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:20)
    at org.junit.runners.BlockJUnit4ClassRunner.runNotIgnored(BlockJUnit4ClassRunner.java:79)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:71)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:49)
    at org.junit.runners.ParentRunner$3.run(ParentRunner.java:193)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:52)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:191)
    at org.junit.runners.ParentRunner.access$000(ParentRunner.java:42)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:184)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:236)
    at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:50)
    at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:467)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:683)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:390)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:197)

Where does this difference come from?
I think the different behaviour of the two test cases is wrong. In my opinion the first test tells me, that I do not have to override the equals and hashCode methods which indeed is not correct. So EqualsVerifier should fail in both cases.

Is there something wrong in EqualsVerifier or do I just not understand why this behaviour is correct?

Best regards,
Lars Briem

[Tim van Heugten]

Hi,

Reading up on this discussion I must say I was surprised to see the green outcome of the unit test for this Dummy class.

Since EqualsVerifier is called with allFieldsShouldBeUsed(), I assumed that EqualsVerifier would get that the object is equal if (and only if) all fields are equal. Apparently this is not the case.
Instead, allFieldsShouldBeUsed() instructs EqualsVerifier to verify inequality in the case of different fields, the 'and only if' part (am I right?).

I understand that EqualsVerifier primarily checks the contract between the equals and hashCode methods, which are sound in their default implementation. However, with allFieldsShouldBeUsed() I think it would be appropriate to assume (and verify) equality when all fields are equal. Can I herewith submit this feature request ;).


Cheers,

Tim van Heugten

On Mon, Sep 10, 2012 at 9:35 AM, Lars Briem <briem...@googlemail.com> wrote:

Hello Jan,

thanks for your fast answer. It helped a lot. Here is a short describtion where I think EqualsVerifier should fail, when there are no equals/hashCode methods.

I try to use TDD when ever possible, so I started writing a test for my class. Because I knew that my class will get equals/hashCode methods, I wrote a tests which checks this. The test should fail and then I implement the methods. This is the short time, where I want to test equals/hashCode of a class which does not have one.

Another case could be, that someone wants to guarantee, that a class has equals/hashCode methods.

I hope this ideas can help :)

Best regards,
Lars

[Jan Ouwens]

Hi Tim, Lars,

I think I agree, and I would like to make EV fail when equals is
inherited directly from Object. There is little point in using
EqualsVerifier if you don't override equals, after all. However, this
breaks backward compatibility, and I know there is at least one user
who relies on the current behaviour. So I can't change it -- at least,
not in a 1.x version. I will put it on the list for version 2.0.
(Another thing that's already on this list which is relevant here, is
to make allFieldsShouldBeUsed() the default, which you can disable
using suppress).

Please don't read this statement as a guarantee that this version will
be released any time soon though, because it won't be :).

I also agree that the behaviour of allFieldsShouldBeUsed() is
confusing in this case. I agree that it should fail on the name field
in the Dummy class. So if you would create a new issue for this in the
issue tracker, I will schedule it for the next bugfix release.


Regards,
Jan

[Jan Ouwens]

On Tuesday, September 11, 2012 9:05:14 AM UTC+2, Jan Ouwens wrote:

I also agree that the behaviour of allFieldsShouldBeUsed() is
confusing in this case. I agree that it should fail on the name field
in the Dummy class. So if you would create a new issue for this in the
issue tracker, I will schedule it for the next bugfix release.

As you may already have noticed, I just released version 1.1.4, which solves this issue.


Regards,
Jan