Code Review

“It’s a simple 3-step process. Step one: Fix! Step two: It! Step three: Fix it!” – Oscar Rogers (Kenan Thompson), Saturday Night Live, 2/2009

Code review is the last line of defense between a mistake that the IDAES team will see and a mistake the whole world will see. In the case of that mistake being a leak of proprietary information, the entire project is jeopardized, so we need to take this process seriously.

Summary

Warning

This section is an incomplete set of notes

Every piece of code must be reviewed by at least two people.

In every case, one of those people will be a designated “gatekeeper” and the one or more others will be “technical reviewers”.

The technical reviewers are expected to consider various aspects of the proposed changes (details below), and engage the author in a discussion on any aspects that are deemed lacking or missing.

The gatekeeper is expected to make sure all criteria have been met, and actually merge the PR.

Assigning Roles

The gatekeeper is a designated person, who will always be added to review a Pull Request (PR)

Gatekeeper is a role that will be one (?) person for some period like a week or two weeks

The role should rotate around the team, it’s expected to be a fair amount of work and should be aligned with availability and paper deadlines, etc.

The originator of the PR will add as reviewers the gatekeeper and 1+ technical reviewers.

Originator responsibilities

The originator of the PR should include in the PR itself information about where to find:

Changes to code/data

Tests of the changes

Documentation of the changes

The originator should be responsive to the reviewers

Technical reviewer responsibilities

The technical reviewer(s) should look at the proposed changes for

Technical correctness (runs properly, good style, internal code documentation, etc.)

Tests

Documentation

No proprietary / sensitive information

Until they approve, the conversation in the PR is between the technical reviewers and the originator (the gatekeeper is not required to participate, assuming they have many PRs to worry about)

Gatekeeper responsibilities

The gatekeeper does not need to engage until there is at least one approving technical review.

Once there is, they should verify that:

Changes do not contain proprietary data

Tests are adequate and do not fail

Documentation is adequate

Once everything is verified, the gatekeeper merges the PR

Automated Checks

The first level of code review is a set of automated checks that must pass before the code is ready for people to review it. These checks will run on the initiation of a pull request and on every new commit to that pull request that is pushed to Github (thus the name “continuous integration”).

The “continuous integration” of the code is hosted by an online service – we use CircleCI – that can automatically rerun the tests after every change (in this case, every new Pull Request or update to the code in an existing Pull Request) and report the results back to Github for display in the web pages. This status information can then be used as an automatic gatekeeper on whether the code can be merged into the master branch – if tests fail, then no merge is allowed. Following this procedure, it is not possible for the master branch to ever be failing its own tests.