I suggested this idea a few weeks ago on Twitter and got mostly negative reactions. That’s why I wrote this blog post, to elaborate on the subject in an attempt to convince you. Here is the rule I’m suggesting: always submit changes to the code separately from the changes to its unit tests. Simply put, in one pull request you modify the tests, maybe marking some of them as “disabled.” You merge this pull request and then make the second one, modifying the code and most probably removing the “disabled” markers from the tests. You don’t touch the body of the tests in the second pull request.
It may look like a contradiction of the principles of TDD. However, to me this approach looks like an extreme application of TDD, not a violation of it. First we merge the tests, which most probably will break the build, since the functionality that they test is not present yet. In order to avoid the broken state of the build we disable the new tests that we added and the tests that we modified. In JUnit 5, for example, we do this with @Disabled annotation.
Reviewers validate the changes you make, asking themselves these questions: “Do we really need this new functionality? Does it conflict with the existing functionality? Does it make sense to test new functionality this particular way?” They don’t think about how the functionality will be implemented, they only care about the requirements you impose in your tests against the product. The reviewers act more or less as requirements engineers at this stage. They validate the intent, not the realization of it.
Then, in the second pull request, you modify the code without touching the tests. Now, reviewers can rest assured that you haven’t changed the requirements just to make them more suitable for your implementation. In other words, they know that you didn’t cheat. Since you didn’t touch the tests, it’s a guarantee for reviewers that requirements remain stable and you only modify the implementation. Speaking business language, you don’t change the contract if/when you understand that you can’t deliver what was promised.
Moreover, when you modify the tests only, without touching the code, it’s much easier for the reviewers to understand whether or not your changes truly belong to the problem you are supposed to be solving. We programmers tend to make a typical mistake: we make changes to the code, some tests fail, we fix the tests … no matter whether they are “our” tests or not. We simply make the tests pass regardless of why they fail. Instead of listening to them, we shut them up. Later, the reviewers may not understand why some tests were modified. Especially if a pull request is big. They will most probably blindly trust you and merge the pull request.
That’s why separating tests from code is a solution. First, the tests get modified and the reviewers will pay attention only to the scope of tests. They will easily catch you if the changes are too broad and are not related to the problem you are solving. Then the code gets modified and reviewers don’t need to worry about tests at all. They don’t pay attention to them, they only review the implementation. They know that you can’t break the tests since the build pipeline won’t allow you to do this.
What do you think now? Does it make sense?
