Karl Seguin

Sponsors

The Lounge

Advertisement

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!


Posted Sat, Sep 12 2009 4:34 PM by karl

[Advertisement]

Comments

Glenn Block wrote re: Unit Testing - Do Repeat Yourself
on Sat, Sep 12 2009 5:20 PM

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 :-)

DotNetBurner - Unit Testing wrote Unit Testing - Do Repeat Yourself
on Sat, Sep 12 2009 5:56 PM

DotNetBurner - burning hot .net content

Piotr wrote re: Unit Testing - Do Repeat Yourself
on Sat, Sep 12 2009 7:31 PM

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.

Gustav wrote re: Unit Testing - Do Repeat Yourself
on Sun, Sep 13 2009 3:31 AM

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);  

     }

}

}

Sanjeev Agarwal wrote Daily tech links for .net and related technologies - September 13-15, 2009
on Mon, Sep 14 2009 2:03 AM

HTML clipboard Daily tech links for .net and related technologies - September 13-15, 2009 Web Development

Ivars wrote re: Unit Testing - Do Repeat Yourself
on Mon, Sep 14 2009 7:27 AM

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).

Jeremy Hadden wrote re: Unit Testing - Do Repeat Yourself
on Mon, Sep 14 2009 11:11 AM

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.

Bill Sorensen wrote re: Unit Testing - Do Repeat Yourself
on Tue, Sep 15 2009 1:16 PM

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.

karl wrote re: Unit Testing - Do Repeat Yourself
on Tue, Sep 15 2009 1:31 PM

@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.

scott wrote re: Unit Testing - Do Repeat Yourself
on Thu, Sep 24 2009 1:23 PM

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.

James Peckham wrote re: Unit Testing - Do Repeat Yourself
on Mon, Sep 28 2009 11:25 PM

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.