Wouldn't it be beneficial to write tests during code review?

Question

A colleague of mine came up with an idea that I found interesting.

Wouldn't it be beneficial to write tests during code review, by the person doing the review assuming that we don't do TDD?

For this question assume that this is a purely academic project so there is no life at stake. Moreover the team is 4 people. Everyone knows the language and is familiar with all the tools/libraries/frameworks used and can write tests. So basically people who are not senior fullstack lead ninja engineers but decent coders.

Pros I found:

1. Encourages deeper understanding of code during review to write meaningful tests.
2. You could then add code review of those tests done by the author of the code being tested.

Cons I found:

1. Feedback loop between code writing and testing grows.

EDIT: I know that it won't work well on "normal" web applications. What I had in mind was a corner case where you implement complex, scientific algorithms that require care to the details. Let's assume something like implementing my own graph library, NLP etc. I wonder if the code we are writing is isolated from databases and such but very tough to comprehend wouldn't the added level of control, the other person who needs to understand the source code and do meaningful test, make the whole process less prone to those less obvious bugs that don't crash the application but ultimately make your results rubbish?

Answer 1

Wouldn't it be beneficial to write tests during code review, by the person doing the review?

I have found that a good time to write tests is when you realize you need a test for a situation.

Task switching for computers is expensive - even more-so for humans.

At this point in time, you generally have a good understanding of the requirements and dependencies for the test. So leverage your team's immersion in the problem. If you need to refine your new test in the future, great, you already have the test framework/fixtures in place, and all you need to do is change the part that needs improvement.

If that happens during code review, why not go ahead and do that? I've done that before. I've found that it's better than not, especially if you can do it quickly, and even better if you wouldn't have done it otherwise.

Assuming that we don't do TDD?

Even if you do practice TDD, if you realize you need a test while doing code review, one that you don't have, why not write the test then and there?

Pros

• You leverage your focus on the code under review.
• Sometimes code review becomes hang-out and chat time when people aren't into it. Writing a test encourages everyone to more actively think about the code being reviewed.
• More junior members of the team will have an opportunity to learn from the test-writing experience.
• You may identify talent on your team that you didn't know you had.

Is it really a con that more tests may lead to more code? If the test was needed, and the code was needed for the test, and now you have it, then that's a good thing.

Caveats

Maybe some of the team needs to be focused on other things. If it causes a distraction from priorities, or your code review goes over schedule, then you need to limit or cut the actual writing of the test out. However, code review can certainly identify tests that need to be written, and perhaps they can at least be stubbed out for the writer to complete later.

Answer 2

This is a wonderful idea, with one caveat. Don't replace developer written tests with reviewer written tests. Have your reviewers look for corner cases and inputs that will break the code. In other words, have them try to write new tests that the original developer didn't think to write.

Writing characterization tests is an absolutely wonderful way to gain an understanding of a piece of code you didn't write. Having your reviewers drive additional tests at the code gives them a much better understanding of how the code works, how it can be broken, and how it can be improved. All the while, increasing your code coverage.

These are all wins in my book.

Answer 3

I don't think the idea is entirely without merit - however, the main benefit of the TDD et al is that problems are found early. The developer is also best placed to spot which corner cases may require specific attention. If this is left until the code review, then there is a risk this knowledge could be lost.

Writing tests during the code review would suffer from the same problem as traditional manual testing - the understanding of the business rules can vary from developer to developer as can the diligence.

There is also the age old discussion which will run and run as to whether developers would test their code so well if they knew there was a testing function further upstream that should catch the more serious bugs.

Answer 4

I agree with @RobbieDee's answer but I have a bit more to add.

If you really like this idea, why not have the same people write the tests before the code as executable acceptance criteria for the user story?

That would do the same thing, still keep the feedback short and get everyone to have a discussion around the story ,which I think would be of greater value.

The downsides are the danger of an endless acceptance criteria meeting :-( and I think you are trying to get people in the code review to get a look at the implementation code but I'd suggest pair programming and rotating pairs as a better solution to that problem.

The OP added an edit where they layout further details of this being a difficult or algorithm heavy feature.

In response to that I would add that your instinct of getting more eyes on the problem and solution is good. Maybe pair with multiple people one on one until everyone has seen the really difficult bit of implementation code and tests. Each throwing out new ideas and adding more value.

There is an idea sometimes call mob-programming, like pairing but with more people. This is almost what you are talking about but they help at the time of writing rather than in a formal review afterwards. This is not for everyone, and can require a strong driver (leader) to make it work, or a team that is very comfortable with each other and the process.

Doing the after the fact mob-programming I guess would have many of the same advantages of many eyes seeing the problem and and suggesting improvements and if that is how your team are comfortable operating then that can help but I would really try to keep the necessary occurrences of this down to a minimum as I think it could slow the team down.

Answer 5

Like you say, if you're running a TDD team, then this is moot since the code should already be tested.

Overall I don't think this is all that great an idea, but it depends on your current approach and what works for you. Basically, the problem I see is that you lose the "short feedback loop" advantage of tests. Getting instant notification the moment you break something even as you're writing new code is where tests really shine. If you postpone testing until code review, you are basically saying "well we COULD have fixed this issue earlier in less time and with fewer people involved, but at least we all learned something (maybe)". I'd prefer just making sure people submit tested code for review, and then you judge the correctness and maintainability of the tests. After all, code review is for reviewing, not for writing code.

On the other hand, I DO recommend you to FIDDLE with the tests/code during the review. Try to break something. Comment out an if condition. replace a boolean with a literal true/false. See if tests are failing.

But yeah, all in all, I recommend you write your tests together with your code and then review it all at once.

Answer 6

It depends what you are doing in code review. I think there are two main reasons for writing tests at that stage:

• first, if you do also refactoring during code review, and you note there are not enough unit tests to cover the kind of refactoring you want to apply, add such tests

• second, if the code looks to you as if it might have a bug and you want it prove (or disprove) this, write a test for it

Both cases express a need for tests which are not there at the moment, but should be. Of course, it might depend on your teams culture if these kind of tests should be written by the reviewer, or by the original author, but someone should write the tests.

Actually, I don't think this is "a corner case" just suitable for "complex, scientific algorithms" - quite the opposite, this is suitable for any kind of software you expect a certain degree of quality from.

Answer 7

No, don't do it. You'll make them think TDD is horrid.

I think @k3b has it right in the comments on the question. Code written through a TDD-style process tends to look, and interact, very differently to code written without tests. Adding (good) tests to untested code usually takes a lot of refactoring the code to clarify its intent and moving parts.

By adding the tests after writing the code, you miss the architectural aspects of TDD's benefits (which to my mind are one of the major benefits). Not only that, you are asking somebody else, who is not nearly so familiar with the code, to take the hit of adding tests which are already hard to add.

Either the person adding tests will have to significantly refactor the code, or they will have to work very hard to test untestable code. Either way, they are not going to enjoy the experience. Even though this might not be classic TDD, they won't see it that way, and you could put them off TDD once and for all.

(If you're following a TDD process already, writing additional tests during code review would be less harmful, although in my experience, if the tests are already well-written, it's just as easy to explain the extra test to the person submitting the code for review and have them write them.)

Answer 8

Unit tests during code review are a poor substitute for unit tests during development.

What you're suggesting makes a lot of sense, intuitively. What's the review for? To check that the code is good. What are tests for? To check that the code is good. So why not combine the two?

Here's why.

Bringing code under test is hard work. Writing code that just works at the one thing it's meant to do is one thing; writing code that can be effectively and efficiently tested is another. Just the fact that the code now runs under two scenarios - "real work" and "test" - demands much greater flexibility, demands that that code be capable of standing on its own in a meaningful way.

Writing your code so that it's testable is extra work and skill. Refactoring somebody else's code for testability, when it wasn't written with testability in mind to begin with, can be a major task.

You're duplicating effort between the developer and the reviewer. Presumably, your developer isn't handing his code in for review without at least some level of confidence that it's working. He already needs to test the code. Now, there are different levels and scopes of testing. QA tests the code after the developer and the reviewer. But whatever scope you think is appropriate for the developer and reviewer, it makes no sense for the developer to figure out how to test the code to that level once, but make his tests throwaway and difficult to reproduce, and then bring in the reviewer to develop test again, this time ones that are automated and reproducible. You're just having both of them invest time in writing the same tests - once poorly, once well.

You're turning review into a much longer, more laborious step. If testing is a major part of the review process, what happens when some tests fail? Is the reviewer responsible for getting the tests all running, so she needs to debug the code as well? Or is it going to be ping-ponged back and forth, one writing tests, the other getting them to pass?

Sometimes you can write a whole bunch of tests that are all orthogonal to each other, so you don't need to ping-pong. Reviewer writes a dozen tests, half of them fail, developer fixes the bugs and all the tests remain valid, and pass now. But... plenty of the time, you've got blocker bugs, or bugs that require redesign and API changes, or whatnot. If you're tossing responsibility for passing tests back and forth between reviewer and developer, then you're not actually at the review stage. You're still developing.

Needing to write tests doesn't incentivize more thorough review. It basically means that the deeper you go, the more tests you have to write, and probably they'll be hard tests that need to go deep into the system.

Compare to the developer writing the tests, where his incentive is: if I don't write important tests, the reviewer will point that out in the review.

Even the reviewer will have a much better understanding of the system if she needs to go over thorough tests of the code, then if she needs to decide for herself when she can stop writing deep-digging test and just OK the code review.

If the developer isn't writing unit tests, the reviewer won't either. There are many obstacles to adopting testing as a common practice. Maybe you're under too much pressure, and your code base is hard to bring under test. Maybe you're not that experienced in testing, and feel like you can't afford the learning curve. Maybe you've got an axe murderer sending threatening notes to people who write tests. I don't know!

But whatever the cause is, it's safe to bet that it applies equally to the reviewer and to the developer. If the team is stressed out, the reviewer doesn't have any more time than the developer does (if she does, redistribute the work so people aren't so stressed). If nobody knows how to write unit tests well, the reviewer probably doesn't either (if she does, she should sit down and teach her teammates).

This suggestion sounds like trying to pass the buck from one colleague to another. And I'm just not seeing any way for that to work out well, first and foremost because it's really hard (and unhealthy) to create a situation where one person is the only one who can do testing, and another person can't do any testing at all.

---

What does work is having the review cover tests as well. If the developer has already written ten tests, it's much more likely the reviewer can help suggest another ten, than if the developer hadn't written any.

And, if testing corner-cases is a major task, it might make sense to distribute that more widely across the team. **Once the code is testable in the first place, writing more tests becomes much much easier. **

Review is a great time to spot corner cases. And, if the reviewer can jump in and write a test for corner cases she finds, then hey - all the better! But generally speaking, assuming that the reviewer can write tests where the developer didn't sounds like a very poor idea.