Code reviews are a popular method of catching bugs early in development through peer-reviewing someone’s code. But perhaps more important than catching bugs, these reviews also serve as a chance to see how something is built and have a conversation around such decisions. The reviews can focus on any number of things, including logic problems, code and style convention, security problems, and test coverage.
Only the people doing the code review get the benefits of learning about the changes, questioning the assumptions, clarifying ambiguities, and generally participating in the knowledge-sharing. Because the seniority level of the people providing or receiving the review doesn’t matter, testers should be among the peers who review code. We question software differently from our developer peers, so it’s important that we get to participate in a big knowledge-sharing practice.
Code Reviews in Practice
Many companies these days make code reviews a standard practice. The company I work for requires a code review from at least one person before any change can be qualified as mergeable, and our systems won’t allow a merge to take place until that happens. It’s also a separate step in our kanban workflow—after something is done with development, it goes into a code review state before it can be tested or considered deployable.
There’s no shortage of things to try to understand when reviewing someone else’s work:
- How was this code written? What design patterns and conventions are being used?
- Are things readable? Do names reflect the things they are supposed to represent?
- Are there any obvious logic errors?
- Are there any potential security problems with the code?
- Does this code write debugging information to a log file somewhere?
- Does this code use any third-party services?
- Are we including any unit or integration tests with these changes? If so, can I understand what the tests are doing?
- What files are being changed? Do these changes belong there?
Notice that many of these questions can be asked by testers or developers.
At my company, the person doing the code review will open up the pull request in GitHub, which has a nice visualization of the code changes between branches, and use GitHub’s built-in tools to make individual line item comments, request changes, or approve the pull request. Whether you are using GitHub, Stash, or some other tools in your development process, the point is to find out where code reviews are happening and take a look for yourself.
Some companies will have established practices for reviewing code, and others will simply rely on the developers to use their own judgment. In cases where it’s up to the developers, note how the reviews differ from person to person.
Why Testers Should Participate
Through the code reviews I’ve done, I’ve learned (or gotten comfortable with) a big part of our codebase accidentally. For example, I had a backend developer who was confused about how our A/B tests were implemented, so I described the process to him. I knew because I repeatedly saw how we did it. This is a slightly different set of knowledge from knowing how a feature works because I tested it. Thanks to code reviews, I also have some insight into where the code lives and what was involved in the implementation.
Sometimes this helps me test, question changes, or decide to make a small change. Almost always there is some incremental knowledge I walk away with: Oh, I see we’ve decided to switch to this particular third-party service. Good to know.
Eventually this might mean you become the primary person to do code reviews of a particular type because you understand the changes, in addition to testing those changes.
Code changes also can be another input into your testing. If you don’t understand the code changes being made, code reviews could help with that—especially because you get to ask questions! Most code review tools have the ability to comment and contribute to discussions. Even if you don’t do this, reviews are opportunities to talk to a developer about why they did something.
The added benefit is that once developers know you pay attention to code reviews, they’ll explain things to you. You become their rubber ducky. This opens up all kinds of communication lanes. It’s like peering into the whitebox, which can have all kinds of downstream effects in regards to performance, security, and more.
So take the opportunity to review the code and ask questions. Your questions don’t always have to make sense, and you don’t have to know much of anything at first. But if you are patient, you’ll learn a lot.
How to Get Started
So, how do you do that?
The first thing you need to know is that you don’t need to know how to code. The point here is not for you to do any programming, or even necessarily to catch logic errors. You are there to look at the changes, ask questions, and try to learn as much as you can about the changes. If you have access to the code, pull it up and search for the files that are being changed.
The second thing you need to know is that you should start with small changes, because they are easier to conceptualize and model. Sometimes automation code might be a good starting place, but don’t be afraid to look at code reviews across your applications and repositories when you can.
If someone is already assigned to do a code review, ask if you can pair or be a fly on the wall as they walk through it. Ask what they are looking for and watch as they make comments. Are they pointing out obvious things you can see, like logging messages? If the original author is available, ask them to walk you through the changes, and ask questions. Look for misspellings and any potential testability problems. Did this change remove a data-test attribute or some CSS class your automation was working? Does this update contain some text or tooltip changes that are missing punctuation or contain misspellings? These become easy targets to fix as you’re code reviewing or testing.
Pairing works very well with testers when they’re given the opportunity. At first, just generally try to make sense of it all. If it doesn’t make sense immediately, that’s okay; keep up with it. Code reviews are a long-term process for sharing knowledge and getting to know your application. As you see changes happen over time, you will see patterns, get more comfortable with the codebase, and be able to question more parts of the application.
Remember that code reviews are primarily a learning tool. The main purpose is educating the other developers and testers about the changes by showing them the code changes. Don’t miss out on this potential education.
About the author
Chris Kenst is a Software Quality accelerator and Test Engineer for a startup company in Southern California. In his spare time he leads AST’s Webinar program, is a lead BBST instructor. Chris writes at Kenst.com and maintains a open source list of software testing conferences at TestingConferences.org.
User Comments
This is the checklist we use for code reviews.
Structure
o Does the code completely and correctly implement the design?
o Does the code conform to any pertinent coding standards?
o Is the code well structured, consistent in style, and consistently formatted?
o Are there any uncalled or unneeded procedures or any unreachable code?
o Are there any leftover stubs or test routines in the code?
o Can any code be replaced by calls to external reusable components or library functions?
o Are there any blocks of repeated code that could be condensed into a single procedure?
o Is storage use efficient?
o Are symbolics used rather than “magic number” constants or string constants?
o Are any modules excessively complex and should be restructured or split?
Documentation
o Is the code clearly and adequately documented with an easy-to-maintain commenting style?
o Are all comments consistent with the code?
Variables
o Are all variables properly defined with meaningful, consistent, and clear names?
o Do all assigned variables have proper type consistency or casting?
o Are there any redundant or unused variables?
Arithmetic Operations
o Does the code avoid comparing floating-point numbers for equality?
o Does the code systematically prevent rounding errors?
o Does the code avoid additions and subtractions on numbers with different magnitudes?
o Are divisors tested for zero or noise?
Loops and Branches
o Are all loops, branches, and logic constructs complete, correct, and properly nested?
o Are the most common cases tested first in IF- -ELSEIF chains?
o Are all cases covered in an IF- -ELSEIF or CASE block, including ELSE or DEFAULT clauses?
o Does every case statement have a default?
o Are loop termination conditions obvious and invariably achievable?
o Are indexes or subscripts properly initialized, just prior to the loop?
o Can any statements that are enclosed within loops be placed outside the loops?
o Does the code in the loop avoid manipulating the index variable or using it upon exit from the loop?
Defensive Programming
o Are indexes, pointers, and subscripts tested against array, record, or file bounds?
o Are imported data and input arguments tested for validity and completeness?
o Are all output variables assigned?
o Are the correct data operated on in each statement?
o Is every memory allocation de-allocated?
o Are timeouts or error traps used for external device accesses?
o Are files checked for existence before attempting to access them?
o Are all files and devices are left in the correct state upon program termination?
Hi Mark, thanks for sharing your checklist!