Diff for "PreMergeReviews"

Not logged in - Log In / Register

Differences between revisions 1 and 8 (spanning 7 versions)
Revision 1 as of 2009-05-25 07:30:24
Size: 9893
Editor: sinzui
Comment:
Revision 8 as of 2010-02-19 21:20:11
Size: 4854
Editor: flacoste
Comment: typo
Deletions are marked like this. Additions are marked like this.
Line 5: Line 5:
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. 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.  We use Launchpad Code Merge Proposals to manage the
queue of work.
Line 7: Line 10:
As of 2009, we use Launchpad Code Merge Proposals to manage the queue of work.
Line 11: Line 13:
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-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.
Line 15: Line 22:
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. We '''require''' all comunity developers to arrange pre-implementation
design reviews with a Launchpad staff developer from the relevant team
or with significant domain knowledge. Pre-implementation reviews should
preferably happen by phone call but IRC can be used if a phone call is
not possible. Launchpad staff developers are also encouraged to have
pre-implementation reviews with another staff developer.
Line 17: Line 29:
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. Pre-implementation calls should be used before starting to implement any
feature or fix any
bug. For more details on the work flow and what is
expected from both callers and callees, see DesignPhoneCall.
Line 19: Line 33:
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". :)
Line 23: Line 36:
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. 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.
Line 27: Line 46:
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. 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.
Line 29: Line 53:
== What are reviewers looking for ? ==
Line 31: Line 54:
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.) == 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.) Reviewers will also check that your
implementation has been discussed with a Launchpad staff developer in a
pre-implementation call.
Line 35: Line 66:
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:  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:
Line 37: Line 71:
If what you're working on is complex enough that you'll likely be making several commits, you definitely need a branch. If what you're working on is complex enough that you'll likely be making
several commits, you definitely need a branch.
Line 39: Line 74:
== 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:

 * arrange with the reviewer first, perhaps on IRC
 * use the `-p` option to diff to get reasonable amounts of context
 * CC launchpad-reviews@lists.canonical.com
Line 51: Line 79:
  '''The coder is responsible for getting his branch through the review process'''   '''The coder is responsible for getting his branch through the review
process'''
Line 53: Line 82:
This means that you, as a coder, are reponsible for This means that you, as a coder, are responsible for
Line 56: Line 85:
  1. requesting review (and editing PendingReviews)
  1. finding a reviewer (if the process breaks down - normally this is managed by the review team lead)
  1. reminding/nagging/pursuing/coordinating with your reviewer
  1. dealing with review comments / requests
  1. landing your change (cleaning up PendingReviews once that is done).
  1. requesting review (use Launchpad Merge Proposals)
  1. finding a reviewer (ask an on-call reviewer on #launchpad-reviews,
  or arrange with another reviewer)
  1. dealing with review comments / requests (Reply to all queries)
  1. landing your change (You may land when the proposal is Approved)
Line 64: Line 93:
  * Reviewing requests in a timely manner.   * Reviewing requests in a timely manner, (respond to requests on
  #launchpad-reviews, )
.
Line 66: Line 96:
  * Be clear about the changes you expect to see
  * Set the status of your review, and if you are the final reviewer,
  set the status of the merge proposal.
Line 67: Line 100:
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. 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.
Line 71: Line 108:
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. 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.
Line 73: Line 113:
  '''Reviews should be requested by adding a tagged and dated branch name to the PendingReviews page, under the section for your favorite reviewer.'''

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.
Line 81: Line 116:
A thorough guide to dealing with reviews and finally merging your changes is available at WorkingWithReviews. A thorough guide to dealing with reviews and finally merging your
changes is available at WorkingWithReviews.
Line 83: Line 119:
== 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:

 * The launchpad-reviews mailing list is open to any team member that would like to subscribe to it.

  '''Reviewers /must/ subscribe to the mailing list (in order to receive notifications, for one).'''

 * As stated above, review requests should be added to the PendingReviews page, which the the mailing list is subscribed to (using the LaunchpadReviews wiki user).
  
 There is no need to send email to launchpad-reviews with your review request if you add your branch to PendingReviews.

 * Keep replies to launchpad-reviews mail CCed to launchpad-reviews. You should only move the thread to launchpad-list if you:

    * Identify a topic that is worthy of launchpad attention (setting new policy is included in this group)
    * Change the subject to a meaningful indication of the topic

 * The reviewers team will make a summary of the important review issues of the week and send this out to the launchpad mailing list.

= Official Reviewers =

Currently, the official review team is:

  * RobertCollins (team lead)
  * SteveAlexander
  * AndrewBennetts
  * JamesHenstridge
  * FrancisLacoste
  * GuilhermeSalgado
  * BjornTillenius
  * BarryWarsaw
  * TimPenhey
  * ChristianReis
  * ElliotMurphy
  * BradCrittenden
  * MatthewPaulThomas (User Interface)
  * StuartBishop (Database)

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.
Line 132: Line 125:

= Future Plans =

 This section is for documenting future plans that are not well formed enough for specs yet.
 {{{
21:08 < spiv> I like it, although I'm starting to itch for slightly more workflow, so that we'd show "days since last touched" or something ;
21:09 < lifeless> I'd love to have stats on average time to merge a branch
21:09 < lifeless> time to review
21:09 < lifeless> time to reply
21:10 < BjornT> lifeless, jamesh: it would be nice to have a quick way of getting the description of the branch as well.
}}}

== Annotated diffs? ==

 When reviewing a diff of a very large change that presumably developed over many revisions, I find myself wondering "why did they change this?" In some cases, a comment could be added to the code that was added or changed. But that doesn't make sense for all modifications -- for instance, deleting files or code isn't something you can sensibly leave comments in for.

 If the HTML of the diff had something like the output of "`bzr annotate`" for the changed lines, that might help. Ideally I'd want to see what revision deleted a line, as well as which one added it, and what the commit message was. Tooltips would be a tolerable way to display this information. (I don't want the ability to trivially cut-and-paste diffs into my review mails impeded.)

''-- 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 <<DateTime(2006-05-01T11:51:27Z)>>''

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

Introduction

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. 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

We require all comunity developers to arrange pre-implementation design reviews with a Launchpad staff developer from the relevant team or with significant domain knowledge. Pre-implementation reviews should preferably happen by phone call but IRC can be used if a phone call is not possible. Launchpad staff developers are also encouraged to have pre-implementation reviews with another staff developer.

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

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

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.) Reviewers will also check that your implementation has been discussed with a Launchpad staff developer in a pre-implementation call.

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.

You are responsible for your branch

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

  • The coder is responsible for getting his branch through the review

process

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

  1. writing the code
  2. requesting review (use Launchpad Merge Proposals)
  3. finding a reviewer (ask an on-call reviewer on #launchpad-reviews, or arrange with another reviewer)
  4. dealing with review comments / requests (Reply to all queries)
  5. landing your change (You may land when the proposal is Approved)

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

  • Reviewing requests in a timely manner, (respond to requests on #launchpad-reviews, ).
  • Participating in follow-up discussion once a review is sent.
  • Be clear about the changes you expect to see
  • Set the status of your review, and if you are the final reviewer, set the status of the merge proposal.

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.

Dealing with reviews

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

See also

PreMergeReviews (last edited 2021-11-11 10:00:34 by cjwatson)