Eliminating obscure tests

One of the purported benefits of unit tests and TDD in general is unit tests doubling as living documentation.  Unit tests are documentation in the form of executable client code demonstrating small units of behavior.  The idea is that if you wanted to understand how a class worked, you simply needed to look at the unit tests.

But documentation, whether in the form of user manuals or unit tests, is only as valuable as the clarity of its content.  Obscure, vague tests can confuse developers trying understand behavior.  For example, can anyone follow the ambiguity of the following test?

[Test]
public void Should_compare_based_on_last_name_then_company()
{
    Contact contact1 = new Contact() 
        { FirstName = "a", LastName = "a", CompanyName = "a" };
    Contact contact2 = new Contact() 
        { FirstName = "a", LastName = "a", CompanyName = "a" };

    Assert.AreEqual(0, contact1.CompareTo(contact2));
    contact2.LastName = "b";
    Assert.AreEqual(-1, contact1.CompareTo(contact2));
    contact1.LastName = "c";
    Assert.AreEqual(1, contact1.CompareTo(contact2));
    contact2.LastName = "c";
    contact2.CompanyName = "b";
    Assert.AreEqual(-1, contact1.CompareTo(contact2));
    contact1.CompanyName = "c";
    Assert.AreEqual(1, contact1.CompareTo(contact2));
    contact2.CompanyName = "c";
    contact2.FirstName = "b";
    Assert.AreEqual(-1, contact1.CompareTo(contact2));
    contact1.FirstName = "c";
    Assert.AreEqual(1, contact1.CompareTo(contact2));
}

The name of the test is reasonable enough, it’s describing comparison behavior for a Contact object:

public class Contact : IComparable<Contact>
{
    public string FirstName { get; set; }
    public string LastName { get; set; }
    public string CompanyName { get; set; }

    public int CompareTo(Contact other)
    {
        int compareResult = LastName.CompareTo(other.LastName);
        if (compareResult == 0)
            compareResult = CompanyName.CompareTo(other.CompanyName);
        if (compareResult == 0)
            compareResult = FirstName.CompareTo(other.FirstName);
        return compareResult;
    }
}

Unfortunately, the name of the test is about the only good thing about it.  There are about a half-dozen asserts, sprinkled between changes to each objects state.  To understand what each assert is actually testing, you have to trace back the history of the changes to see what the state of each object truly is.

This test is suffering from the Obscure Test smell, the cause being Eager Test.  It’s testing far too much at once, and in doing so, misses quite a few scenarios.  If I change around the order of the comparisons in the Contact class, the test still passes!  That’s a good sign of a bad test, if you can change the underlying behavior and the tests supposedly verifying behavior still passes.

Test one thing

So let’s start over.  We have a request from the business that they want their contacts sorted by CompanyName first, then LastName.  Usually I keep my sorting concerns outside of the classes being sorted, but for posterity sake, let’s keep the behavior in the Contact class.

The main problem with the original test is that it tried to test a bunch of different scenarios at once.  Unit test convention tells us to test one thing per test.  It can be hard to define “one thing”, but I like to think of it as “one context, one behavior“.  A single piece of code might behave differently in different scenarios, so tackling it from the context/behavior side tends to get you more branch coverage.

Now it’s time to think of one context, and describe the desired behavior given that context.

When comparing Contacts with different last name and different company names, it should sort based on the company names

We could write several more scenarios, but one will suffice for now.  Using a BDD-style specification, this would result in the following specification:

[TestFixture]
public class When_comparing_Contacts_with_different_last_names_and_different_company_names
{
    private Contact contact1;
    private Contact contact2;

    [SetUp]
    public void Before_each_spec()
    {
        contact1 = new Contact() 
            { FirstName = "a", LastName = "b", CompanyName = "a" };
        contact2 = new Contact() 
            { FirstName = "a", LastName = "a", CompanyName = "b" };
    }

    [Test]
    public void Should_sort_based_on_company_names()
    {
        contact1.CompareTo(contact2).ShouldEqual(-1);
    }
}

Now my test specification is not satisfied (i.e. failing), so I need to change the behavior of the Contact class to satisfy the new specification.  A couple things to note:

• All setup information is put in the SetUp method
• The specification part only contains assertions

Now that I’ve explicitly partitioned the context (SetUp) from the specification (Test), it’s far easier to make the connection between desired behavior and the observed behavior.

Punished by laziness

When I get lazy and try to test too much in a single unit test, I always pay more in the end.  Obscure tests are a subtle form of technical debt, as I’m fooling myself into thinking I have a good design because I have unit tests with coverage.  But if I can’t look at a test and understand what it’s testing, it probably put me in a worst spot than having no tests.  Instead of having to decipher just the underlying class, I have to decipher the test as well.

Sticking to one context/specification of behavior helps me ensure that I can truly understand the underlying behavior of the system simply by reading the tests.