Code Reviews
UPDATE: Post-commit reviews are no longer allowed.
Code reviews exist to do three principal things:
- Catch bugs (both trivial and deep problems)
- Educate team members (prevent silos from forming)
- Cause developers to think it through (performance, security, cleanliness, etc)
Catching trivial bugs is often less effective in a code review. With tools like CheckStyle and FindBugs and Xlint we can automate part of the process and get better (and more consistent) results than we will with human code review. Every minute a developer is spending doing something an automated tool can do, is time lost. Trivial changes such as formatting can be controlled by standardizing on a formatter (e.g., the NetBeans formatter works well and also supports removing trailing witespaces). All code that is checked in must be formatted in a single standard manner.
For experienced code owners, very deep bugs are sometimes found when the reviewer reviews the code. Developers must not commit and push code without first having the changes reviewed. This is a recipe for releasing code with prints and weird comments. The most common type of review is to create a webrev, publish it on http://cr.openjdk.java.net/, and post a request for review in JBS and on the mailing list. Another method for code review that is effective is for the developer and the reviewer to browse the code together and commit it when they are both satisfied. This gives the owner a chance to go over the work and it is well known that explaining something to someone else causes a deeper understanding for both. The review is interactive. If a trivial problem is found, it is fixed by both people and the review continues. If a deep but non-fatal issue is found, a follow on bug report can be entered and the review continues. If a fatal flaw is found or a flaw that will take some time to fix, the review ends and the developer fixes the problem. Code reviews like this are typical for major features or very hard problems.
Code Review Policy
The code review policy for OpenFX is this:
- Every change must be reviewed by at least one reviewer before it is pushed to the repo; complicated changes should have a second reviewer
- Each code area has an owner and at least one reviewer
- The owner has final say in what goes in to his area (subject to project leader override in rare cases)
- The reviewer is the primary go-to developer whenever the owner needs a review
- The owner should review all change sets which go into the code area, but can do so after the commit as long as a reviewer has approved it before it is committed
- For complicated or risky reviews, the reviewers should wait a reasonable amount of time before approving to give other interested parties who might be in other time zones a chance to spot problems
It is not necessary that every developer is owner of something. Not being an owner doesn't mean that you have no responsibility – perhaps your work is cross cutting and affects many different areas (like mnemonics or accessibility). In these cases you may own some small part but need to get reviews for the code that touches other owner's areas. Or maybe you are an architect and own very little but work all over the platform. In such cases you may not be an owner for the places where you are principally working.
A fundamental idea behind this strategy is that the owner and reviewers must have a shared model of how the code is changing, the bugs that are being fixed and who is making changes. Neither the owner nor the reviewer should be surprised by a code change.
IMPORTANT: Do not reformat code as part of a bug fix. The makes more changes for code reviewers to track and review, If you want to reformat a class, make a changeset that has only formatting changes. Do not reformat code that you do not own.
No post-commit reviews
A former policy of the OpenJFX Project used to allow post-commit reviews in some cases. As the OpenJFX project becomes more mature, and as we move closer to integration with the rest of the JDK, including the use of jcheck, we are eliminating the notion of post-commit reviews. All reviews must happen before the code is committed and pushed.
Technical Discussions and Code Reviews
Too often, important design decisions and intricate code details are scattered around the mailing list, wiki and bug system. This makes it hard to understand why certain decisions were made and why things are implemented the way that they are. It is critical to OpenJFX that JIRA is the tool used to track the details. When an interesting discussion happens on the mailing list, move the discussion to JIRA and invite interested parties to watch the bug.
The team has been evaluating Crucible as a code review tool. Unfortunately, Crucible is not integrated with JBS and cannot be accessed by the community. Therefore, we have decided to continue using webrev and conduct code reviews in JBS bugs.
Starting a Review
When code is ready for review, the developer creates a webrev and makes it available at a public link location (see http://openjdk.java.net/guide/codeReview.html) or attaches a patch to the JIRA. For small code changes or in cases where the person contributing the code is not a committer, patches are acceptable.
Then they do the following:
- Add the list of reviewers to the watchers list in JIRA
- Add a comment to the JIRA with an indication that the review is starting, a link to the webrev, a list of desired reviewers, and any technical information that would assist reviewers. The following is a good example for the JIRA comment to start the review:
.
Please review the following: http://cr.openjdk.java.net/foo/bar
Reviewers: kcr, jgiles
This is a really complicated, yet elegant fix to this issue. The idea is to do this, that, and the other thing, thus avoiding the xyz bug...
- An e-mail is sent to openjfx-dev@openjdk.java.net with a pointer to the JIRA as follows:
.
Subject: [<release>] Review request for <bug-id>: <summary>
To: <desired explicit reviewers>
CC: <mailing list>
Body: <link to the JIRA with a very short description of the problem>
All discussion and approvals will happen in JBS (not on the mailing list). In this manner all activity associated with the closing of the bug is captured in a single place. An exception to this is that openjfx-dev list members who do not have direct write access to JBS should send review comments to the mailing list. When the developer has addressed the points raised by the review, a new webrev is created and the review continues.