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