wiki:CodeReviews
Last modified 11 years ago Last modified on 01/29/2008 05:59:10 PM

Using Trac to Manage Code Reviews

Out-of-Date: Upgrade for DC3 Ticket Workflow Installation

The proposed process (See ReviewStatusInTrac for trac support) is influenced by industry practices, and by http://divmod.org/trac/wiki/UltimateQualityDevelopmentSystem.

The life history of a code change is:

  • When new development or a bug fix begins, management opens a ticket labeled appropriately, e.g. "Develop MaskedImage class" and assigns the ticket to a developer.
  • Developer accepts the ticket and notes the ticket number. The ticket status is "notReady".
  • The developer creates a subdirectory named after the ticket number (e.g. "128") under the package "tickets" directory. The developer copies the package trunk (and any other packages he will depend on in other tickets that are not yet in the trunk) into this ticket number directory. If other packages are needed and not yet in the trunk or a ticket, the responsible developer for that other packages can create and populate a "stubs" directory under the branches directory and this can be used by those needing it. So the svn command is something like svn cp file:///scr0/tracrepo/trunk file:///scr0/tracrepo/tickets/34
  • The developer creates and updates the code under this ticket number directory. Note that changes made to source copied from the trunk or other tickets will require careful merging and should be avoided where possible, i.e. in newly developed modules only the new module files should be edited. The developer may copy the code back to the trunk when "it solves more problems than it creates", so that others may use it in builds. However, the status of the ticket will remain "notReady" until the code is ready for review.
  • When code is stable and the corresponding logical model UML is updated in EA (either via forward or reverse engineering), the developer checks it into svn; this will result in a numerical revision number, e.g. "1251". The check in message references the ticket number as "128".
  • This proposed change now exists in svn with a well-defined name. The developer updates the status of the ticket, with a reference to the revision "1251" and assigns the ticket to a reviewer with Review state "needsReview". The developer should ensure that the ticket documentation provides the reviewer with guidance on points to look out for in the review. The reviewer can check out the fixed version or svn switch to it. Trac has tools for visualising the changes accessible via the 1251 revision in the trac ticket. Using the svn revision number rather than the trac ticket is a little unfortunate, but the link includes the full tickets/128 repository link. A key to this part of the process is to move quickly, we want reviews to be done right away and tickets to be short-lived.
  • After reviewing the code and running tests, if the code does not pass review, the reviewer sends the ticket back to the developer with status "notReady", and further changes are made under the ticket number directory and in EA. If the code passes, the reviewer signs off in the ticket, changing the Review state to "reviewed" and reassigning the ticket to the developer.
  • The developer changes the ticket action to "fixed". The developer merges the new code into the trunk.
  • When the package has acquired enough (new) stable functionality, the developer can declare a "release" of the package. This is done by creating a copy of the trunk into a tags subdirectory named after the release version. The first useable version of a package in DC2 will be the DC number, i.e. 2.0 (thus, the code will be found under "DC2/{pkg}/tags/2.0". Subsequent DC2 releases will increment the minor version, e.g. 2.3, 2.14, etc. At this point, the tag subdirectory for this release is made read-only.

Conducting Code Reviews - Details

How Reviewers are Assigned

In our code review procedure, the developer himself assigns a reviewer. In practice, developers have been sending a request to the Project Manager, and he has assigned a reviewer. If individual developers are not comfortable assigning a reviewer, they can continue to request anyone on the CCB to assign one. The assignment is made by assigning the trac ticket to the reviewer and setting the status to needsReview.

Who Should Be Assigned

Everyone who is developing code or designing parts of DC2 should bear some of the burden of these reviews, including part-timers and contractors. This is a significant workload, and we need to spread it around as much as possible.

What Should be Reviewed

There are several items that should be reviewed:

  • Conformance with code standards
  • Consistency of code with UML model
  • Conformance with documentation standards
  • Adequacy of design/implementation versus requirements/scope

SVN Cookbook for Ticket Management

The svn commands to carry out this process are as follows. Assume that we have ticket 666, in repository svn+ssh://svn.lsstcorp.org/DC2/foo. If you don't feel like typing that URL, try svn info | grep URL

Create ticket branch
  • svn cp svn+ssh://svn.lsstcorp.org/DC2/foo/trunk svn+ssh://svn.lsstcorp.org/DC2/foo/tickets/666
Switch to that branch
  • svn switch svn+ssh://svn.lsstcorp.org/DC2/foo/tickets/666
    • This command should be issued from foo's top level directory. You don't just want to switch a subdirectory by mistake
Resolve issue
  • The svn commands for this step are unfortunately unavailable
Check in the changes
  • svn ci -m "Helpful message referring to #666"
    • note down the revision number, say 580205
Notify trac that you are ready for review
  • Edit the ticket,
    • Change Review to needsReview
    • Reassign it to the reviewer; this may well be the person who filed the ticket in the first place
    • Make sure that the Comment field requests a review, refers to [580205], and explains what you did
  • At this point you should stop working on the tickets/666 branch, so now's a good time to svn switch back to the trunk
Reviewer reviews patch, and notifies trac
  • Reviewer edits the ticket,
    • Change Review to reviewed
    • Reassign ticket to the original owner (so that it shows up as her ticket)
Merge changes back to trunk
  • cd foo
    • In top level directory; get this wrong and you'll be confused (I was)
  • svn switch svn+ssh://svn.lsstcorp.org/DC2/foo/trunk
  • svn update
    • Note revision number; say 581225
  • svn log --verbose --stop-on-copy svn+ssh://svn.lsstcorp.org/DC2/foo/tickets/666
    • Note revision number where copy was created; say 570121
  • svn merge -r 570121:581225 svn+ssh://svn.lsstcorp.org/DC2/foo/trunk
    • Run scons. Check that all is well
  • `svn ci -m "Ticket #666; merged -r 570121:581225"
    • Note revision number; say 581231
Close ticket
  • Change Status to closed
  • Include revision [581231] in comment