wiki:PatchReview

Buildbot receives a lot of patches, and everyone is invited to comment on and review those patches. This page is a quick checklist to organize such reviews.

Overall Process

When a patch is submitted as a GitHub pull request, notifications go out to the commits email list as well as the #buildbot IRC channel. At this point, anyone other than the submitter can review the patch, following the guidelines here. Once the review is complete, the reviewer sets the "merge-me" label in GitHub. Once that label is set, anyone with commit access, other than the submitter and the reviewer, may perform an additional spot-check on the patch and merge it to the appropriate branch.

Notes

First, if you cannot review a patch completely, please provide as much feedback as you can anyway, but be explicit -- it can be frustrating for patch authors to address all of comments in a review only to get another set of comments on code that has not changed.

Make yourself familiar with the SubmittingPatches page for the expectations of a patch. It can be helpful to point patch authors to this page if they miss simple things.

Checklist

  • visual inspection..
    • does the patch add tests for all new code?
    • are all new or changed behaviors documented?
    • are all user-visible changes noted in release-notes.rst?
    • will the changes cause backward-compatibility problems for users?
    • are changes to existing methods or interfaces reflected in fake versions of those objects, so that tests are running against accurate interfaces?
    • does the code follow the coding style guide?
      • in particular, are function and method names written in interCaps?

  • deep thought
    • does this patch increase the API surface of Buildbot, and if so, is the change worth it, or can the desired functionality be implemented in a simpler to use manner that has less impact on the API?
    • does this patch make changes to Buildbot's model that are not reflected everywhere? (this can be tricky to spot, since by definition any missed changes do not appear in the patch)

  • with the patch applied..
    • do all of the tests pass?
    • does pyflakes succeed?
    • does the patch add unnecessarily long lines?
    • does the patch use four-space indents (no tabs)?
    • if there are db-related changes, do they work on all supported db's?
      • it's OK to leave this to metabuildbot's testing if you don't have the necessary infrastructure set up

Utilities

If you have Buildbot set up in a virtualenv, add coverage, mock, sphinx, and pyflakes, activate the virtualenv, and run ./common/validate.sh to check the items under "with the patch applied", above.

Last modified 13 months ago Last modified on Jan 28, 2016, 10:40:16 PM