When refactoring some code recently, my pair and I came across yet another reason why state based testing is bad.. and even more.. why testing using access to private state is incredibly bad.

We were doing some serious refactoring…. tackling a class that had become a catch-all… thousands of lines and several dozen methods… and splitting it up into multiple classes, each with a single responsibility.

We did this by creating new classes and moving groups of methods and associated instance variables to the new class, and leaving delegating methods in the original class. These we will clean up on the next pass.

We did this for one responsibility and suddenly tests that had been written long ago started to fail. This perplexed us since the original methods remainded in place in the original class, simply delegating to the real methods in the new class.

A quick exploration of the failing test revealed what was happening. The test in question was using reflection to get access to private fields in the class being tested. This went both ways… private fields were being set with fake values for purposes of the test, and assertions were being made on the values of private fields.

This is evidence of a serious misunderstanding of how you should be writing tests: they should be based on behaviour and function.. not state. An object’s state is simply an implementation detail, and should not be exposed to, or relied upon by, other objects. Guess what.. that means no getters & setters, either. But that’s a discussion for another time.

The problem that we ran into with accessing private fields in a test is that it makes the tests brittle, throwing a monkey wrench into the process of refactoring. When we were refactoring we made a change that did not effect the behaviour. That should NOT cause tests to break. But it did.

Why? Because those tests were accessing a private field of the class via reflection.. and we had moved that field to a new class.

Example:

Let’s start with the example from the first paper in the references section below:

class FieldTest {
  public String publicString = "Foobar";
  private String privateString = "Hello, World!";
}

We’ll pull in the test from that article as well (even if it is contrived):

...

FieldTest f = new FieldTest();
String s = (String)PrivateAccessor.getPrivateField(f, "privateString");
assertEquals("Hello, World!", s);

Now, say we add some behaviour that uses the private field:

public String makeHeader() {
  return "<h1>" + privateString + "</h1>";
}

so now we see some behaviour that doesn’t belong in the class (just go with me on this).. creating the header.. so we can extract that and the variable that only it uses to a new class:

class HeaderCreator {
  private String privateString = "Hello, World!";
  public String makeHeader() {
    return "<h1>" + privateString + "</h1>";
  }
}

class FieldTest {
  public String publicString = "Foobar";
  public String makeHeader() {
    return new HeaderCreator().makeHeader();
  }
}

Now the test that relies on accessing privateField will fail since we’ve, quite legitimately, moved it.

So, it’s a bad idea to write tests based on the state of an object, especially when that state is only available in private fields. That being said, don’t do it..and if you are.. stop doing it… and fix where you did do it.

References

1. Subverting Java Access Protection for Unit Testing by Ross Burton (NOTE: this article presents what NOT to do)
2. State vs Interaction Based Testing by Nat Pryce
3. Mocks Aren’t Stubs by Martin Fowler