The Importance of Overridding Equals

I'm trying to write JUnit tests for some code that uses classes defined in Netscape's Lightweight Directory Access Protocol (LDAP) Software Development Kit (SDK) and I am having difficulties because the authors of several classes did not override equals.

For example, suppose I want to write a JUnit test for a method that returns an LDAPModificationSet. My first attempt is as follows:

public void testGetModifications()
{
  final LDAPModificationSet expected = // some expected modifications
  final LDAPModificationSet actual = classUnderTest.methodUnderTest();
  assertEquals(expected, actual);
}

No matter how hard I try, this test will always fail. LDAPModificationSet does not override equals, so its equals method only returns true when passed the exact same instance.

Consider the alternatives for my test:

• Assert something weaker. For example, instead of assertEquals(expected, actual), I could use assertEquals(expected.size(), actual.size()).
• Write my own method that compares LDAPModificationSet instances. For example, I could create an LdapModificationSets class with a method boolean equals(LDAPModificationSet firstSet, LDAPModificationSet secondSet).
• Have my test manually compare each element in LDAPModificationSet. (Although that's not even possible in this case, because each element is an LDAPModification, and that class also doesn't override equals.)

None of these alternatives are particularly fun.

The moral of this story: Override equals in value objects! The second moral of this story - buy "Effective Java: Second Edition" because Joshua Bloch already made this point in Item 8 when he wrote the book!

Difficulty Testing With Static Objects

I was going through some old code the other day improving test coverage when I came across an untested class that looked like this:

public class Foo
{
  private static final String KEY = "SOME_KEY";
  private final Preferences preferences;
  public Foo()
  {
    this.preferences = Preferences.systemNodeForPackage(
          SomeOtherClass.class);
  }
  public void disable()
  {
    this.preferences.putBoolean(KEY, false);
  }
  public void enable()
  {
    this.preferences.putBoolean(KEY, true);
  }
  public boolean isEnabled()
  {
    final boolean defaultValue = false;
    return this.preferences.getBoolean(KEY, defaultValue);
  }
}

Do you see anything wrong yet?

Nothing looks terribly wrong, but there's a lurking problem - it's hard to test the class because it calls a method that returns a static object. Let me explain by going through the process of adding tests of this class.

My first test method confirms that isEnabled returns false after disable is called:

public class FooTest extends TestCase
{
  public void testDisable() throws Exception
  {
    final Foo foo = new Foo();
    foo.disable();
    assertFalse(foo.isEnabled());
  }
}

There's still apparently nothing wrong. As expected, this test passes, so let's add a test of the enable method:

public class FooTest extends TestCase
{
  public void testDisable() throws Exception
  {
    final Foo foo = new Foo();
    foo.disable();
    assertFalse(foo.isEnabled());
  }
  public void testEnable() throws Exception
  {
    final Foo foo = new Foo();
    foo.enable();
    assertTrue(foo.isEnabled());
  }
}

Everything still works fine. Both tests pass, so let's add a test that confirms that the property defaults to false immediately after construction:

public class FooTest extends TestCase
{
  public void testDisable() throws Exception
  {
    final Foo foo = new Foo();
    foo.disable();
    assertFalse(foo.isEnabled());
  }
  public void testEnable() throws Exception
  {
    final Foo foo = new Foo();
    foo.enable();
    assertTrue(foo.isEnabled());
  }
  public void testConstructor() throws Exception
  {
    final Foo foo = new Foo();
    assertFalse(foo.isEnabled());
  }
}

Woah - the assertion in testConstructor fails! Even though each Foo instance has its own Preferences member, each member refers to the same Preferences object. Each member gets its value from a static method (Preferences.systemNodeForPackage) that returns the same instance every time it's called by the Foo constructor. This makes our tests lack independence - the call to foo.enable in testEnable impacts the Foo instance in testConstructor.

In my next post, I'll explain how to use a package-private constructor to handle this problem.