Code Review and Test First Development
How to leverage existing tools and processes to make sure an efficient Test First approach is being applied
There is a term called Test First, it is used when the developer starts writing the tests before the implementation. Test-Driven Development (TDD) is a little different.
It is reasonable to expect that, in some projects, some practices that yield great benefits, like TDD, are not being applied by most developers, although there are those who understand and apply it very well. Most developers might not even understand what is the purpose of that practice and it should be the responsibility of the other members of the team to help them understand the benefits:
When we write our passing tests after writing the application code, even if we believe to know exactly what the code is doing, there is a high likelihood of us being wrong, so we might write a test that passes but doesn’t cover what we intended to do, or we might write a complicated test because the architecture turned out to be sub-optimal.
When we write our failing tests before writing the application code, even if we believe to know exactly what the code is doing, there is a high likelihood of us being wrong, so we can empirically check early that we are wrong and write a testable application code that, by consequence, will also have an optimal architecture.
TDD is the mindset built around the process of making sure the test fails before making it pass so that we can ensure precise coverage and drive a better architecture
One of the most important practices that encourage a team to learn with themselves is Code Review. Some of the benefits of Code Review are:
- Share knowledge in the form of feedback from other co-workers
- Find honest mistakes through an unbiased view of the changes
- Make sure that the changes comply with the project standards
Many projects run Code Review slightly differently. But a common pattern is to hand over the diff of a small set of changes to another member of the project so that they can statically analyze it with an unbiased view, for the purpose of finding early problems and increase the quality of what is being delivered.
Code Review is to hand over a small set of changes to somebody else for the purpose of statically analyzing it with an unbiased view and increase the quality of what is being delivered
What if it was possible to use Code Review to help the team improving its testing skills by deliberately exposing the way they write the tests?
If the reviewer only looks at the final changes that already contain the related tests, it will be very hard to verify how those tests were written in the first place because they will already be passing.
Let’s say, for example, that we want to test a username validation regex in JavaScript that is already being used elsewhere in the front-end but we have no Test-Driven specs to look up:
/^(?=.*[a-z])[1-9a-z][0-9a-z]{3,19}$/iIt will be very hard to infer that the developer has used TDD because it is very hard to reason about the architecture of a regex to make sure every rule was strictly necessary.
One could argue that, in order to infer if an efficient Test First approach was used, the reviewer should run the code locally again, tweaking the application code (and the tests) to see if the tests are precise or not. The problem with that approach is that, besides doing the work twice, the reviewer is not doing static analysis anymore, they are actively running the code and doing dynamic analysis instead. The reviewer has moved away from doing Code Review into doing something else, and that has an additional cost. We should refrain from increasing the cost of analysis and try to make it possible for the reviewer to statically verify if a Test First approach was applied or not.
Inferring a Test First approach through Code Review is hard because the final changes will contain the application code and the passing tests.
It will be harder to infer a mindset, like TDD, when the only thing we have is the final code. If we can’t statically verify by looking at the diff that the tests were written before or after the application code, there is no apparent way we could leverage Code Review to check a Test First approach and potentially find opportunities to:
- Share Test First knowledge for other co-workers
- Find honest Test First mistakes
- Make sure that the changes comply with the project’s Test First standards.
But if we think about the Code Review process differently and make some exceptions to the definition of atomic commits, we might be able to come up with something that could help, even if temporarily.
For example, let’s assume we are using a Github Code Review workflow. If we don’t restrict the Code Review process to be responsible for analyzing only the final diff, we might be able to verify if TDD was applied using a special pattern of commits, Feature Pull Requests and a Continuous Integration Service that runs on each Commit of that Pull Request.
Ideally, Test First approaches like TDD should be done in the local development cycle, and commits should contain the passing test inside an atomic change. Trying to apply it in Code Review has a bigger cost than doing it earlier.
It is proven that good Pair Programming and coaching are the best way to help driving a TDD mindset. But if there are too many people in the team that may or may not understand TDD, it could be worth facing the increase of the cost of change a little bit in order to expose how people create a design with a Test First approach in the Code Review step. An experiment that should be temporary, because it deviates from the purpose of an atomic commit and the CI.
The idea is that, for review purposes only, the first commit will log the change that makes the test suite running on the CI fail. The second commit will log the actual implementation that makes the test suite running on the CI pass. If all of this is done in a Pull Request with a single concern (not the main branch or pipeline), then the author of those changes can expose their inner thoughts and the reviewer can potentially verify if TDD was used correctly by looking at the history of changes on each of those commits.
There are some clear problems with this:
- TDD is a mindset, so it will still be hard to infer if it is being applied by looking at the commits, but at least there is an exposure of how the application code is being created by the Test First approach, which might make the intent clearer to the reviewer. If necessary, this can trigger a coaching or Pair Programming with the author of the changes to explain more efficiently how TDD works.
- It might consume unnecessary resources from the CI because we would be using it for something it wasn't supposed to be used for.
After the review, since we should not log a change that makes the CI fail, the changes will be squashed into an atomic commit that will contain the application code with the passing test, which can be easily automated.
By logging a carefully crafted set of changes for the purpose of improving the Code Review process, it might be possible to statically verify the inner thoughts of the author behind those changes
Creating a process like this and put it as one of the criteria for the Code Review might help driving the mindset of crafting changes in a way that makes it easier to expose potential problems for the reviewer. If the reviewer has difficulties to see a problem, then it will be unlikely that the author of the changes will receive an efficient feedback, which is one of the main goals of Code Review.
Conclusion
The Code Review process don’t need to be restricted by only looking at the final changes. By taking a step back and leveraging the VCS, it might be possible for the author to expose additional information so that the reviewer can infer the inner thoughts behind the changes and be able to provide a more efficient feedback.
The idea is: don't be afraid of deviating from best practices sometimes and experimenting different things, as weird as they might seem. The difference is that the best practice should be understood, and experiments like this should have a clear and temporary purpose so that it's treated as the exception, not the rule.
There is no 100% solution to identify and make sure someone is following a practice like TDD or Test First, but at least we can try to create experiments that might help that practice to become a habit. After the experiment has been tried and served its purpose, we can go back to the correct way of doing things.
Related Examples:
- A Pull Request with an Atomic Commit that fails and one that passes, exposing the issue and the fix, see the Travis Console
- A Pull Request with two atomic commits exposing another cross-browser issue
Have some feedback? Check out Twitter | Facebook | Github
