I frequently see a bold claim come up in tech circles. That as a team you’re wasting time by doing code reviews. You should instead rely on automated tests to catch bugs. This surprises me because I can’t imagine anyone thinking that such a blanket statement is true. But then most of the time this is brought up in places like Twitter where nuance is impossible and engagement farming is rife. Still it got me thinking about why I think code review is important even if you have amazing tests.

Before I elaborate I’ll point out what should be obvious. Different projects have different needs. You shouldn’t listen to me tell you that you must do code review any more than you should listen to anyone else tell you that you must not do code review. Be pragmatic in all things. Beware one-size-fits-all statements (in almost any context).

We’ve been religiously performing code review on every (well almost every) patch at Mozilla since well before I joined the project which was quite some time ago. And in that time I’ve seen Firefox go from having practically no automated tests to a set of automated test suites that if run end to end on a single machine (which is impossible but let’s ignore that) would take nearly two months (😱) to complete. And in that time I don’t think I’ve ever heard anyone suggest we should stop doing code review for anything that actually ships to users (we do allow documentation changes with no review). Why?

# A good set of automated tests doesn’t just magically appear

Let’s start with the obvious.

Someone has to have written all of those tests. And others have to have verified all that. And even if your test suite is already perfect, how do you know that the developer building a new feature has also included the tests necessary to verify that feature going forwards?

There are some helpful tools that exist, like code coverage. But these are more informative than indicative. Useful to track but should rarely be used by themselves.

# Garbage unmaintainable code passes tests

There are usually many ways to fix a bug or implement a feature. Some of those will be clear readable code with appropriate comments that a random developer in three years time can look at and understand quickly. Others will be spaghetti code that is to all intents and purposes obfuscated. Got a bug in there? It may take ten times longer to fix it. Lint rules can help with this to some extent, but a human code reviewer is going to spot unreadable code a mile away.

# You cannot test everything

It’s often not feasible to test for every possible case. Anywhere your code interacts with anything outside of itself, like a filesystem or a network, is going to have cases that are really hard to simulate. What if memory runs out at a critical moment? What if the OS suddenly decides that the disk is not writable? These are cases we have to handle all the time in Firefox. You could say we should build abstractions around everything so that tests can simulate all those cases. But abstractions are not cheap and performance is pretty critical for us.

# But I’m a 100x developer, none of this applies to me

I don’t care how senior a developer you are, you’ll make mistakes. I sure do. Now it’s true that there is something to be said for adjusting your review approach based on the developer who wrote the code. If I’m reviewing a patch by a junior developer I’m going to go over the patch with a fine tooth-comb and then when they re-submit I’m going to take care to make sure they addressed all my changes. Less so with a senior developer who I know knows the code at hand.

# So do tests help with code review at all?

Absolutely!

Tests are there to automatically spot problems, ideally before a change even reaches the review stage. Code review is there to fill in the gaps. You can mostly skip over worrying about whether this breaks well tested functionality (just don’t assume all functionality is well tested!). Instead you can focus on what the change is doing that cannot be tested:

• Is it actually fixing the problem at hand?
• Does it include appropriate changes to the automated tests?
• Is the code maintainable?
• Is the approach going to cause problems for other changes down the road?
• Could there be performance issues?

Code review and automated tests are complimentary. I believe you’ll get the best result when you employ both sensibly. Assuming you have the resources to do so of course. I don’t think large projects can do without both.