Pinderkent

Recently, I've been working with a software development team that makes use of peer code review. They've opted to take a relatively lightweight approach, with one or two of the other developers reviewing each commit. This process has apparently worked quite well for them over several projects now. The main benefit is higher-quality software. Another benefit is that more of the undocumented knowledge about the software is spread among more of the developers. In general, their communication is much better than that of many other development teams I have worked with. I think their peer review process has helped with this.

Although they don't really adhere to the practices of test-driven development, they do also make use of a large number of automated unit tests written using a custom framework they developed in-house. Part of their development process requires that every commit include new or updated unit tests, where necessary. Another requirement of their review process is that all of the unit tests must run successfully before the commit is made, and this must be demonstrated to the reviewer.

Unfortunately, they seemed to put little to no emphasis on reviewing the actual unit test code. The reviewers would typically put much emphasis on reviewing and suggesting changes to the application code, but as long as the unit tests all passed, the code changes there were generally ignored. While ignoring the unit tests did speed up the review process, it left the team vulnerable to bad unit tests.

Early last week, one of their more important customers called in with a problem they were having with the software. For this team, it was actually quite rare for this to happen. So they immediately halted their development efforts, to focus the team on finding and solving this particular customer's problem. After some initial difficulties reproducing it, they soon enough found the case. It ended up being a relatively benign problem, and was quickly fixed.

Unfortunately, several of the developers spent an entire day investigating this problem. For a small company, waste of this sort is very detrimental. But what made it worse, however, is that an automated unit test had been written in the past to detect this very problem. Checking their code repo's logs showed that a critical assertion had been commented out of this unit test during a bug fix. Their review logs showed that the code changes for the fix had been reviewed by a couple of other developers, including the team lead. But they'd neglected to review the unit test code change, and didn't notice there was a problem with the unit test because all of the tests still ran without error.

If they had reviewed the unit test changes as part of the bug fix code review, I have no doubt that they would have caught the issue immediately. Viewing a diff of the changes made it obvious why it happened. The developer had been commenting out certain other assertions and replacing them with new assertions. But apparently this developer had done a search-and-replace on the source code file that ended up accidentally commenting out the unrelated, yet textually similar, assertion.

So I think the lesson we can take from their troubles is that automated unit tests are just as essential as application code. When it comes to peer code reviews, it's often worthwhile to put as much emphasis towards reviewing such unit test code as is put towards the app's code itself. Indeed, their peer code review policy has been updated to include mandatory reviews of any unit test changes. And although this will cause the reviews to take a little bit more time and effort, catching even one incorrectly commented line of unit test code, for instance, could save them many hours of development time, as well as prevent inconvenience to the users of their software. That will likely make it well worth the extra effort.