I fail at TDD?

I actually think I’m pretty good at TDD. Every now and then I get reminded that I’m not as good as I think I am.

I’ve been working on a new project (an implementation of the Mustache template language in C# that I’m calling Nustache) and have been having a lot of fun with it. This is the project I’m going to use as an example of how I fail at TDD.

Since this project involves parsing a string, I decided I would probably need a class to scan the string for tokens so those tokens could be parsed and then evaluated.

While writing the test for my Scanner class, I wrote it so that it would assert on the sequence of tokens it returns. I decided the tokens would be instances of a class called Part. One specific subclass of Part would be LiteralText. It represents a span of characters from the source template that is not supposed to be evaluated and just rendered directly to the output. I figured this would be the easiest way to start testing my Scanner class.

The test probably looked like this (I’m writing this way after the fact):

[Test]
public void It_scans_literal_text()
{
    var scanner = new Scanner();

    var parts = scanner.Scan("foo");

    CollectionAssert.AreEqual(new Part[]
                              {
                                  new LiteralText("foo"),
                              },
                              parts);
}

At this point, the test didn’t compile because I hadn’t defined my Scanner, Part, and LiteralText classes yet.

Having written the test first, I learned a few things about the Scanner class it was trying to test:

  • It has a default constructor
  • It has a method named Scan
  • Its Scan method takes in a string
  • Its Scan method returns an IEnumerable<Part> (I know this because of the parameters for CollectionAssert.AreEqual)

I also learned something about the LiteralText class:

  • It derives from Part (because I’m adding it to a Part array)
  • It has a constructor that accepts a string
  • It must override the Equals method or this will never work

Since this test is describing the Scanner class, I decided to work on it first:

public class Scanner
{
    public IEnumerable<Part> Scan(string template)
    {
       return null;
    }
}

This wouldn’t compile until I defined Part:

public class Part
{
}

The test still needed LiteralText to be defined:

public class LiteralText : Part
{
    public LiteralText(string text)
    {
    }
}

At this point, I was able to compile and run my test. When I did, NUnit said this:

Test 'Nustache.Tests.Describe_Scanner_Scan.It_scans_literal_text' failed: 
  Expected: < <Nustache.Core.LiteralText> >
  But was:  null

I liked that failure message, but I wanted to go a bit further and see what the failure message would be when I return an empty array instead of null since it didn’t make sense to me for Scan to return null. Scan changed to this:

public IEnumerable<Part> Scan(string template)
{
    return new Part[] { };
}

The failure message became this:

Test 'Nustache.Tests.Describe_Scanner_Scan.It_scans_literal_text' failed: 
  Expected is <Nustache.Core.Part[1]>, actual is <Nustache.Core.Part[0]>
  Values differ at index [0]
  Missing:  < <Nustache.Core.LiteralText> >

OK, that wasn’t too bad. Next, I wanted to see if I could get it to pass by doing the simplest thing I could possibly do so I changed Scan to this:

public IEnumerable Scan(string template)
{
    return new Part[]
               {
                   new LiteralText("foo")
               };
}

After seeing that pass, I would have implemented it a little more realistically, but I was in for a surprise. Instead of passing (which is what I expected), I got this failure message:

Test 'Nustache.Tests.Describe_Scanner_Scan.It_scans_literal_text' failed: 
  Expected and actual are both <Nustache.Core.Part[1]>
  Values differ at index [0]
  Expected: <Nustache.Core.LiteralText>
  But was:  <Nustache.Core.LiteralText>

Uh… Oh, yeah! LiteralText needs an override of the Equals method or NUnit will never be able to tell if one instance is “equal to” another.

In order to implement that, I need to make sure the string that gets passed in to the LiteralText constructor gets saved in some sort of field or property. Then I could write my Equals override by hand or let ReSharper generate it for me.

I decided to let ReSharper do it (I’m lazy) and got three methods: bool Equals(LiteralText other), bool Equals(object obj), and int GetHashCode().

After getting that to work, I added a ToString method to LiteralText to make the failure message even clearer.

See the problem? I went off and started implementing code in LiteralText when I was in the middle of trying to get a test for Scanner to pass! Sure, it’s just the Equals and GetHashCode methods, but it’s still code!

I did all of this in response to test I was trying to get to pass so I was still doing TDD, right?

Right?

At the time I was doing this, I didn’t even notice this “problem”. It wasn’t until much later when I decided to run my tests under NCover to see how I was doing. I was practicing TDD, so my coverage should have been pretty good, if not perfect. Sadly, I found I had a bunch of Equals, GetHashCode, and ToString methods that weren’t fully covered and ruining my flawless code coverage report!

So what’s the big deal? Everybody agrees that 100% code coverage isn’t sufficient to ensure the correctness of your code. I absolutely agree with that. Many people also agree that getting 100% code coverage isn’t even worth it. That, I disagree with. As does Patrick Smacchia (author of NDepend) who described why 100% code coverage is a worthwhile goal here. It’s a great article and I highly recommend you all read it.

To rectify this predicament, I forced myself to write tests for my LiteralText class (writing tests after the fact is so boring!).

Since I originally defined it, I discovered that Part had grown a Render method and LiteralText was overriding it. The method was being covered by other tests, but there was nothing that was directly testing LiteralText. That might not be such a big deal, but one of the oft-touted benefits of unit tests is that they can also act as executable documentation. Since I had no unit tests for my LiteralText class, I had no executable documentation for it! How would I ever re-learn (months from now) how it’s supposed to behave without that?

OK, I’m being a bit silly, but I went for it anyways and I really liked the result. Here’s what I came up with:

[TestFixture]
public class Describe_LiteralText
{
    [Test]
    public void It_cant_be_constructed_with_null_text()
    {
        Assert.Throws<ArgumentNullException>(() => new LiteralText(null));
    }

    [Test]
    public void It_renders_its_text()
    {
        var a = new LiteralText("a");
        var writer = new StringWriter();
        var context = new RenderContext(null, null, writer, null);

        a.Render(context);

        Assert.AreEqual("a", writer.GetStringBuilder().ToString());
    }

    [Test]
    public void It_has_a_useful_Equals_method()
    {
        object a = new LiteralText("a");
        object a2 = new LiteralText("a");
        object b = new LiteralText("b");

        Assert.IsTrue(a.Equals(a));
        Assert.IsTrue(a.Equals(a2));
        Assert.IsTrue(a2.Equals(a));
        Assert.IsFalse(a.Equals(b));
        Assert.IsFalse(a.Equals(null));
        Assert.IsFalse(a.Equals("a"));
    }

    [Test]
    public void It_has_an_Equals_overload_for_other_LiteralText_objects()
    {
        var a = new LiteralText("a");
        var a2 = new LiteralText("a");
        var b = new LiteralText("b");

        Assert.IsTrue(a.Equals(a));
        Assert.IsTrue(a.Equals(a2));
        Assert.IsTrue(a2.Equals(a));
        Assert.IsFalse(a.Equals(b));
        Assert.IsFalse(b.Equals(a));
        Assert.IsFalse(a.Equals(null));
    }

    [Test]
    public void It_has_a_useful_GetHashCode_method()
    {
        var a = new LiteralText("a");

        Assert.AreNotEqual(0, a.GetHashCode());
    }

    [Test]
    public void It_has_a_useful_ToString_method()
    {
        var a = new LiteralText("a");

        Assert.AreEqual("LiteralText(\"a\")", a.ToString());
    }
}

As you can probably tell, I’m using a non-standard (for .NET developers) naming scheme for my tests. It’s inspired by RSpec and I really like it.

If you take away all the code, the ugly underscores, and the weird prefixes, you get the documentation:

  • LiteralText
    • can’t be constructed with null text
    • renders its text
    • has a useful Equals method
    • has an Equals overload for other LiteralText objects
    • has a useful GetHashCode method
    • has a useful ToString method

Could that get any clearer? (Seriously, leave a comment if you think it could.)

I formatted that list by hand, but generating it could easily be automated by processing NUnit’s XML output or using reflection on the test assembly. RSpec has a feature built in to it that can generate this kind of output. (Don’t worry, I don’t plan on switching to Ruby like every other .NET weblogger out there seems to be doing. I do think they have some great ideas, though, and have been really enjoying reading the beta version of the RSpec Book which is what made me try out this new naming scheme.)

Even though this particular class is trivial, doing this helped set up an example to follow for the other Part subclasses which aren’t as simple. Also, I feel much more confident about this class knowing that there is a suite of tests in a single, well-named fixture that provides 100% code coverage for it.

I’m not saying that every class should be covered by one and only one fixture. If the class demands it, I’ll happily break its tests up into multiple fixtures. I could have one fixture per method or one fixture per context. I’m flexible about that. I’d use the desire to that as a possible smell that the class might be trying to do too much, though.

You can also see that I’m pretty flexible about not limiting myself to just one assertion per test. I strongly believe that most tests should only have one assertion but, in this case, it would have been ridiculous to have written a test case for each of the different ways Equals could be invoked.

I was also a little lax on the Arrange/Act/Assert format. This is another practice that I try to consistently follow. Some tests just don’t need anything to be set up! And NUnit’s Assert.Throws syntax kind of forces you to act and assert at the same time. There’s not a lot I can do about that.

Here’s the one thing that I’m firm on: Ensuring that I have a set of tests that fully cover 100% of the unit that they directly test is a Good Thing.

And here’s the million dollar question: What can I do to prevent these kinds of mistakes in the future? Is it even possible?

To be honest, I’m OK making mistakes as long as I can catch and fix them quickly enough. Did I know about the Part and LiteralText classes before starting work on Scanner? I can’t actually remember. Let’s pretend I didn’t. As soon as I saw that the test for Scanner was referencing other classes, should I have stopped what I was doing, put an Ignore attribute on the test, and started working on tests for those other classes first? I’m not so sure about that. I feel like doing that might have negatively impacted the journey I had already embarked on.

So maybe “failing” at TDD in this way is expected? It’s rare when a class has no dependencies. If I’m using tools like NUnit, NCover, and NDepend, I should be able to catch my “mistakes” pretty quickly. This means that my unit tests have to be fast or I’ll rarely run them and, if that happens, my mistakes won’t be caught until much later. By that point, I’ll have too many mistakes to fix, I’ll be out of time, forced to move on to the next task, and, there I go, abandoning everything I believe in (about developing and testing)!

And my coworkers wonder why I hate our “unit tests” that read from and write to the database so much…

3 thoughts on “I fail at TDD?”

  1. I believe You should mock `Part` out. Overriding `equals` just for NUnit doesn’t sound right. That would satisfy Your desire for 100% code coverage too.

  2. Hi Arnis.

    Mocking `Part` isn’t currently possible because `Scanner` constructs the subclasses directly. I didn’t see a need to make that more flexible.

    Regarding overriding `Equals` just for NUnit: I hear you and just posted a follow-up with an alternative that doesn’t require that. =)

  3. Making something more flexible **only** for testability doesn’t sound right either. It’s like marking attributes with public setters just to inject fake objects. That eventually leads to poor encapsulation, unnecessary higher complexity.

    Your alternative approach looks really good though.

    ———
    My own problem with TDD is that it steals focus on problems I’m trying to solve. It’s like trying to choose between NHibernate and Entity framework instead of listening to what client needs.

    Best way to solve that would be to become truly fluent at TDD.

Leave a Reply

Your email address will not be published. Required fields are marked *

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>