Last modified 4 years ago Last modified on 09/08/2015 10:55:46 AM

LSST DM Code Review Process

Last update: 2015-09-08

When to review

Code reviews should happen before every merge to master from a ticket branch. Trivial fixes that happen directly on master generally should not need a review.

How to arrange a review

When development is complete, the developer should request a review. This is done by selecting In Review from the Workflow menu of the JIRA ticket. See "Who are the reviewers?" for hints on picking a suitable reviewer.

If the assigned reviewer does not have time to review the code within a few days, the reviewer should notify the deloper as soon as possible, so the developer has a chance to assign the review to somebody else or agree wait. However, the developer is also expected to keep an eye on the review and ping the reviewer if necessary. Two filters can be helpful here:

  • Issues I'm Reviewing: Reviewers in (currentUser()) AND status = "In Review"
  • My Issues In Review: assignee = currentUser() AND status = "In Review"

Goals of a review

The primary goals of a code review are to answer these questions:

  1. Does the code satisfy the design requirements?
    1. Explicit requirements for functionality
    2. Non-functional requirements like memory usage, performance, etc.
  2. Is the code maintainable?
    1. is it understandable by a reasonable programmer?
    2. is it adequately documented?
    3. does it have adequate unit tests?
    4. does it follow our coding standards?
  3. Have any backward-incompatible changes been announced to lsst-data?

These goals do not include nit-picking over choice of variable names :-).

Who are the reviewers?

Code reviews performed by peers are useful for a number of reasons besides the primary goals above:

  1. Peers are a good proxy for maintainability.
  2. It's useful for everyone to be familiar with other parts of the system.
  3. Good practices can be spread; bad practices can be deprecated.

Primary reviewers should thus be chosen from the set of active developers on the project and may (often) cross the Middleware/Apps? line. All developers are expected to make time to perform reviews; the System Architect will ensure that no developer is overburdened with this responsibility.

For major changes, it is good to choose someone more experienced than the author. For minor changes, it may be good to choose someone less experienced than the author. For large changes, more than one reviewer may be assigned, possibly split by area of the code.

The review

The reviewer examines the changes and makes comments in the "Files changed" tab of a GitHub pull request or, if small, in the original JIRA issue. The reviewer then summarizes the results of the review in the JIRA issue.

If the reviewer sees a problem that might be out of scope for this ticket, ask the developer to either fix the problem or file a new ticket. For a problem that is obviously out of scope, the reviewer should file a new ticket.

When the review is done the reviewer puts the ticket into the state "Reviewed". The developer will then make appropriate changes and rebase from master (if master has changed).

At this point the developer usually merges the ticket to master (without checking back with the reviewer) and marks the ticket as Done. However, if the changes are sufficiently large, the reviewer or developer may request that the reviewer take a look at the cleaned up code before it is merged to master.