wiki:CodeReviewProcedure

Version 11 (modified by jreed, 8 years ago) (diff)

in review reply mention to copy and paste checklist (as discussed in conference call)

Overview

All BIND 10 code must be reviewed by another coder on the BIND 10 team before it may be merged into the main trunk.

Typically a piece of code will be created in a branch, and worked on until it is thought to be ready by the coder working on it. When this happens it is reviewed, as described below.

Starting the Review Process

When a coder has a piece of code that is "done", then they must:

  1. Create a ticket on the Trac site describing the code that is to be reviewed. This ticket should include the word "review" in the title, and the branch name, source branch and revision number at which it was branched in the description.
  2. Announce that the code is ready for review.

The announcement can be done either by sending something to the development mailing list, bind10-dev@…, or by mailing the Programme Manager, shane@….

The coder can ask someone to become the reviewer, or ask the Programme Manager to pick someone to review the code. If the Programme Manager does this, he or she will send an introduction e-mail including both the coder and the reviewer. It is not necessary to copy the Programme Manager on any follow-up mails between the coder and reviewer.

Reviewing the Code

When the reviewer is ready for the code, he or she takes ownership of the ticket on the Trac site.

The code should be reviewed to make sure it is clear, correct, and follows BIND 10 coding guidelines.

A short checklist:

  • General
    1. The code must have Documentation (for the coder or user as appropriate)
    2. The CodingGuidelines must be followed
    3. There must be automated unit tests
    4. Installation should work
  • For API's
    1. Does the API make sense?
    2. Are there functions missing?
    3. Are there functions unnecessary?
    4. Does the documentation specify exceptions and memory allocations?
  • For code, read through the source code and try to find anything that might 'not work' or is not understandable. Some hints:
    1. Are there any internally undocumented weird steps performed?
    2. Are there hard-coded constants that should have a defined name?
    3. Are design choices written down?
    4. Are branches, switch statements and loop conditions correct?

Anything about the code is fair game, as long as it fits in the BIND 10 style and is intended to make the software better.

Any questions or concerns should be added to the ticket, and then ownership passed back to the coder.

Iterating

The ticket may pass back and forth between the coder and the reviewer several times as it is discussed. This is quite normal, especially for large and/or complicated tickets.

Comments about the review should be added to the ticket.

Ticket ownership matters! A reviewer may make several comments, especially in a long review process. The signal to the coder that a given review pass is complete is getting ownership of the ticket back. Likewise when the coder thinks that all of the issues have been addressed, the signal to the reviewer is getting ownership of the ticket back. When the review is complete, copy-and-paste the above checklist in the reply and note "YES" for each item checked or explain why not checked or about any issues.

If problems arise in the process - for example due to unclear guidelines about how things should work, or disagreements about what the best way forward in a given case is - then the issue can be brought to the mailing list or to the Programme Manager.

Completing the Review Process

When all issues have been resolved, the reviewer adds an entry to the ticket noting that, and passes ownership back to the coder.

The code is then ready for merge. Branching and merging procedures can be found in the Subversion handbook chapter 4. The ChangeLog file is updated. After this merge is complete, the ticket may be closed. Depending on the scope of the change, either a congratulatory cup of caffeinated liquid or a bubbly alcoholic beverage may be imbibed.

Deadlines

A reviewer will be assigned within 5 working days of the coder asking for review.

The reviewer should provide feedback within 5 working days of receiving the review. For a complex review, the review may take longer than 5 days, but the coder needs to know that someone is working on it.

About this Document

Created by Shane 2009-12-18
Review scheduled 2010-06-18