Code Review Process

Objective

The idea is to maintain code quality across the board: make sure that the architecture is sound and that the code quality practices are followed. It's also an opportunity to spread knowledge and communication across teams, hear new ideas and learn new techniques.

Scope

The review process described here is intended for a small core team (25 or so contributors), where everyone knows everyone and where everyone is pretty much contributing 100% of their time to the project. It covers both the needs of devs with commit privileges and those without such privileges.

This process however is not intended to scale to a very large team (50+ contributors) and certainly not to a large number of occasional contributors. We'll need another, more rigourous process by then (likely inspired from Mozilla's SR process). To be honest, we can't wait to have this problem to solve... :)

Code Review Processes

We distinguish between 3 kinds of code reviews:

• Review of new features code: significant chunks of code implementing a spec or a part of it.
• Review of critical patches code: stuff we do in the end cycle or on branches (Demos, milestones) and where no regression is a requirement.
• Review of external submission patches: stuff (big or small) submitted by devs with no commit privileges to the SVN repository.

Review of new features code

When

One for every 2 to 4 weeks of coding work in close association with the Design Doc (in the spec) writing.

Up to the dev to setup the review. Considering we all work on one or two "areas" and that we have one big rework (sliced in a bunch of tasks...) per milestone, 1 review per milestone should be fine, may be 2 in some cases (like say one on infrastructure stuff and one on use of infrastructure in some feature implementation).

Rule of thumb: don't wait to be 100% finished to call for a code review. If a significant issue is raised and requires a rewrite, the longer you wait, the more code you'll have to throw (and the worse you'll feel...).

Also, for this kind of review, don't wait for the review to commit your code: we expect you to commit small chunks during those 2 to 4 weeks. The review is here to validate a solution and ensure code quality.

What

Review focus on architecture. Areas to keep in mind during review (not exhaustive):

• soundness of the solution
• adequacy to the spec
• consistency with the rest of the app
• scalability of the solution
• maintainability of the code
• security review
• error / exception handling
• accessibility (of UI, if any)
• internationalization readiness
• coding conventions
• other best practices

Who

• 1 dev from the same team (peer)
• 1 dev from another team (peer)

How

1 or 2 hours meeting between the members of the review committee. Send the code to review and links to docs (spec, design docs) ahead of time (several days at least).

Review of critical patches

When

Code review of bug fixes are enforced for critical bugs on branches (demos, milestones) and for all bugs when triage (bug council) is enforced (end of cycle).

Outside those situations, a dev can (and should) always ask for a code review when a bug fix is going beyond a simple fix (refactoring, code overhaul, etc...). Currently, this judgement call is left up to the dev (if s/he would like an expert advice on a particular fix before commiting the fix).

Those reviews must be held before the code is commited to SVN.

What

Code review focus on code stability and regression risks: the same areas as above can be probed and commented by reviewers of course but the review of a bug fix will squarely focus on stability / risk of regression (in other areas).

Who

• 1 dev from the same team or not (peer)

How

• The Bug Council will first triage the bug (using milestone fields and blocking flags). All bug triaged that way must be code reviewed before being commited to SVN.
• When you have a fix, use the Attachment field in Bugzilla to attach the patch and ask for a review.
• Wait for the reviewer's approval before commiting any code.
• As soon as the code is approved in Bugzilla (review:+). commit the code in SVN and resolve the bug fixed.

Review of external submission patches

When

If you do not have check in privileges in the Chandler SVN repository, you need to follow this procedure so that your code can be integrated in Chandler.

This process doesn't make the difference between small and large submissions and doesn't try to enforce much rules. We are not experiencing a lot of such submission for the moment so this light process works for us. Would we be flooded by patches from the Chandler community, we will revise this process as appropriate.

What

The scope of the review is, depending of the patch submitted, a combinaison of both the critical patch and the new feature review scope.

The reviewer will pay particular attention to security and maintainability.

Who

• 1 dev, owner of the code area who has commit privileges (reviewer)

How

• Contact a person who currently has commit privileges on SVN and ask him/her if s/he'd consider committing a patch for you (make your case as to why we should consider that patch...). If you don't know who to contact, send an e-mail to the OSAF dev list.
• Create a bug in Bugzilla (if one does not already exist) covering your patch intent.
• Attach your patch to the Bugzilla record.
• Request a review through the Bugzilla review process and ask the reviewer to commit the changes once the review is completed.
• The reviewer reviews the code and follows the same procedures as for a critical patch (here above described).

Links

• Mozilla.org's Code Review Process
• OSAF Release Management with Bugzilla