PreMergeReviews

Not logged in - Log In / Register

Revision 1 as of 2009-05-25 07:30:24

Clear message

<<TableOfContents: execution failed [Too many arguments] (see also the log)>>

Introduction

As of April, 2004, the Launchpad code has been placed under mandatory pre-merge review. Read this document carefully for a description of the review process and what it entails.

As of 2009, we use Launchpad Code Merge Proposals to manage the queue of work.

What are Pre-Merge Reviews?

Pre-merge reviews are peer reviews of source code modifications. They are a great tool for ensuring quality of the resulting product, and also for communicating technique and style between team members. A good review process ensures that everybody learns at a much faster rate, and creates a valuable technical relationship between reviewer and coder.

Pre-Implementation Call

As of June 2006, we encourage all developers to arrange pre-implementation design reviews with another developer, preferably by phone call. IRC can be used if a phone call is not possible, but voice contact is more desirable.

Pre-implementation calls should be used before starting to implement any feature or bug. For more details on the work flow and what is expected from both callers and callees, see DesignPhoneCall.

The PendingReviews template has been updated to include a line where you will be expected to add your pre-implementation call buddy's name, and your reviewer will ask you whether you've had a pre-implementation review. The expected answer is "yes". :)

Working with PreMergeReviews

For pre-merge reviews to be effective, however, small modifications to the development workflow are required. The main modification is, of course, requesting peer review of your code changes before merging your code into RocketFuel. The sections below go into the details of how this is best performed. There is a crib sheet for getting a branch merged on the WorkingWithReviews page.

Branch size limits

As of 12-Dec-2007 a limit of 800 lines per branch diff has been set as a maximum. If your branch grows above this limit you must negotiate with a reviewer to accept the oversized branch. Note that very large branches are not recommended for on-call reviews as they slow down the work of the reviewer who is trying interactively review a high volume of branches during the shift.

What are reviewers looking for ?

Reviewers are checking both the design of your branch and the quality of the code in it. The best way to understand what a review is looking for is to read TipsForReviewers. (You can also use the CodeReviewChecklist cheat-sheet to do a self-review.)

If in doubt, use a branch

Any non-trivial changes for review must be on a branch. The goal is to make it easy to look at which changes have been made without spurious diffs, and without having to worry about RocketFuel changing from under your feet. A good rule of thumb is:

If what you're working on is complex enough that you'll likely be making several commits, you definitely need a branch.

Trivial changes

If the changes are simple, it may be appropriate to request a review by sending a diff directly to a reviewer. If you do this:

You are responsible for your branch

The central rule that is used in the Launchpad review process is:

This means that you, as a coder, are reponsible for

  1. writing the code
  2. requesting review (and editing PendingReviews)

  3. finding a reviewer (if the process breaks down - normally this is managed by the review team lead)
  4. reminding/nagging/pursuing/coordinating with your reviewer
  5. dealing with review comments / requests
  6. landing your change (cleaning up PendingReviews once that is done).

The reviewer, conversely, is responsible only for two important things:

The reviewer is the obvious bottleneck in the process, and if you can make his life easier, please do. This includes making smaller changes, coordinating with other developers to order reviews to minimize conflicts, and reminding of forgotten branches periodically.

Requesting review

Before you request a review, you should make sure that make check_merge reports no errors after having first merged rocketfuel[1], as fixing any broken tests after a review might involve an unnecessary additional review which could have been avoided.

The entry in the reviews page and the email sent should contain a summary of the changes, the name of the branch they're on, and URLs of any bugs that they fix.

[1] As of 2007-03-15 it's been confirmed that only PQM gets through all the tests unscathed. This could be because PQM runs on Dapper, and most developers are running Edgy or Feisty, and the test suite hasn't been fixed to accommodate these.

Dealing with reviews

A thorough guide to dealing with reviews and finally merging your changes is available at WorkingWithReviews.

The Launchpad-Reviews mailing list

We have a review-specific mailing list: launchpad-reviews@lists.canonical.com. The mailman page for it is at http://lists.canonical.com/mailman/listinfo/launchpad-reviews

This mailing list should concentrate review activity on the mailing list, including both review requests (see below) and reviewer comments and replies. The following guidelines apply to it:

Official Reviewers

Currently, the official review team is:

Review requests placed on them are visible at PendingReviews. If you are a reviewer you should definately read TipsForReviewers.

For smaller patches, you _may_ at your discretion seek out another peer to serve as your reviewer. Use your judgement in determining whether the patch you are working on is small enough to not require an official reviewer. Patches that are not reviewed by an official review must be rubber-stamp or trivial patches.

See also

Future Plans

Annotated diffs?

-- AndrewBennetts.

Better display of moved code?

In a plain diff, moved code shows up as a large delete and add (or a large add and delete). It would make the reviewer's life a bit simpler if somehow large chunks of code that are both removed and added were obviously shown as moves, so the reviewer knows they don't need to pay as much attention to it as new code. Perhaps this is more in the realm of a graphical tool like meld than a diff on a web page.

-- AndrewBennetts.

Tests/documentation first in diffs

Additions and changes to tests and documentation help explain what the code changes in a patch are for. It would be nice if, say files with "ftests", "tests", or "doc" in their paths appeared before others in a diff, so that reviewers can easily read them first.

-- AndrewBennetts 2006-05-01 11:51:27