Roy Osherove

View Original

Unit test readability should not come in place of maintainability - they go hand in hand

Brian Button writes this post about a best practice he believes in when writing unit tests.
 
The main idea in that post is that when you use Setup and TearDown methods in your tests, you're essentially separating parts of the unit tests into 2 or three areas in the class, thus making the person reading the tests more likely to miss out on some subtleties inside the test method. In short, the test is less readable if there is a "hidden" setup  that occurs before it happens. He also notes this best practice is valid for things that are *not* resource based (such as initializing things inside the file system etc..). The final thought in that post says that you would rather have a little duplication in your tests rather than remove readability.
 
While I do believe that using Setup makes the test less readable, I do not believe that a little duplication is OK.
The problem with code duplication in your tests is that it soon (very soon) leads to unmaintainable tests.
   You're creating an object instance inside 20-50 tests in your test fixture, and now suddenly the design changes and the class constructor signature changes. Do you really think fixing 50 tests is something a developer will take the time to do? Maybe once. What about the second time this happens? and the third? And what specific point in time will this developer start thinking " Gosh darn it, these tests are more trouble than their worth, and I have a deadline to reach!" and finally let the tests keep on failing?
 
Sooner than you think.
 
I've seen this happen time and time again. With Unit tests the slightest duplication of code is orders of magnitude a bigger problem than duplication in production code might be, because tests come in large numbers on each class or even function.
 
So, does all this mean you should still use Setup and TearDown? Not necessarily. You can still remove duplication and  keep your tests fairly readable by following a few simple rules.
The three rules of test duplication are:
  1. Never instantiate objects inside a test - always use a factory method
  2. Refactor configuring(preparing for test) object state into shared methods (not static, but common to be used by multiple tests)
  3. Refactor duplicated Act-Assert combinations into common methods (very methods)
Never instantiate objects inside a test - always use a factory method
instead of
[Test]
public void Create()
{
MyOBject x = new MyObject();
} 
 
do:
[Test]
public void Create()
{
MyOBject x = makeDefaultMyObject();
}
 
private MyObject makeDefaultMyObject()
{
return new MyOBject();
} 
 
 
Refactor configuring(preparing for test) object state into shared methods (not static, but common to be used by multiple tests)
 
instead of
[Test]
public void NumOfUsers_2UsersAdded_Returns2()
{
MyOBject x = makeDefaultMyObject();
x.AddUser("a","b");
x.AddUser("c","d");
Assert.AreEqual(1,x.NumOfUsers);
} 
 
[Test]
public void HasUsers_2Users_ReturnsTrue()
{
MyOBject x = makeDefaultMyObject();
x.AddUser("a","b");
x.AddUser("c","d");
Assert.IsTrue(x.HasUsers);
} 
 
do:
 
[Test]
public void NumOfUsers_2UsersAdded_Returns2()
{
MyOBject x = makeDefaultMyObject();
init_AddDefault2UsersToMyObject(x);
Assert.AreEqual(2,x.NumOfUsers);
}
 
 
[Test]
public void HasUsers_2Users_ReturnsTrue()
{
MyOBject x = makeDefaultMyObject();
init_AddDefault2UsersToMyObject(x);
Assert.IsTrue(x.HasUsers);
}
 
private void init_AddDefault2UsersToMyObject(MyObject x)
{
x.AddUser("A","b");
x.AddUser("C","D");
}  
 
Refactor duplicated Act-Assert combinations into common methods (very methods)
 
Instead of
[Test]
public void ParseSum_3NumbersSingleDigit_ReturnsSum1()
{
Calc c = makeDefulatCalculator();
int sum = c.ParseSum("1,2,3");
Assert.AreEqual(6,sum);
} 
[Test]
public void ParseSum_3NumbersSingleDigit_ReturnsSum2()
{
Calc c = makeDefulatCalculator();
int sum = c.ParseSum("4,5,6");
Assert.AreEqual(15,sum);
} 
 
do:
 
[Test]
public void ParseSum_3NumbersSingleDigit_ReturnsSum1()
{
Calc c = makeDefulatCalculator();
int expectedSum=6;
verify_ParseSum(c,"1,2,3", expectedSum);
} 
[Test]
public void ParseSum_3NumbersSingleDigit_ReturnsSum2()
{
Calc c = makeDefulatCalculator();
int expectedSum=15;
verify_ParseSum(c,"4,5,6", expectedSum);
} 
 
private void verify_ParseSum(Calc c,string input, int expectedSum)
{
int result = c.ParseSum(input);
Assert.AreEqual(expectedSum,result);
}
 
 
No Setup or TearDown code present, tests are still readable, but are also still maintainable. Next time a constructor changes - you'll only have to change one method in this test fixture. Next time configuration of an object changes functionality (and thus needs to be modified in all tests requiring such an instance) you only need to change one method (or a small number of common methods) instead of tens or hundreds of tests.
 
Re-usability and Readability are possible. You just need to follow some guidelines on how to implement them.