Thoughts on Unit Testing

I've had several discussions with people with regards to what makes a quality unit test. Should we test protected methods? It's not line coverage that matters, rather the path coverage that is the key! Should we let the code hit the database or mock the database away? If we mock it away doesn't that mean we leave the code open to issues in the Data Access Layer? What's the point of unit testing anyway?

So what's the answer?
"It depends". It depends on what you want to achieve, it depends on what you are willing to do in order to make sure that the test is appropriate for the logic you are trying to prove is correct. I don't really care anymore if you want to test protected methods or not, although I know myself and Seb have had rather large "debates" with Rowan and Ben before about whether you should test protected methods or not. In fact Ben has written an article about this, and it pains me to say it, but I am now edging towards agreeing with both of them that maybe you shouldn't. But in the end, does it really matter? Is it really black and white, right or wrong to test protected methods? If you want to test a protected method because the inner workings are quite complex, and it really is just easier to test it in isolation then go for it. Any test is valid if it actually asserts something. If you are trying to test protected methods then maybe you are missing an opportunity for another class, or the logic under test is in the wrong class, but that is by the by.

Assert something, anything but make it worthwhile
Asserting something is equal to what you expect. That is pretty much the quintessential aspect of a unit test. Set up some code, run it and then assert the output and make sure that the outcome is the same, over and over again. If you do not assert something then you are really just running some code, which isn't worth the effort. I've seen many unit tests now that do not assert anything, and when raised with the developer the answer is: "But it has 100% code coverage". Well, that's fantastic, but if that is your goal then I can write some unit tests in seconds that runs a whole bunch of code, doesn't really mean it is doing anything useful – well maybe checking for parse errors at most. Code reviewing reams and reams of unit tests can be tedious when you see test after test not asserting anything, so I decided to take the opportunity and write a new feature for PHPUnit this week. It allows a developer to pass in a –strict flag whilst running phpunit so that it would mark a test as incomplete if it did not assert anything. I personally would want to see it fail, but there may be some valid reasons as to why you haven't asserted something yet (maybe!). This feature will be available in the PHPUnit 3.5 release and I'm really looking forward to using it on a daily basis. I've already ran it over some of the tests that I have access to, and it is rather surprising how many do not assert anything. I've so far found one of my own tests that do not assert anything, imagine my horror. The only excuse I can come up with is that it was one of my first! This is a great opportunity however, as it allows us to direct some attention at the quality of tests rather than making them all pass, something which I have discussed previously. Having an automated tool to highlight these kind of mistakes is the way forward as it removes the human aspect which allows such mistakes to happen in the first place.

Path Coverage
So, this brings me onto the code coverage discussion. Having 100% code coverage from tests that assert something is one thing, but making sure all paths throughout the code have been called is another. I think the only sane way of doing this is by using data providers, something which I absolutely love and use religiously. Let's take a peak at some code from PHPUnit (Full copyright disclosure can be found here):

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
if ($useXdebug) { // BRANCH 1
$data = $this->codeCoverage->stop(FALSE);

if ($this->collectRawCodeCoverageInformation) { // BRANCH 2
$this->rawCodeCoverageInformation[] = $data;
} else { // BRANCH 3
$filterGroups = array('DEFAULT', 'TESTS');

if (!defined('PHPUNIT_TESTSUITE')) { // BRANCH 4
$filterGroups[] = 'PHPUNIT';
}

$this->codeCoverage->append($data, $test, $filterGroups);
}
}

if ($errorHandlerSet === TRUE) { // BRANCH 5
restore_error_handler();
}

$test->addToAssertionCount(PHPUnit_Framework_Assert::getCount());

if ($this->strictAssertions && $test->getNumAssertions() == 0) { // BRANCH 6
$this->addFailure(
$test,
new PHPUnit_Framework_IncompleteTestError(
'This test did not perform any assertions'
),
$time
);
}

if ($error === TRUE) { // BRANCH 7
$this->addError($test, $e, $time);
}

else if ($failure === TRUE) { // BRANCH 8
$this->addFailure($test, $e, $time);
}

You can get 100% code coverage on that snippet by making sure that every line is called, which is a start, but what happens when some of the conditions are met and not others? What happens if you go down branches 1, 2, 5, 6, 7, 8 with one test, and then 1, 3 and 4 in another test. That's it, 100% code coverage, but what happens down the other paths that the code can take? One path may fundamentally change the outcome of the function, so you should make sure that all possible paths are covered. Without doing that, 100% code coverage is not as good as you first thought. I have also seen bugs in code that has been unit tested, and it is because certain paths have not been tested. Unit testing paths may also highlight redundant code, again something I have seen recently.

I love unit testing and having the confidence to re-factor logic without fear of breaking something unforeseen in another area, but you need to make sure that you really are covering what you think you have covered.

And on that note, I'm done!

2 thoughts on “Thoughts on Unit Testing

  1. In most languages you can’t test private methods directly, you HAVE to test via the classes contract or not at all.This post gives a wide range of options for testing private methods and while I don’t agree with the one the author has selected, there are plenty of reason why this is the case in other answers http://stackoverflow.com/questions/34571/whats-the-best-way-of-unit-testing-p…The thing I take from those answers is that if you can only get confidence in your unit tests by testing the private methods then there’s probably a better way of writing the class.

  2. Testing protected/private methods really is the one thing no-one seems to agree on.. I’ve never seen a book state that you shouldn’t but I have to admit that just recently I think I would now change the class, rather than go to some extremes in order to test a protected method. In think the situation dictates the need.I know that this will please both Rowan and Ben as I can remember having heated discussions about this in the past, which means they didn’t do a good enough job at convincing me ;-)

Comments are closed.