Testing methods – visibility / testability problem.

So, a big argument I’ve encountered in Java OOP is a design problem about testing methods that aren’t public. I shall give an example.

I am writing a virtual BlackJack game. I have a class CardDealer which deals cards. It has a method which works out if the deck needs reshuffling, with the following logic:

public class CardDealer implements ICardDealer {
  
  @Override
  public void dealCards(Collection<Player> players, Deck deck)  {
    shuffleIfRequired(players, deck);
  }

  private void shuffleIfRequired(Collection<Player> players, Deck deck) {
    // calculate if enough cards left undealt in deck and if not shuffle...
  }
}

Now the problem is whether to test this method in isolation, and there are conflicting concerns:

1. Only test the Class against its public interface. Oh, well we can’t test the shuffleIfRequired method in isolation then. In my experience this is good and bad. I might have 5 ‘shuffleIfRequired’ scenarios I want to test and for each one I have to concern myself with the whole of the ‘dealCards’ method.

The answer – extract class (Refactoring by Martin Fowler). Using the logic given above should take this method out of this class, removing its context and then we can provide the functionality in another class, e.g.

public class Shuffle {
  public static void ifRequired(int players, Deck deck) {
    // context-free functionality here.
  }
}

Arguments against include ‘moving the responsibility out of the most appropriate class’, and ‘yet another utility class/method’.

2. Put testing above method visibility. This is typically done by making private methods protected or package-private / default access – to allow calling them from Unit tests (JUnit classes are typically in the same package). This argument can sound very bad to some people, their argument being that this waters down the design and leave the class open to misuse. There are 2 arguments for this as follows:

1. the code is more at risk from untested code than misused class, assuming code reviews.
2. the non-private, non-public method will not be part of the public interface of the class – being neither public nor in the interface.

Where the ‘test against contract’ enforces each test of shuffleIfRequired be a call to the ‘dealCards’ method, this approach means dealCards can be ignored – which some people accept and some people think is not so good.

3. Test private methods using reflection to invoke the method. This means hard-coding the method name into the test case and losing the power of refactoring tools and compile-time checking. There aren’t many arguments for this, but I think generally this is:

the test will break the first time the code is built locally and can be fixed easily enough depending on how much the method is called and how much it is changed.

Vinny’s Verdict:

I won’t tell people which of these to go with – I doubt I’ll change anyone’s opinion. But… I think it’s worth at least discussing here and hopefully this post can help focus discussions on it.