Hopefully just a brief post in response to one I consider to be spectacularly wrong.

TL;DR

  • Don’t test other people’s code.
  • Don’t test implementation details.
  • Don’t pad your test suite with pointless tests.
  • Don’t have code which is only ever called by tests.
  • TDD is awesome; don’t make it look bad.

What’s all this about then?

The author of the above-linked post knows a hell of a lot about TDD and the various disciplines that go along with it, and I’ve generally got a lot of respect for him and his work, but when you’re wrong, you’re wrong, and when you’re an authority figure and you’re wrong it needs to be pointed out in case you make a lot of other people wrong too.

The assertion (no pun intended) at issue is that this line of code requires a unit test to cover it:

[sourcecode language="csharp" padlinenumbers="true"] public int Year { get; set; } [/sourcecode]
The proposed tests for this single line of code are:
[sourcecode language="csharp"] [Fact] public void GetYearReturnsAssignedValue() { var sut = new DateViewModel(); sut.Year = 2013; Assert.Equal(2013, sut.Year); }

[Fact]
public void GetYearReturnsAssignedValue2()
{
var sut = new DateViewModel();
sut.Year = 2010;
Assert.Equal(2010, sut.Year);
}
[/sourcecode]

The post goes on to discuss using parameterized tests to “save time”, but never thinks to suggest writing some reflection code that finds all the public types in an assembly, then all the public properties on all of those types, and tests setting them to all the possible values to make sure it always works, if you will excuse the reductio ad absurdum (of course, this wouldn’t show up in your code coverage, so you’d probably end up using T4 templates to generate parameterized tests and then check them into your test suite…).

Anyway, here are a few reasons why it’s wrong.

1. It’s testing the compiler, not your code

The line of code being tested is syntactic sugar for a property with a backing field. The tests are checking only that the compiler has done its job properly, and that’s already been extensively tested by the people who wrote the compiler. You don’t need to test it again. This applies to all language features, standard library functions and classes, and public APIs. You don’t test the .NET StreamWriter class to make sure it writes to a stream; you test the code you’ve written to make sure it writes the right thing to the stream.

2. It’s test-driving an implementation detail, not application logic

This Year property is clearly used by some application logic, otherwise it is redundant and should be excised. Both the getter and the setter for the property will be covered by the tests which test whatever the class with the year property actually does, and hopefully with a representative range of valid values.

There is no user story on the board which says “As a developer using the DateViewModel, I want to set the Year property to a positive integer, so that I can get it back again later”.

There may be a user story that says “As a user, I want to be prevented from accidentally adding events in the past”, in which case there should be some validation code that happens in the view-model when the Year property changes, and that should be tested.

In the paragraph headed “Causality”, the author makes the argument that “the tests drive the implementation”. This is true, but what you are implementing here is not a Year property; it’s some kind of higher-level date-related functionality that is required by your application. The Year property is a part of that implementation; it is not the implementation itself.

3. Pointless tests are worse than no tests at all

Using the suggested approach to testing, it would probably be possible to achieve 100% coverage of all public properties and methods in a project without actually testing 100% of the application logic. Now you have a metric that tells you you’ve done everything right, even though you’ve done everything wrong.

For example, let’s say our hypothetical DateViewModel also has Day and Month properties, and a Date property, all of which have getters and setters. Setting any of Day, Month or Year should change the Date property, and setting the Date property should change all the others. But with tests that test each property in isolation, you have 100% code coverage, and no useful information at all. Conversely, if you have a test for the Year property that asserts against the Date property, and a test for the Date property that asserts against the Year property, then the other tests are redundant, and redundant tests serve only to make the test suite look better than it really is.

4. It hides code redundancy

Code coverage is not just a metric for making sure you’re testing properly, it’s an excellent way of detecting redundant code. If you have worthless tests covering essentially non-functional code, then neither the compiler nor your coverage tool will ever be able to say “this property doesn’t appear to be used by anything”.

5. It presents TDD in a terrible light

The author of the post suggests that Bob Martin’s statement that “you don’t need to test-drive trivial code” is “horrible advice for novices”. I would submit that the counter-advice he goes on to give is far more horrible, since it makes TDD look unnecessarily time-consuming, pedantic and pointless. Telling people that they can’t apply judgement, common sense and pragmatism to TDD is going to result in them writing the whole thing off as a bad job, which is a far worse outcome than them maybe skipping the odd test here and there (which they will learn not to do over time, as those missed tests come back and bite them in the backside).

2013-03-12: The comments have descended into abusive nonsense, and been disabled.