A code review is a quality assurance activity to ensure that check-ins are reviewed by someone other than the author. If not pair-programming when writing the code, mature development shops often practice this activity as it’s an excellent way to catch errors early in the process.
I often state that test code should be treated with the same care as feature code, and therefore it should undergo code reviews as well. Test code can be reviewed by other automation engineers or developers who are familiar with the project and code base.
But what exactly should you look for when reviewing test code? Here are 8 specific things I pay close attention to when conducting code reviews for tests.
1. Does the test verify what’s needed?
When verifying something manually, there are a lot of subconscious validations that are being made. So if anything was incorrect, we’d likely notice. Our tests are not as good at this. In fact, they will only fail if the conditions that we explicitly specify are not met.
For example, let’s say we have a test to increase the quantity of one of the products within an online shopping cart. And here’s the test that you need to review:
@Test
public void testIncreaseQuantity() {
cart.addProduct("Be A Good Person");
String product = "Air Fryer Squad";
cart.addProduct(product);
cart.increaseQuantity(product, 1);
cart.clickUpdate();
assertEquals(cart.getQuantity(product), 2);
}At first glance, this looks fine. The test increased the quantity of one of newly added products by 1 and is verifying that the quantity is now 2. However, let’s look at the UI.
There’s so much here that’s not being verified by this test.
- Where’s the assertion to make sure the product’s price was updated correctly?
- How about the subtotal of the entire cart? This needs to be verified.
- The tax should change based on the total, so when the quantity was increased, the tax should have increased. Where’s the assertion?
- The shipping is a flat rate. When the cart changes, how can you be sure the shipping cost remains the same?
- What about the other item in the cart? Was it inadvertently affected when we changed the quantity of Air Fryer Squad shirt? Who knows…there are no assertions for this.
As you can see, the one action of increasing the quantity affects many different aspects of the shopping cart – and this is often the case with software features. So when reviewing test code, make sure the test is including assertions for all that is needed. Pro tip: visual testing with Applitools will catch all of these subconscious assertions without you needing to think of them all; highly recommended! ?
2. Does the test focus on one thing?
Each test should focus on a single thing. This may be a bit confusing because in the test above, I mentioned a bunch of things that should be asserted. However, all of those assertions work together to verify a single thing (increasing a product’s quantity within a shopping cart).
If, however, the test above also verified the company’s logo or some other feature besides the cart, that would be outside of the scope of this test.
3. Can the test run independently?
In addition to focusing on a single thing, each test should be independent, meaning it should not rely on other tests at all. This makes it much easier to track down failures and their root causes, and will also enable the team to run the tests in parallel to speed up execution if needed.
While the author may agree that tests should be isolated, they still sometimes fall into the trap of using related test runs as setup for other tests. For example, if the test is to remove an item from the cart, it’s tempting to run the “add to cart” test first and depend upon it before running the “remove from cart” test. Call this out in reviews! Your recommendation should be that the test create and delete whatever it needs, and if possible to do this outside of the GUI. More on this in #4 ⬇️
4. How is test data being managed?
How tests deal with test data can be the difference between a stable and reliable test suite versus a flaky and untrustworthy one. As each test should be developed to run independently and all tests should be able to run in parallel at the exact same time, each test should be responsible for their own test data. Trying to share data that is being manipulated is the perfect ingredient for an unreliable test. When the tests are running in parallel and have different expectations of the test data’s state, the tests end up failing when there’s no real issue with the application. Recommend to the author that they create whatever data is needed for the test within the test itself. Since adding something to the cart already has a test, the “remove item” test can use code seams such as an API or database call to add the item to the cart.
5. Is there a separation of concerns?
Remember, test code should be treated with the same care as feature code. That means that clean coding practices, such as separation of concerns, should be followed. The test method should only focus on putting the application in a desired state and verifying that state. The implementation of manipulating the application’s state should not be within the test itself.
For example, in this test, notice the call to addToCart(). The implementation of adding a product to a cart is not the responsibility of the test, and therefore it is separated out into another method, and even another class. Same goes for goToCart().
@Test
public void testAddToCart() {
product.addToCart("Be A Good Person");
page.goToCart();
eyes.checkWindow();
}
Likewise, the non-test methods should not make any assertions. Their responsibility is to manipulate the state of the application, and the test’s responsibility is to verify that state. Adding assertions within non-tests decreases the reusability of those methods.
6. Is there anything in the test that should be a utility?
Separating concerns already addresses this in most cases, but double check that the test isn’t implementing something that can be reused by other tests. Sometimes this may be a common series of verification steps that certainly is the test’s responsibility, however, is/will be duplicated across multiple tests.
In these cases, it’s good to recommend that the author move this block of code into a utility method that can be reused by multiple tests.
7. Are the selectors reliable?
For GUI and mobile tests, the author will need to use selectors to locate and interact with the web elements. The first thing to ensure is that these locators are not within the tests themselves. Remember…separation of concerns!
Next, make sure the selectors can stand the test of time. For example, using selectors that depend on the structure of the page (e.g. indices) will only work until the structure of the page is changed.
This is a horrible selector and it’s only a matter of time before the test breaks because of it:
//*[@id="main"]/ul[1]/li[2]/aIf another list item is added before this one, all of a sudden, this selector is no longer good. Encourage the author to use unique IDs to locate any elements they need to interact with, even if that means adding the ID to the element as part of this check-in.
8. Is the wait strategy solid?
Flag any hard-coded waits. Automated tests run faster than customers would actually interact with the product and this could lead to issues with the test, such as trying to interact with or verify something that is not yet in the desired state. To solve for this, the code needs to wait until the application is in the expected state before proceeding.
However, hard-coding a wait is not ideal. It leads to all kinds of problems such as lengthening the duration of execution, or still failing because it didn’t wait long enough.
Instead, recommend that the author use conditional waits that will only pause execution for the least amount of time that is needed.
For example, if testing a POST API request, while the response may have been returned, there could still be some processing on the backend. Attempting to run another request on this newly-created entity before the entity has actually been created would result in a test failure. Instead of waiting a set 3 seconds before proceeding, recommend that the author includes logic that polls every few milliseconds until the test is sure the entity has been created, and then proceed.
You’re ready to review!
Hopefully these tips provide something substantial to look for in reviews of test code. Remember, your test code is what’s used to determine the quality of your feature code, so it’s really important that it’s developed with care. If you’re looking for more tips or advice on how to set yourself up for success in test automation, check out my free course over at Test Automation University, Setting a Foundation for Successful Test Automation.
