What should be done first: Code reviews or Unit tests?

Question

Hello,

If a developer implements code for some module and wants to get it reviewed. What should be the order :

*First unit test the module after designing test cases for the module, debugging and fixing the bugs and then give the modified code for peer code review

(Pros- Code to be reviewed is 'clean' to a good extent. Reduces some avoidable review comments and rework. Cons- Developer might spend large time debugging/fixing a bug which could have pointed/anticipated in peer code reviews)

Or

*First do the code review with peers and then go for unit testing.

What are your thoughts/experience on this?

I believe this approach for unit testing, code reviewing should be programming language agnostic, but it would be interesting to know otherwise(if applicable) with specific examples.

-AD

Answer 1

Code review needs to be done before any commit to the source control. Any bug fix is also reviewed. This way any code in the source control is reviewed.

As part of code review I get developer to demo the functionality as well so that it can be verified that its working.

Answer 2

in my company we strictly follow to do unit-test first. The main reason to do so is that the functionality for which the code changes are made - this is ensured in unit test. If this is not done fully, it does not make sense to do code review.

Answer 3

Imho there's no golden rule which needs to be done first. Early code reviews are nice, especially for a less experienced developer, but in reality if you have to wait for someone to review the code, go ahead and do the unit tests first.

Answer 4

Since you should always start your unit test before you start your class/method and write the two in tandem, the unit test should be as complete as the code that is being reviewed.

Design reviews, sometimes with pseudocode or POC code, often happen before any of this.

Answer 5

Unit test first has the advantage that, during the code review, you can start by reading the tests, which effectively comprise a speclet for the new functionality. When I see a code review containing a bunch of code changes without tests, it's like, ok, what is this supposed to do or fix? When the tests are there, it's easy - the tests set the stage for the goal of the rest of the code review. Plus, the reviewer has more confidence that the code actually works (rather than just someone coded something up and didn't thoroughly test it and wants to spend your time finding bugs they might have found themselves by writing tests).

Plus all the advantages that test-first development usually confers (apart from reviews).

Answer 6

If time was short, which would you rather have done?

As said above, a code review is nice, but unit testing is vital.

Unit testing finds more bugs than code review, in my experience. Therefore, you do unit testing first.

Also, when I do code reviews, I need to know that the code works, so that I can look ritically at the structure of the code, how the SQL is constructed, whether the code fulfils the needs of the functionality etc. It's a lot harder to review code which I don't KNOW works.

From a logistics point of view you need to schedule someone to do the code review, but you don't need this for unit testing.

Unit testing always before code reviews.

Answer 7

Well, if you take the Test Driven Development (TDD, well they like to call it Behavioural so BDD now) then you would write the unit tests before the code.

In this instence though, as the code is already written, I would suggest that a junior developer get the code inspected first, as the meaning of the class under test might well change entirely invalidating the previous test cases. If there is less of an emotional investment in the code being reviewed (created by the increased time spent to make the unit tests), the author will be less prone to argue about changes and could well learn much more from the coding mentor.

I don't hold that opinion too strongly though, and if the developer has nothing else to do while waiting for the code review, or is sufficiently experienced, then certainly get a start on the unit tests.

Answer 8

I am an advocate of code reviewing being the final step after the developer has done all of their testing (unit testing, integration testing, stepping through the code, make the code warning free, and what ever "black" box testing is appropriate). Now I might ask for feedback on the code (hey how does this look for an API) long before this if I have questions about something. My experience is that code reviews find bugs but not a ton of the super hard logic ones unless you spend a lot of time as a reviewer going over the code. The reviewer might ask a couple hard questions (is this thread safe? What happens if x fails?) but it is really hard to figure out all the cases in code that you haven't been thinking about for a while. It is still super useful to help keep track of what is going on in the rest of the project.

Answer 9

If the code review happens before the unit tests are written, doesn't that mean no-one's reviewing the unit tests? Or would you anticipate that being done in a second review?

As others have said, writing the tests either before or at the same time as the code is generally a better idea. I have to admit there have been times where it's been "obvious" where a bug is, and I've fixed it before writing the unit test to demonstrate the breakage... I've found that in such cases, if I'm disciplined and comment out the fix, write the test and see it go red before fixing it, my "guessed" fix is often not the right one anyway.

Answer 10

I'd recommend writing the unit-tests before the review. So, you'll have a safety net to modify the code based on the review comments.

(on the other hand, if the code was not unit tested before the review, you'll never know if those changes break anything :0)

Answer 11

When a bug is reported, and other have confirmed/reproduced than the first thing that needs to be done is to write a reduced test case. The bug fix code written by any developer needs to be run against the reduced test case. Only when the fix code passes the test case should it be moved to review. Review is process that can not be automated. A reviewers task is not to look if the code works, if it is submitted for review it most likely does. His task is to give a 'look' to see if there could be any gotcha in the code, or if the developer didn't follow some coding standards, or did not use the right function and used the wrong library and a bunch of other stuff which only a human will be able to tell.

What you can do is assign someone more experience to write the the reduced test case. And a lot of other developer will be looking at this test case anyway, so this is like some sort of informal review. Once there is not voice being aired about the test case, just get it assigned.

Seriously though you do not want to go through the trouble of writing code and getting it reviewed only to find out that it doesn't pass any test cases at all!

The problem about writing test code later is that two things might sometimes happen: 1. The dev might write the test code with his code in mind. 2. While writing the test code the dev might figure out that, hey!, my code would not pass this and will go and fix his code again and write a test code that is explicitly shoe horned to pass the code he just wrote.

It is always a good idea to get the test case written first.

Answer 12

Unit tests are written first, before there's any code to review ...

Answer 13

I blogged about this great IEEE article by Robert Glass called Frequently Forgotten Fundamental Facts about Software Engineering which is relevant to your question.

• You really need both peer reviews and white box testing.
• If I had to choose between code reviews and unit testing, I would pick neither. The most efficacious defect removing activity is design reviews.

Answer 14

We try to do the following:

• Person A writes some code (with tests of course)
• Person B writes some unit tests against the code
• Person A+B make a Review of the code and the tests

Answer 15

Code review but with the explicit goal that you are identifying test cases to be recorded as you do the review. This focuses the reviewer on looking for anomalies that can be tested, eg: checking inputs, edge cases etc.

I would expect in most places that the coverage of code reviews will be far less than the amount of code being covered by unit tests. A followup review should be scheduled for the same code with tests. The lessons learned in creating and refining these tests then become guidelines for people to self-review and note when writing new tests.

Answer 16

There are several ways to do code reviews so the answer depends on what you mean by it. Code reviews in it's basic form is letting other people take a look in whatever specific piece of code you've written.

When people talk about code reviews they immediately think they are in form of meetings where a bunch of developers sit and walkthrough the code they've done. It doesn't have to be that way. If the developers find it useful, then stick to it. But if it is found to be wasteful then you need to find some alternatives to this types of meetings.

A code review meeting can be scaled in many ways, i.e. it can be short discussion with just two persons (the guy writing code and the guy reading code) or a long meeting where several people that walk through the code.

Before versus after

If you decide to write unit tests after then the code review becomes more about verifying if the code works. What I mean with that is that the reviewers need to step through all conditions in their head, e.g. make sure that the compiled code can take in arguments that wasn't intended.

If you write unit tests before a code review then the code reviews become more about verifying that the code does the right thing and you can be sure that, whatever the requirement specification was made, a specific test added also works and tests what is intended. This also helps traceability between requirement, code and tests.

Why not both at the same time?

…or you could just do it both at the same time using Pair Programming with the driver and mapreader metafor. The driver writes code, while the mapreader is the other set of eyes making sure the driver writes good code and usually writes the unit tests too. This could be considered unit testing and code review in really small, minutes long, iterations.

In my experience, I'd go with tests first using Test Driven Development and let someone check on my code to see I did the right thing. That will work if all developers in a project are aware of good code architecture but that's a whole different topic.

Answer 17

Since "code review" here means review by someone other than the code author, then I'd have to say code review second, and thus unit tests first. But not for any of the reasons given so far.

The lion's share of software cost is in maintenance: fixes and extensions. The greatest cause of defects in that phase is incorrect changes because the maintaining developer did not fully understand the code s/he's changing. Therefore the most cost-effective focus of code review is readability. Not trivial style issues, but readability = understandability + right level of detail in every scope.

A code author's goal, then, is to produce readable code, knowing that a code reviewer will challenge the code for not being readable enough. The code author must first do whatever s/he can to make the code readable to himself first. Writing unit tests is a great way to do that, because it forces the code author to re-read his own code and make sure that it is understandable to a simple and unforgiving audience: the unit test. Actually passing the unit tests is a bonus -- it's the failed unit tests that are worth something because they cause the code author to read the code yet again and fix it. Put another way, writing unit tests is the closest a code author can come to "stepping outside of himself" and reviewing his own code objectively.

After the code is readable enough for the code author to write unit tests, then further benefit comes from getting an objective other -- the code reviewer -- to read it.

Therefore,

1. Write unit tests
2. Get code review

(BTW, this is all if you're writing unit tests. If you have to choose, keep the code review as described, because unreadable code is much more error-prone than readable code that hasn't been tested at the unit or single function or method level. More important testing, that must happen, is at the component and system level. That's because most failures occur when multiple components interact -- unit tests won't find those problems.)

Answer 18

You should be doing the code review first because it forces people to take time to read through code. Unit-tests are automatically run and some people will fall into the trap of thinking "it passes the test so it works". The tests may not be testing the right thing and if they made a mistake, they might not have thoroughly understood their mistake to prevent it from happening again.

Answer 19

My answer: Code review.

Unit tests may be rewritten due to underlying design or functionality change as a result of the code review.

Answer 20

Don't waste the reviewers' time. Unit tests and automated review tools, including static analysis and test coverage, are a cheap way of finding problems, just as the compiler is.