Unit Testing – Do Repeat Yourself

Writing effective unit tests is as much about the test itself as it is about the code under test. All the experience in the world isn’t going to help you write clean and meaningful tests against highly coupled code. Conversely, complex and messy unit tests don’t add any value even if the code under test is perfectly designed. Ultimately, the secret is practice – the more tests you write, the better you are at it, and the cleaner your code becomes as a consequence.

One thing that I’ve noticed though is that one of the fundamental practices for maintainable code can actually be counter-productive when applied to unit tests: DRY (Don’t Repeat Yourself). Outside of unit tests, DRY simply makes sense – it reduces the amount of code you have, generally increased readability, and makes maintenance considerably easier. As a developer you have a ton of choices to ensure code isn’t repeated – from your typical OO paradigms (i.e. inheritance) to a slew of refactoring options, and even some language-specific features like extension methods .

However, when applied irresponsibly to unit tests, DRY can very likely make your tests brittle – brittle tests are what lead to developers complaining of “spending more time maintaining and fixing our tests than anything else.” Let’s look at a modified example from the CodeBetter.Canvas project. Our UserBinder class is responsible for binding both the new users and existing users, it looks something like:

public override User BindNew()
{
    var user = BindBase(new User());
    user.Credentials.Password = _encryptor.Encrypt(Get("Credentials.Password"));
    return user;
}

public override User BindExisting(User user)
{
    BindBase(user)
    var password = Get("Credentials.Password");
    if (!string.IsNullOrEmpty(password))
    {
        user.Credentials.Password = _encryptor.Encrypt(password);
    }
}

private User BindBase(User user)
{
    user.Name = Get("Name");
    user.Credentials.Email = Get("Credentials.Email");
    return user;
}

In order to avoid some repetition, the BindBase method is used to bind all the properties shared when binding new users and existing ones – a simple but effective Extract Method was used. So, let’s write the unit tests for the BindNew Method:

public class BindNewUserTests : BaseFixture
{
    [Fact]
    public void BindsBaseProperties()
    {
        var encryptor = Dynamic<IEncryptor>();
        var parameters = new NameValueCollection{ {"Name", "Giru"}, {"Credentials.Email", "giru@playa.cool"},  {"Credentials.Password", ""}, };
        var binder = new UserBinder(encryptor);
        SetField(binder, "_parameters", parameters); //ack that's ugly!!

        encryptor.Stub(e => e.Encrypt(null)).IgnoreParameters().Return(null);
        encryptor.Replay();        

        var user = binder.BindNew();
        Assert.Equal("Giru", user.Name);
        Assert.Equal("giru@playa.cool", user.Credentials.Email);
    }
    [Fact]
    public void BindsEncryptedPassword()
    {
        var encryptor = Dynamic<IEncryptor>();
        var parameters = new NameValueCollection{ {"Name", "Giru"}, {"Credentials.Email", "giru@playa.cool"},  {"Credentials.Password", "plain text"}, };
        var binder = new UserBinder(encryptor);
        SetField(binder, "_parameters", parameters); //ack that's ugly!!

        encryptor.Expect(e => e.Encrypt("plain text")).Return("encrypted");
        encryptor.Replay();        

        var user = binder.BindNew();
        Assert.Equal("encrypted", user.Credentials.Password);
        encryptor.VerifyAllExpectations();
    }
}

So there’s a bit of ugliness, but overall those are two decent tests. Now what about tests for the BindExisting method? We know we need to handle the special password handling (if it’s blank, that means it wasn’t changed, so we don’t overwrite it):

public class BindExistingUserTests : BaseFixture
{
    [Fact]
    public void LeavesPasswordAsIsIfNotSet()
    {
        var original = new User{Password = "the originator" };
        var parameters = new NameValueCollection{ {"Name", "Giru"}, {"Credentials.Email", "giru@playa.cool"},  {"Credentials.Password", string.Empty}, };
        var binder = new UserBinder(null);
        SetField(binder, "_parameters", parameters); //ack that's ugly!!

         binder.BindExisting(original);
         Assert.Equal("the originator", original.Credentials.Password);
    }
    [Fact]
    public void EncryptsTheNewPassword()
    {
        var encryptor = Dynamic<IEncryptor>();
        var original = new User{Password = "the originator" };
        var parameters = new NameValueCollection{ {"Name", "Giru"}, {"Credentials.Email", "giru@playa.cool"},  {"Credentials.Password", "new password"}, };
        var binder = new UserBinder(encryptor);
        SetField(binder, "_parameters", parameters); //ack that's ugly!!

        encryptor.Expect(e => e.Encrypt("new passwordt")).Return("newly encrypted");
        encryptor.Replay();        

         binder.BindExisting(original);
        Assert.Equal("newly encrypted", original.Credentials.Password);
        encryptor.VerifyAllExpectations();
    }
}

So far so good, but what about a test to verify that Name and Email are also properly bound? Your first instinct may be to skip it, since you know, from the tests against BindNew that everything works as expected. You may also consider mocking the BindBase call. Neither of these are good choices. Fundamentally, your test should be written in a way that’s independent of the implementation (in large part because the implementation could change, resulting in a broken test). This means that you should effectively repeat your test:

    [Fact]
    public void BindsBaseProperties()
    {
        var original = new User();
        var parameters = new NameValueCollection{ {"Name", "Giru"}, {"Credentials.Email", "giru@playa.cool"},  {"Credentials.Password", ""}, };
        var binder = new UserBinder(null);
        SetField(binder, "_parameters", parameters); //ack that's ugly!!

        binder.BindExisting(original);
        Assert.Equal("Giru", original.Name);
        Assert.Equal("giru@playa.cool", original.Credentials.Email);
    }

The benefit is that if the implementation of BindExisting changes, our test will catch it. On the flip side, if BindBase changes, two tests will break. You can mitigate against this by extracting some of the logic, for example:

    [Fact]
    public void BindsBaseProperties()
    {
        var original = new User();
        var parameters = new NameValueCollection{ {"Name", "Giru"}, {"Credentials.Email", "giru@playa.cool"},  {"Credentials.Password", ""}, };
        var binder = new UserBinder(null);
        SetField(binder, "_parameters", parameters); //ack that's ugly!!

        binder.BindExisting(original);
        AssertBase(original, "Giru", "giru@playa.cool");
    }

    private static void AssertBase(User user, string name, string email)
    {
        Assert.Equal(name, user.Name);
        Assert.Equal(email, user.Credentials.Email);
    }

But that won’t always be practical, and greatly decreases your tests readability.

There’s two points to all of this. First and most importantly, your unit tests shouldn’t know or be written in a way that relies on a specific internal implementation – else you will spend more time fixing your tests than anything else. This is the same reason why you shouldn’t test private methods – because tests against public methods should adequately cover them. Secondly the goals of unit testing is different enough from system code that common principles should often be reconsidered. If your test is brittle or hard to read, find an alternative!

This entry was posted in Uncategorized. Bookmark the permalink. Follow any comments here with the RSS feed for this post.

9 Responses to Unit Testing – Do Repeat Yourself

  1. Ho-hum…

    set some state
    run some logic
    assert state

    remove duplicated state setup -> (FixtureSetup or SetUp in NUnit)
    remove duplicated asserts -> (extract to method)

    To me most of your problem in that code was that it was very procedural. If you remove redundant code in an extensible/object oriented manner it wouldn’t be so brittle.

  2. scott says:

    I like to think of tests as being “damp”. Not totally DRY, but DRY enough.

    Context/Spec helps a lot with this. Organizing around shared context keeps duplication within the context at a minimum, but you may have some duplication between contexts, and…I’m ok with that.

  3. karl says:

    @Bill
    I’m hardly always right. I blog as much to learn and engage in a conversation as I do to help and teach.

    I think my point wasn’t properly illustrated by my example. Ultimately, unit tests should be decoupled from internal implementations, which will often lead to tests that test the same thing. This is a good thing.

    However, you can use techniques within tests themselves to reduce code duplication. That’s fine.

  4. I have to think twice about disagreeing with both Glenn Block and Karl, but I think there’s room for compromise here. Duplication that makes tests harder to maintain can be refactored out. Using helper methods, custom assertions, and/or base classes in tests can be beneficial. In all cases readability is an important consideration.

  5. Jeremy Hadden says:

    I think that – as always – these things should be decided on a case-by-case basis.

    In the example above your application of DRY is basedon replacing 4 lines of repeated code with 2 lines. You could certainly argue that the readability trade-off doesn’t stack up in this case. However, what if it’s 10 lines of repeated code – or more, if they are repeated over many more tests. Then the trade-off may swing the other way.

    And what if further base fields are added? Without the refactor, adding a new base field means adding a test for that field in each unit test – thereby increasing the risk that you miss one out.

    And finally, I think that brittleness is less about how many tests break as a result of a single change and more about how hard it is to fix them. A good example is setting up state in order to run a test. Good DRY refactoring for test set-up means that if an operation suddenly requires an extra piece of data to complete you don’t find yourself manually editing 20 broken tests to fix.

  6. Ivars says:

    Well, as with any software development technique – it depends. For example, recently I wrote bunch of tests (well, integration tests, not unit tests), where was common steps in so-called “arrange” part of tests. I did extract them in methods, because in my opinion it’s better to have test like

    public void SomeTest()
    {
    string userName = “user”;
    string password = “pass”;
    Login();
    // Do test specific part
    // Do assert
    }

    than duplicating login logic in each test. Also such approach allows to avoid multiple asserts per test which is kind of “Bad Thing(tm)” (I mean multiple asserts, not avoiding them).

  7. Gustav says:

    You could extract the user binding logic into its own class. That way you would only need to test it once:

    public override User BindNew()
    {
    var user = new User();
    return userBinder.Bind(user);
    }

    public override User BindExisting(User user)
    {
    return userBinder.Bind(user)
    }

    private UserBinderHelper userBinder = …

    //——

    public class UserBinderHelper
    {
    public void Bind(User user)
    {
    user.Name = Get(“Name”);
    user.Credentials.Email = Get(“Credentials.Email”);
    var password = Get(“Credentials.Password”)
    if (!string.IsNullOrEmpty(password))
    {
    user.Credentials.Password = _encryptor.Encrypt(password);
    }
    }
    }

  8. Piotr says:

    I agree: I tend to apply DRY far less aggressively in test code than in production code. Also, a lot of duplication in test code may be a signal that you test too much functionality per test: instead of extracting methods, you may be better off splitting your tests into smaller ones.

  9. Glenn Block says:

    My experience exacty. Applying DRY to unit-testing code makes the tests much harder to follow. Yes there’s duplication but in this case it’s one good use of cut & paste :-)