Unit testing best practices – Simplicity avoids confusion

This post is a continuation from our Unit testing best practices series. You can find the previous post here

Sometimes, we get that great idea to code the unit test code of the century. With a few dozen lines of code, we can cover most of our test cases, saving us from having many separate tests. We code, run it, and it works perfectly fine, what a great day of work. But then, a few weeks later, we have to do some maintenance on that piece of code because our requirements have changed. And then, we regret. So hard. If you’ve never felt this before, read on, and see how following this next unit testing best practice can save the day of your future self.

So, let’s suppose we are creating a string converter of some sort, which should repeat each one of the characters in a string N times. The code looks like this:

static class NexGenStringConverter
{
    public static string Convert(string value, int numberOfRepetitions)
    {
        var returnString = string.Empty;

        foreach(var character in value)
            for (int i = 0; i < numberOfRepetitions; i++)
                returnString += character;

        return returnString;
    }
}

Pretty simple right? Get every character, repeat accordingly to the numberOfRepetitions parameter and return it. Now, we need to create some unit tests for it. Since our code can support any string, maybe we should test a bunch of them? Sounds great. Even better, we could randomly create them, so we could eventually catch some edge cases. This looks promising and simple enough, let’s code it!

[Test]
public void ShouldHopefullyConvertToNextGen()
{
    //Arrange
    var stringsToTest = new List<string>();
    var repetitionNumber = 5;
    var wordsToTest = 100;
    var random = new Random();
    for (int i = 0; i < wordsToTest; i++)
    {
        var newString = string.Empty;

        var randomNumbers = new HashSet<int>();
        while(randomNumbers.Count < 5)
        {
            var randomNumber = random.Next(1, 100);
            randomNumbers.Add(randomNumber);
        }

        foreach (var number in randomNumbers)
            newString += (char)number;

        stringsToTest.Add(newString);
    }

    //Act
    var results = stringsToTest.Select(s => NexGenStringConverter.Convert(s, repetitionNumber)).ToArray();

    //Assert
    for (int i = 0; i < wordsToTest; i++)
    {
        var initialValue = stringsToTest[i];
        var result = results[i];

        foreach (var character in initialValue)
            Assert.That(result.Where(r => r == character).Count, Is.EqualTo(repetitionNumber));
    }
}

Brilliant! With a code like this, we don’t even need to obfuscate it, since probably nobody will understand what the hell is going on. Although this test does verify a bunch of strings, it’s far from being considered good. Let’s run through some of the issues:

• The name doesn’t clearly state its purpose (you can read more about naming best practices here)
• We need to try really hard to discover the input and the expected output
• Even if we get a failed result, we can’t consistently reproduce the failing case. That means, we know we have a bug, but we don’t know how to fix it.
• Very hard to maintain. Imagine, for example, if our Convert method changes so it inserts a ‘;’ between each character?

So now that we have established how bad our unit test is, let’s get fixing it. I’m not going to write step by step on how to fix it because that piece of code is hopeless. Instead, let’s write a new one from scratch.

[TestCase("Simplicity", 3, "SSSiiimmmpppllliiiccciiitttyyy")]
[TestCase("What a great input", 2, "WWhhaatt aa ggrreeaatt iinnppuutt")]
[TestCase("", 10, "")]
public void ShouldRepeatEachCharacter(string input, int numberOfRepetitions, string expectedOutput)
{
    //Act
    var result = NexGenStringConverter.Convert(input, numberOfRepetitions);

    //Assert
    Assert.That(result, Is.EqualTo(expectedOutput));
}

Much better right? Before it seemed like rocket science, now I dare to say that its complexity is so low that any junior developer will be able to understand it. If we want to test new cases, we simply have to add a new TestCase attribute and that’s it. We could even add one to check what would happen if we passed null as the input (spoiler: code goes boom ).

One might argue that the previous test would cover more cases. It might seem to be true, since we’re generating 100 random strings, but it’s 100 strings in the same format, it’s always the same scenario. Randomness in the tests can be good, yes, but you need to implement it differently. In order for it to be effective, you need to generate random values before compile time, so your tests are always consistent. In this case, for example, we could generate 10 random strings and create 10 test cases.

Focus on simplicity when implementing your unit tests, a good test should be easy to read and maintain. Covering different scenarios is surely important, but there are better ways to do it, such as using the TestCase attribute. And if you want to have a lot of random values, avoid generating them in runtime, as it can cause your tests to be unpredictable and flaky. And fixing flaky tests can be a true hell.

Cross-posted from my personal blog.