10

How much should a reviewer trust the coder who submits the code for review?

I always have that dilemma of whether I should go test of the changes proposed or should I trust the submitter that he would be knowing what he is doing? Mainly because at the end of the day its reviewers responsibility also.

Code-reviews are not fun always.

Improve this question
5
  • No unit test results? Why not?
    – S.Lott
    Jul 12, 2011 at 18:44
  • @S.Lott: Good hint. But thats again pretty tricky. I (being a reviewer) might not know what all test-cases are needed to cover all possible corner-cases.
    – hari
    Jul 12, 2011 at 18:49
  • 1
    It's not "tricky". Unless you're God, you can never know if all test-cases are needed to cover all possible corner-cases. Since you can never know this, it's not "tricky".
    – S.Lott
    Jul 12, 2011 at 18:51
  • If the reviewer asks for his code to be reviewed, by all means, test it!
    – rlb.usa
    Jul 12, 2011 at 19:08
  • Remain impartial. You wouldn't want testers not to test just because of some developers rep to always get it right and then miss something important that blows up in your most important client's face. So test, but don't take on the testing responsibilities of the programmer nor those of the testing team. When doing reviews I run a couple of cases to ensure that the testers/QA do not have to send it straight back to development, but the programmer remains responsible for being confident that the testers/QA won't find anything.
    – Marjan Venema
    Jul 13, 2011 at 7:14

6 Answers 6

Reset to default
20

No, you don't need to trust them. You were asked to review the code do so, it is not the same as testing it:

  • Is the code clear and easy to understand? (This is my primary concern.)
  • Does it comply with site standards?
  • Does it comply with best practices?
  • Does it do what it needs to do, and only that?
  • Are there edge cases which are easily caught by code inspection? (Off by one, incorrect limits, missing default case, unintended or undocumented case fall-through, etc.)
  • Are there ways it could have been better written?
    • Can you enlighten the developer on better approaches to use on future work?
    • If the code works, it may not be appropriate to apply the changes to the current code?
  • Did they do anything that caught your eye as interesting?
  • Anything else covered by your review standards?
  • Do note things they did well in addition to problems?
Improve this answer
4
  • 3
    Excellent answer. Put another way, the purpose of code reviews is to catch the things that testing can't catch.
    – Karl Bielefeldt
    Jul 12, 2011 at 21:38
  • @BillThor: Thanks. I strongly disagree to the point that reviewer should thoroughly test the code. It's not and should not be the job of a reviewer. At the best, I can ask the submitter to run a few test (which I find useful and yet not covered) and attach the results.
    – hari
    Jul 12, 2011 at 22:45
  • @hari, no the reviewer should not thoroughly test the code, that is the job of the programmer. However I do agree that the reviewer should at least run a couple of test cases, common scenario's, to ensure that when the code is passed to QA for testing/verification, it doesn't come straight back...
    – Marjan Venema
    Jul 13, 2011 at 7:08
  • If by "thoroughly test" you mean write additional tests, then no, the reviewer ought to fail the code instead, suggesting additional test cases. If by "thoroughly test" you mean the reviewer ought to run all the tests, and be reasonably sure that there's decent coverage, then yes, the reviewer should "thoroughly test" things.
    – Frank Shearar
    Jul 13, 2011 at 8:00
5

If you don't trust the person to test their code, what really can you trust them with? If you can't trust them to do that, do you honestly think that you can trust them to do any post-code review follow ups?

Sure, one can say that just because they tested it, does not mean that they tested it well. But that leads to a different question--do you trust them to test their code well?

If you have concerns about how well it was tested, think up all the failure paths you can think of, and all the corner cases and keep an eye out for those in the code review, or during your pre-code review inspection (make notes). If you have concerns about the cases you have thought up, ask about them. If they can not be answered to satisfaction at the session, then the developer must follow up on them; make sure to be copied on the results of the investigation.

Improve this answer
2

Code review should be conducted against a set of measurable criteria. I don't think trust comes into it. The source code and accompanying documentation should be open. The questions to the coder should be frank as well.

If the attitude of the code review and the participant is not open, frank and without egos, then it is not a true code review.

Improve this answer
0
0

It depends wholly on your organization's procedures and what process control measures you have in place. If, for example, you employ unit testing, then you could check the total code coverage as a measure of how much to "trust" the code.

Truly, there are far too many factors to weigh, many of which are highly subjective, for there to be any hard and fast rule for this. I definitely take into account who is checking in code - I'm much less nitpicky with code checked into by a veteran than I am with a fresh college kid.

Improve this answer
0

Depends on the size of the code changes to my mind:

Extreme 1: Small changes - If there is a bug being fixed by changing a few lines of code, then I'd generally like to show a before and after execution of the code just as a standard part of the review. This could be taken as paranoia on my end, but it just makes sense to do this usually.

Extreme 2: Big changes - If there are dozens of new classes being added, then it may be a few hours just to go over the design so that demonstrating all the functionality isn't a realistic thing to do. In this case there is some trust in getting the finer details right as well as some trust that the higher level details can be communicated in a somewhat concise manner as I don't want to have to go read every line that was added.

In between is where 99% of code changes are made and there is a bit of a judgment call as to where the line is. Generally I like to see some working cases with a few boundary cases if it is realistic to do a basic smoke test of the new functionality.

Improve this answer
0

Changes to the tests should be part of the same review as changes to the system under test. If they can't be, for organisational reasons (e.g. you're using a tool that works on only one repository at a time, and your tests are not in the same repository as your functional code), then the two reviews should be linked in some way. This might be as simple as an email or a conversation saying which reviews belong together.

In the event that some of your tests are not automated, the review should include at least a statement of the tests conducted.

In all cases, an important part of the review is to identify gaps in the testing, and I always look for edge cases that have been missed and might cause problems for the code.

When reviewing, I don't normally repeat the tests, but would consider doing so if the author asked (e.g. if we're compiling for different platforms, or to confirm that the diff is complete - though usually, the build server will tell us that). It is not a good use of my time to duplicate someone else's actions without good reason.

Improve this answer

Not the answer you're looking for? Browse other questions tagged or ask your own question.