Group Code Reviews

Owned by Michael Downey
Last updated: Aug 14, 2012 by Ben WolfeVersion comment
2 min read

Overview

In the past, OpenMRS developers conducted real-time group code review sessions. This is now generally discontinued in favor of Asynchronous Code Reviews. See the main Code Review wiki page for a description of the preferred method of reviewing code.

If you would like a group code review, email the developers mailing list to request one.

Triggering a Code Review

The weekly code reviews are driven by the Road Map and all issues that are in Code Review status. Changesets that have been committed by any developer to trunk should get a new code review opened in Crucible.

The issue acts as a formal request for code review by a developer or some other party. Issues should only be marked as "Code Review (Pre-Commit)" once the branch/patch author's code is:

  • Well commented
  • Unit tested
  • Formatted correctly

For more information, see the checkstyle ant target and OpenMRS Conventions .

Process

  1. Each week, code to be reviewed is selected before the meeting.
  2. The meeting moderator shares his or her desktop using WebHuddle, vnc, Vyew, or OpenMRS Connect.
  3. The code review is conducted using the Code Review Checklist.
  4. The code review moderator should have a code compare open:
    1. Check out the latest branch into a project
    2. Check out the most recent trunk for the last time that branch was merged to
    3. Select root folder for both projects and right click --> Compare with --> Each Other
  5. Notes about all that is discussed are written into the notes page for that day.

Tips

  • Take care not to let the code review turn into a hack-a-thon.
  • The meeting moderator should make sure that the review stays on task.
  • The code review should focus on detecting and recording, not fixing bugs.

After Code Review

  • Any comments or code changes made during the code review should be committed. These changes should be minor only.
  • The TODO items in the notes should be turned into tickets.
  • The "parent" ticket's "Code Review Status" should be changed to "Reviewed"
  • The "parent" ticket should link to each ticket created by the code review
  • Once all "subtickets" have been completed, the "Code Review Status" should be changed to "Needs Review" again
  • This process continues until the ticket is marked as "Approved" and then either a core developer or the patch author can commit it to trunk.