Using Package-private constructor to Test With Static Objects

In my previous post, I explained the difficulty of testing with static objects. This post shows how to use a package-private constructor to deal with that problem.

Here's the class we need to fix:

final class Foo
{
  private static final String KEY = "SOME_KEY";
  private final Preferences preferences;

  public Foo()
  {
    preferences = Preferences.systemNodeForPackage(SomeOtherClass.class);
  }

  public void disable()
  {
    preferences.putBoolean(KEY, false);
  }
  
  public void enable()
  {
    preferences.putBoolean(KEY, true);
  }
 
  public boolean isEnabled()
  {
    final boolean defaultValue = false;
    return this.preferences.getBoolean(KEY, defaultValue);
  }
}

This class needs fixing because it's hard to write independent tests because each Foo instance uses the same Preferences object. We fix the problem by introducing a package-private constructor:

final class Foo
{
  private static final String KEY = "SOME_KEY";
  private final Preferences preferences;

  public Foo()
  {
    this(Preferences.systemNodeForPackage(Foo.class));
  }

  Foo(final Preferences preferences)
  {
    this.preferences = preferences;
  }

  public void enable()
  {
    preferences.putBoolean(KEY, true);
  }

  public void disable()
  {
    preferences.putBoolean(KEY, false);
  }

  public boolean isEnabled()
  {
    final boolean defaultValue = false;
    return this.preferences.getBoolean(KEY, defaultValue);
  }
}

Note that the package-private constructor does not impact normal consumers of Foo in any way. Consumers continue to use the default constructor that (unbeknown to them) delegates to the package-private constructor.

Rather than use Foo's default constructor, our tests use the package-private constructor and Mockito to inject a mock Preferences object that each Foo instance uses:

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

Now testEnable fails because its Foo instance calls Preferences.putBoolean on a mock. By default, when a void method (such as Preferences.putBoolean) is called on a mock created by Mockito, the mock does nothing. Therefore, we need to change our testing approach.

To test a Foo method, we assert that the Foo instance delegates to its Preferences object. To test the Foo constructor, we assert that no delegation to its Preferences object has occurred:

public class FooTest extends TestCase
{
  private Preferences preferences;
  private Foo foo;
  public void setUp() throws Exception
  {
     this.preferences = mock(Preferences.class);
     this.foo = new Foo(preferences);
  }
  public void testDisable() throws Exception
  {
    this.foo.disable();
    verify(this.preferences).putBoolean(Foo.KEY, false);
  }
  public void testEnable() throws Exception
  {
    this.foo.enable();
    verify(this.preferences).putBoolean(Foo.KEY, true);
  }
  public void testConstructor() throws Exception
  {
    verifyNoMoreInteractions(this.preferences);
  }
}

Note that to test delegation, we changed Foo.KEY from a private member to a package-private member. That's a perfectly natural event that occurs during testing. Treat the test class as a first-class consumer of the production class and let the test class dictate the API of the production class.

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.

Using Java's CountDownLatch for multi-threading

CountDownLatch is a class that was added to the java.util.concurrent package in Java 1.5. As its Javadoc states it "allows one or more threads to wait until a set of operations being performed in other threads completes". I never had the chance to use it until this week, but now that I have, I fully appreciate its power!

A CountDownLatch is instantiated with a positive count and provides a countDown method that decrements the count. The await method "causes the current thread to wait until the latch has counted down to zero". (The maximum amount of time that await actually waits can be configured, although I didn't use that feature.)

I use a CountDownLatch when I am gathering results on a number of hosts and aggregating the results from each host into a single object that is returned to the caller. This is a perfect place to use a CountDownLatch because the number of hosts can be used as the count that is passed to its constructor.

Here's my code (with error-handling removed for brevity):

  public Results getResultsOnAllHosts()
  {
     final List<String> hosts = getHosts();
     final CountDownLatch countDownLatch = new CountDownLatch(hosts.size());
     final List<GetResultsOnHostThread> threads =
           getThreadsThatAreStarted(hosts, countDownLatch);
     waitForAllThreadsToFinish(countDownLatch);
     return aggregateResults(threads);
  }

  private List<GetResultsOnHostThread> getThreadsThatAreStarted(
        final List<String> hosts, final CountDownLatch countDownLatch)
  {
     final List<GetResultsOnHostThread> threads =
           Lists.newArrayListWithCapacity(hosts.size());
     for (final String host : hosts)
     {
        final GetResultsOnHostThread thread =
              new GetResultsOnHostThread(host, countDownLatch);
        thread.start();
        threads.add(thread);
     }
     return threads;
  }
  private void waitForAllThreadsToFinish(final CountDownLatch countDownLatch)
  {
     try
     {
        countDownLatch.await();
     }
     catch (final InterruptedException e)
     {
        Thread.currentThread().interrupt();
     }
  }
  class GetResultsOnHostThread extends Thread
  {
     private final String host;
     private final CountDownLatch countDownLatch;
     private ResultsOnOneHost resultsOnThisHost;
     GetResultsOnHostThread(final String host, final CountDownLatch countDownLatch)
     {
        this.host = host;
        this.countDownLatch = countDownLatch;
     }
     @Override
     public void run()
     {
        try
        {
           results = getResultsOnHost(this.host);
        }
        finally
        {
           countDownLatch.countDown();
        }
     }
  }