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.

CodeProject