Code Review Process
From OpenLaszlo
This document describes the change process that is used by individuals with commit privileges to the source database. Over time, we hope to unify it with the process that is used for contributions that use the contribution process. For now, it is published here in the interests of greater transparency, and as a step towards this unification.
Contents |
Overview
Code review is the process by which code is authorized to enter the development tree. We use code review to keep defects from entering the development tree, and to assure that we're constructing a product that matches the requirements.
The goal of the code review process is to save time, by catching defects before they affect others and before they get lost in the sea. Defects are functional problems (bugs), maintenance problems such as fragile or incomprehensible code, and traceability problems such as unnecessary features or features that don't match the requirements.
The goals of this document are to describe the process that we actually use to make changes to the development tree, and to make this process consistent. It's a starting point, not a destination: like a piece of software, we will debug it and optimize it as we gain experience as a team. It does derive from code review processes we've used successfully in the past, so it's known to work in principle.
Roles
Author
The author is the producer of the product under review. The author provides the required materials to the reviewer prior to the review session. The author leads the reviewer through the product in a logical fashion, paraphrasing and summarizing sections. The author responds to questions about the function, purpose, and organization of the material posed during the review session.. The author's ultimate responsibility is to correct the defects found during the review.
Reviewer
The reviewer's job is to look for defects in the product. The reviewer studies the product provided by the author in relation to other prior artifacts. The reviewer must look at both the implementation details for faults, and at the design from which the implementation was based. It is a fault when the implementation does not meet the specification provided in the design. A reviewer can be any member of the community with committer rights who has expertise in the area under review.
Process
Request (Author)
The Author packages a set of materials for the Reviewer to review:
- Familiarize yourself with the svn change package tools described in SubmitAChangeForReview
- Verify that your changes adhere to the Code Guidelines. Check them against the Code Review Checklist.
- Write test cases for your changes. If they implement a new feature, try to encapsulate the cases that you used to assure yourself that it worked into test cases. (Hopefully, these will include boundary conditions and error reporting, and cover all code branches.) If they fix a defect, the test case is a case that displays the defect in the unfixed code. Add your test cases to the build file.
- Update the documentation so that it matches the post-change functionality of the system. Add documentation for new features. The changed/new documentation becomes part of the change list.
- Write a description of your changes. This should include an overview of the code changes, the names of implemented features or tasks, and the number and/or descriptions of defects. There's a template here, or use the bash script svn-makechangepackage, available in svn-bash.sh.
- Synchronize with the latest sources, and verify that your code passes the smoke test.
- Email a review package to the reviewers, built using svn-makechangepackage (see below) and the bash script svn-review (also available in svn-bash.sh). Be sure to CC the laszlo-dev mailing list.
- The reviewer can either view your change package, or (if possible) sit down at your computer to review your changes directly. If the latter, it is important that the reviewer control the mouse; too many regressions are missed if the reviewer doesn't control the screen.
- If the change fixes a bug, put the bug in In Review stage and assign it to the reviewer.
You can use the svn-makechangepackage tool to create a review package:
- Edit the files in your svn tree.
- Put your release notes in the change set description.
- Execute the shell command svn-makechangepackage, which creates a file name starting with patch...
- Mail the patch file to the reviewer and CC laszlo-dev
Review (Reviewer)
The Reviewer examines the changes:
- Read the change notes. Verify that the tasks are appropriate for the current milestone, and that you understand the descriptions of the feature that the each task implements, and of the defects that the change set fixes.
- Use svn diff (or its friendlier front ends) to examine the changes. Verify that changes and new files adhere to the coding guidelines. Verify that each change implements a task or fix that's described in the release notes. Verify that you understand each change or new file, and how it implements the feature. If there's a design document, check the code changes against the design, and design changes against the specification. If there are specification changes, check them against the requirements.
- Verify that test cases cover new features and functions that are likely to be broken or to break.
- Compile a list of defects and missing test cases, if any, and send it to the Author. (This should happen on a piece of paper of by email, even if you're going over the code together.) Sort defects into a list that must be fixed, and a list that you would like to see fixed.
Note: A confusing though correct piece of code can be in either category, depending on how likely it is to cause subsequent maintenance problems.
The reviewer does not need to test that the code compiles, or verify that it runs the test cases. The reviewer's job is to find defects that can't be found automatically.
Response (Author)
- Fix any "must fix" defects. (Confusing or unclear code can often be fixed by factoring it, documenting it, or adding design documentation.)
- Add notes about remaining defects as source code comments, design document issues, or Gadfly issues.
- Update the change notes.
- If any code was changed, or any documentation was substantively changed, start over at the Request task. Otherwise go on to Submit.
Submit (Author)
- The author submits the change list, with the change notes that were used during the review process.
- The author Resolves any bugs associated with the change.
