Should tests be code reviewed?

👋Hello technical folks ~

We are confident that most (if not, all) of us are familiar with writing tests and have been writing them. Now the question is, do you actually perform code review on the tests that you currently have?

By performing “code review”, we don’t mean just checking for duplicated code, bad naming conventions, long functions, etc. We mean by checking if the tests are really working, useful, and making sense, or are the Software and/or Quality engineers adding tests just to meet the team’s test target/KPI?

Hmm… What do you mean by that?

Writing meaningless test(s)

There may be cases whereby engineers put in useless tests such as checking for the forever true condition statements.

An example of the forever true condition statement

This usually happens due to either of the reasons:

1. the team is rushing for the release and have very little to no time for tests, but one of the team’s Definitions of Done (DoD) is to write tests for x number of functions/features
2. the engineers temporary placed in some dummy checks just to stop the build from failing, but ended up forgetting about it when they’re done with the implementation

Writing test(s) that never actually fail

Most of the time when we write tests, we only put in the correct positive flows and ensure they passed (come on~ touch your heart and answer honestly 😏). This is because we trust the person who set up the test framework has done his/her due diligence to ensure that it is working properly — checked and verified the wrong tests will fail and be reflected accordingly. On top of that, we also have this “when it works for one test, it should also work for the rest of the tests” mindset. This resulted in the wrong tests passing and we just assume that everything’s correct and working fine, especially if your project is running a long list of tests.

Hold up! Does this mean we have to check for ALL types of tests?

If you’re familiar with the Testing Pyramid, we’ll know that the main bulk of the tests comprises the unit tests. Hence, we should also target to code review the unit tests during the peer-to-peer code review (or pull request) stage before merging the code back to the Trunk (or master). But that’s not to say the rest of the tests aren’t important.

Here are some areas to be reviewed for unit tests, on top of the usuals such as code smells:

• Each test must always fail when it doesn’t meet the requirements
  Before writing the happy flow, always ensure that the test will fail with wrong requirements.
• Write the tests first before writing the function
  With this sequence in place, you can ensure that the tests will always fail (initially) until the actual logic/conditions have been implemented.
• Each test should only be checking for one thing
  If you notice any test that is very long, then you should confirm if the function is checking more than one thing.

Writing tests can help to identify and fix any bugs early in the development phase. It also acts as an evidence that your code actually works as intended and meets the requirements. Thus, having meaningful and accurate tests is very important, and code reviewing them is one of the ways to ensure so. We hope this article has helped you to decide (or at least think about) whether you should be code reviewing the tests you and your team have written, and what you should be looking out for while reviewing.

Sure enough, this process is tedious and takes up resources which can be used to increase team delivery. But ask yourself this question: spending more time and resources ensuring quality in the system vs. having production issues — which cost more?

🧙🏼‍♀Team Merlin 💛
Application security is not any individual’s problem but a shared responsibility.