The team is delighted to introduce Bradley Braithwaite as a guest blogger on the Telerik Just blog. Bradley is a well-known expert in the TDD space and author of the blog Contented Coder. In this post, Bradley will share common TDD pitfalls that he has encountered over the past decade, working with clients in a variety of development setups.

I've written a lot of bad unit tests. A lot. But I persevered and now I love to write unit tests. I code faster with unit tests and when I think I'm done I have a certain level of proof that my code works as expected. I don't like my code to have bugs and I've been rescued from silly bugs on many occasions by my unit tests. If I had my way, everybody would write unit tests!

As a freelancer I regularly get to see how different companies work internally and I'm often surprised by how many companies have still yet to adopt TDD (test driven development). When I ask "why?", the response is usually attributed to one or more of the following common mistakes I see when using TDD. Such mistakes are very easy to make and I've been guilty of all of them. These common mistakes attributed to many of the companies I have worked with to abandon TDD with the opinion that it "adds unnecessary maintenance to the code base" or that "the time spend writing tests is not worthwhile".

It's reasonable to postulate that:

It's better to have no unit tests than to have unit tests done badly.

But I also have the experience to confidently say that:

Unit Tests make me more productive and my code more reliable.

With this in mind, let's look at some of the most common TDD mistakes I've seen, made and learnt from.

1. Not using a Mocking Framework

One of the first things we are taught about TDD is to test things in isolation. This means we have to mock, stub or fake dependencies to isolate methods to be tested.

Consider we want to test the GetByID method from the following class:

01.public class ProductService : IProductService
02.{
03.    private readonly IProductRepository _productRepository;
04.   
05.    public ProductService(IProductRepository productRepository)
06.    {
07.        this._productRepository = productRepository;
08.    }
09.   
10.    public Product GetByID(string id)
11.    {
12.        Product product =  _productRepository.GetByID(id);
13.   
14.        if (product == null)
15.        {
16.            throw new ProductNotFoundException();
17.        }
18.   
19.        return product;
20.    }
21.}

To make this work we would need to create a stub of IProductRepository so that we can test the ProductService.GetByID method in isolation. The test along with the stub IProductRepository class would look as follows:

01.[TestMethod]
02.public void GetProductWithValidIDReturnsProduct()
03.{
04.    // Arrange
05.    IProductRepository productRepository = new StubProductRepository();
06.    ProductService productService = new ProductService(productRepository);
07.   
08.    // Act
09.    Product product = productService.GetByID("spr-product");
10.   
11.    // Assert
12.    Assert.IsNotNull(product);
13.}
14.   
15.public class StubProductRepository : IProductRepository
16.{
17.    public Product GetByID(string id)
18.    {
19.        return new Product()
20.        {
21.            ID = "spr-product",
22.            Name = "Nice Product"
23.        };
24.    }
25.   
26.    public IEnumerable<Product> GetProducts()
27.    {
28.        throw new NotImplementedException();
29.    }
30.}

Let's now consider that we wanted to test the negative outcome of this method with an invalid product ID.

01.[TestMethod]
02.public void GetProductWithInValidIDThrowsException()
03.{
04.    // Arrange
05.    IProductRepository productRepository = new StubNullProductRepository();
06.    ProductService productService = new ProductService(productRepository);
07.   
08.    // Act & Assert
09.    Assert.Throws<ProductNotFoundException>(() => productService.GetByID("invalid-id"));
10.}
11.   
12.public class StubNullProductRepository : IProductRepository
13.{
14.    public Product GetByID(string id)
15.    {
16.        return null;
17.    }
18.   
19.    public IEnumerable<Product> GetProducts()
20.    {
21.        throw new NotImplementedException();
22.    }
23.}

In this example we create a separate repository for each test. Alternatively we could have added conditional logic to a single stub repository class, for example:

01.public class StubProductRepository : IProductRepository
02.{
03.    public Product GetByID(string id)
04.    {
05.        if (id == "spr-product")
06.        {
07.            return new Product()
08.            {
09.                ID = "spr-product",
10.                Name = "Nice Product"
11.            };
12.        }
13.   
14.        return null;
15.    }
16.   
17.    public IEnumerable<Product> GetProducts()
18.    {
19.        throw new NotImplementedException();
20.    }
21.}

In the first approach we had to write two separate IProductRepository stub classes and in the second approach we introduced complexity to our stub. If we make a mistake at this level it's going to break our tests and introduce additional debugging effort to establish if the bug is with our code under test or the test setup code.

You may also be questioning the apparent random inclusion of the method GetProducts() in the stub class? Because that method exists in the IProductRepository interface we have to include this method to appease the compiler, even though we are not interested in that method in this context!

When using this approach we have write lots of these stub classes which could arguably leave us with a maintenance headache. We can save ourselves a lot of work by using a mocking framework, such as JustMock.

Let's revisit our previous unit tests only this time we will use a mocking framework:

01.[TestMethod]
02.public void GetProductWithValidIDReturnsProduct()
03.{
04.    // Arrange
05.    IProductRepository productRepository = Mock.Create<IProductRepository>();
06.    Mock.Arrange(() => productRepository.GetByID("spr-product")).Returns(new Product());
07.    ProductService productService = new ProductService(productRepository);
08.   
09.    // Act
10.    Product product = productService.GetByID("spr-product");
11.   
12.    // Assert
13.    Assert.IsNotNull(product);
14.}
15.   
16.[TestMethod]
17.public void GetProductWithInValidIDThrowsException()
18.{
19.    // Arrange
20.    IProductRepository productRepository = Mock.Create<IProductRepository>();
21.    ProductService productService = new ProductService(productRepository);
22.   
23.    // Act & Assert
24.    Assert.Throws<ProductNotFoundException>(() => productService.GetByID("invalid-id"));
25.}

Notice how we are able to write considerably less code? 49% less code in this example, to be exact since our tests using mocks are 28 lines long vs. 57 for the equivalent code without the mocking framework. We are also able to see everything required for the tests within the test method body which is great for readability!

2. Too Much Test Setup

Mocking frameworks make it easier for us to mock dependencies for a class under test, but sometimes it can be too easy. To illustrate this point, scan the following two unit tests and consider which of the two is easier to understand. These two tests assert the same functionality:

Test #1

01.TestMethod]
02.public void InitializeWithValidProductIDReturnsView()
03.{
04.    // Arrange
05.    IProductView productView = Mock.Create<IProductView>();
06.    Mock.Arrange(() => productView.ProductID).Returns("spr-product");
07.   
08.    IProductService productService = Mock.Create<IProductService>();
09.    Mock.Arrange(() => productService.GetByID("spr-product")).Returns(new Product()).OccursOnce();
10.   
11.    INavigationService navigationService = Mock.Create<INavigationService>();
12.    Mock.Arrange(() => navigationService.GoTo("/not-found"));
13.   
14.    IBasketService basketService = Mock.Create<IBasketService>();
15.    Mock.Arrange(() => basketService.ProductExists("spr-product")).Returns(true);
16.       
17.    var productPresenter = new ProductPresenter(
18.                                            productView,
19.                                            navigationService,
20.                                            productService, 
21.                                            basketService);
22.   
23.    // Act
24.    productPresenter.Initialize();
25.   
26.    // Assert
27.    Assert.IsNotNull(productView.Product);
28.    Assert.IsTrue(productView.IsInBasket);
29.}

Test #2

01.[TestMethod]
02.public void InitializeWithValidProductIDReturnsView()
03.{
04.    // Arrange   
05.    var view = Mock.Create<IProductView>();
06.    Mock.Arrange(() => view.ProductID).Returns("spr-product");
07.   
08.    var mock = new MockProductPresenter(view);
09.   
10.    // Act
11.    mock.Presenter.Initialize();
12.   
13.    // Assert
14.    Assert.IsNotNull(mock.Presenter.View.Product);
15.    Assert.IsTrue(mock.Presenter.View.IsInBasket);
16.}

I would hope that Test #2 was the easiest to understand? What makes Test #1 less readable is the amount of setup code. In Test #2 I have abstracted away the busy looking constructor logic for the ProductPresenter class to make the test more readable.

To paint a clearer picture, let's take a look at the method under test:

01.public void Initialize()
02.{
03.    string productID = View.ProductID;
04.    Product product = _productService.GetByID(productID);
05.   
06.    if (product != null)
07.    {
08.        View.Product = product;
09.        View.IsInBasket = _basketService.ProductExists(productID);
10.    }
11.    else
12.    {
13.       NavigationService.GoTo("/not-found");
14.    }
15.}

This method has dependencies on the View, ProductService, BasketService and NavigationService classes that need to be mocked or stubbed. When faced with classes with lots of dependencies such as this the side effect is a lot of setup code as illustrated in this example.

NB This is a modest example. In wild I have seen classes with as many as ten dependencies being mocked.

Here is the MockProductPresenter class that abstracts away the setup of the ProductPresenter under test:

01.public class MockProductPresenter
02.{
03.    public IBasketService BasketService { get; set; }
04.    public IProductService ProductService { get; set; }
05.    public ProductPresenter Presenter { get; private set; }
06.   
07.    public MockProductPresenter(IProductView view)
08.    {
09.        var productService = Mock.Create<IProductService>();
10.        var navigationService = Mock.Create<INavigationService>();
11.        var basketService = Mock.Create<IBasketService>();
12.   
13.        // Setup for private methods
14.        Mock.Arrange(() => productService.GetByID("spr-product")).Returns(new Product());
15.        Mock.Arrange(() => basketService.ProductExists("spr-product")).Returns(true);
16.        Mock.Arrange(() => navigationService.GoTo("/not-found")).OccursOnce();
17.   
18.        Presenter = new ProductPresenter(
19.                                   view,
20.                                        navigationService,
21.                                        productService,
22.                                        basketService);
23.    }
24.}

Since the value of the View.ProductID property determines the logical flow of the method, we pass a mock View instance via the constructor of the MockProductPresenter class. The behaviour of the remaining mocked dependencies can be setup to act accordingly based on the mocked View dependency state.

This approach allows us to mock the differentiating factor (IProductView) in the test body. This can be seen in the second unit test for the Initialize method (where the product is null):

01.[TestMethod]
02.public void InitializeWithInvalidProductIDRedirectsToNotFound()
03.{
04.    // Arrange
05.    var view = Mock.Create<IProductView>();
06.    Mock.Arrange(() => view.ProductID).Returns("invalid-product");
07.   
08.    var mock = new MockProductPresenter(view);
09.   
10.    // Act
11.    mock.Presenter.Initialize();
12.   
13.    // Assert
14.    Mock.Assert(mock.Presenter.NavigationService);
15.}

This does hide away some implementation detail about the ProductPresenter, however readable test methods are paramount.

3. Asserting Too Many Elements

Review the following unit test and try to describe what it does without using the word "and":

01.[TestMethod]
02.public void ProductPriceTests()
03.{
04.    // Arrange
05.    var product = new Product()
06.    {
07.        BasePrice = 10m
08.    };
09.   
10.    // Act
11.    decimal basePrice = product.CalculatePrice(CalculationRules.None);
12.    decimal discountPrice = product.CalculatePrice(CalculationRules.Discounted);
13.    decimal standardPrice = product.CalculatePrice(CalculationRules.Standard);
14.   
15.    // Assert
16.    Assert.AreEqual(10m, basePrice);
17.    Assert.AreEqual(11m, discountPrice);
18.    Assert.AreEqual(12m, standardPrice);
19.}

I would describe this method:

"Tests that the base, discount AND standard price calculations return the expected values."

This is a simple rule of thumb when assessing that a test is covering too much. This test also has three reasons to break. If the test fails we need to debug more to find out which test(s) are wrong.

Ideally each case would have it's own test, for example:

01.[TestMethod]
02.public void CalculateDiscountedPriceReturnsAmountOf11()
03.{
04.    // Arrange
05.    var product = new Product()
06.    {
07.        BasePrice = 10m
08.    };
09.   
10.    // Act
11.    decimal discountPrice = product.CalculatePrice(CalculationRules.Discounted);
12.   
13.    // Assert
14.    Assert.AreEqual(11m, discountPrice);
15.}
16.   
17.[TestMethod]
18.public void CalculateStandardPriceReturnsAmountOf12()
19.{
20.    // Arrange
21.    var product = new Product()
22.    {
23.        BasePrice = 10m
24.    };
25.   
26.    // Act
27.    decimal standardPrice = product.CalculatePrice(CalculationRules.Standard);
28.   
29.    // Assert
30.    Assert.AreEqual(12m, standardPrice);
31.}
32.   
33.[TestMethod]
34.public void NoDiscountRuleReturnsBasePrice()
35.{
36.    // Arrange
37.    var product = new Product()
38.    {
39.        BasePrice = 10m
40.    };
41.   
42.    // Act
43.    decimal basePrice = product.CalculatePrice(CalculationRules.None);
44.   
45.    // Assert
46.    Assert.AreEqual(10m, basePrice);
47.}

Note also the highly descriptive test names. If there are 500 tests in a project and one of these fails you would get an understanding of what the test "should" be doing based on the name alone.

We have more methods but the trade-off is clarity. I read the following rule of thumb in Code Complete 2:

Write a separate test for every IF, And, Or, Case, For and While condition within a method.

TDD purists would argue that you should only ever have one assertion per test. I think that we can flex the rules sometimes when writing tests such as the following method that maps properties of an object:

01.public Product Map(ProductDto productDto)
02.{
03.    var product = new Product()
04.    
05.        ID = productDto.ID,
06.        Name = productDto.ProductName,
07.        BasePrice = productDto.Price
08.    };
09.   
10.    return product;
11.}

I don't think a separate test to assert each property is necessary. Here's how I would write a test for such a method:

01.[TestMethod]
02.public void ProductMapperMapsToExpectedProperties()
03.{
04.    // Arrange
05.    var mapper = new ProductMapper();
06.    var productDto = new ProductDto()
07.    {
08.        ID = "sp-001",
09.        Price = 10m,
10.        ProductName = "Super Product"
11.    };
12.   
13.    // Act
14.    Product product = mapper.Map(productDto);
15.   
16.    // Assert
17.    Assert.AreEqual(10m, product.BasePrice);
18.    Assert.AreEqual("sp-001", product.ID);
19.    Assert.AreEqual("Super Product", product.Name);
20.}

4. Writing Tests Retrospectively

I maintain that there is more to TDD than just tests. The feedback loop that is gained when applying TDD correctly can increase productivity dramatically. I often see developers completing a new feature and writing unit tests retrospectively as an administrative task before their code is committed for review. Catching code regression is only one aspect of writing tests.

Missing out on the Red, Green, Refactor approach where the tests are written first for new code can turn test writing into a laborious task.

To train your unit testing reflex take a look at TDD Katas such as The String Calculator Code Kata.

5. Testing Too Much Code

Review the following method:

1.public Product GetByID(string id)
2.{
3.    return _productRepository.GetByID(id);
4.}

Does this method REALLY need a test? No, I didn't think so, either!

TDD purists may insist that ALL code have test coverage and there are automated tools out there that will scan an assembly and report any areas of code that are not covered by tests, however we need to be very careful not to fall into the trap of busy work.

Many of the Anti-TDD people I speak to cite this as a major reason for not writing any tests. My response to that is "only test what you need to". I'm of the opinion that property setters and getters and constructors do not require exclusive tests. Let's remind ourselves of the rule of thumb we previously mentioned:

Write a separate test for every IF, And, Or, Case, For and While condition within a method.

If a method has none of these conditions, does it really warrant a test?

Happy Testing!

Get the code

The code used in the examples can be found here.

About the author

Bradley Braithwaite

Bradley Braithwaite

Bradley Braithwaite is a freelance software architect with over a decade of experience based in Haute-Savoie, France. He's been coding with .Net since version 1.0 and likes to dabble in JavaScript, Java and Python. Web applications are his preference and his framework of choice is ASP.Net MVC.Brad is interested in all aspects of software development from design patterns to optimisations. He loves Code Katas and has a particular interest in the human element of software. He's always seeking out new ways to make software easier to create and change.You can follow Brad on Twitter via @contentedcoder and read his personal blog at www.contentedcoder.com where he writes about balancing the technical and practical.


Related Posts

Comments