gecko-dev/devtools/docs/contributing/code-reviews-checklist.md

Code reviews checklist

This checklist is primarily aimed at reviewers, as it lists important points to check while reviewing a patch.

It can also be useful for patch authors: if the changes comply with these guidelines, then it's more likely the review will be approved.

Bug status and patch file

• Bug status is assigned, and assignee is correctly set.
• Commit title and message follow the conventions.
• Commit message says what is being changed and why.
• Patch applies locally to current sources with no merge required.
• Check that every new file introduced by the patch has the proper Mozilla license header: https://www.mozilla.org/en-US/MPL/headers/

Manual testing

• Verify:
  • if it's a new feature, the patch implements it.
  • if it's a fix, the patch fixes the bug it addresses.
• Report any problems you find in the global review comment.
• Decide if any of those problems should block landing the change, or if they can be filed as follow-up bugs instead, to be fixed later.

Automated testing

• Run new/modified tests, with and without e10s.
• Watch out for tests that pass but log exceptions or end before protocol requests are handled.
• Watch out for slow/long tests: suggest many small tests rather than single long tests.
• Watch out for new tests written as integration tests instead of as unit tests: unit tests should be the preferred option, when possible.

Code review

• Code changes:
  • Review only what was changed by the contributor.
  • Code formatting follows our ESLint rules and coding standards.
  • Code is properly commented, JSDoc is updated, new "public" methods all have JSDoc, see the comment guidelines.
  • If Promise code was added/modified, the right promise syntax is used and rejections are handled. See asynchronous code.
  • If a CSS file is added/modified, it follows the CSS guidelines.
  • If a React or Redux module is added/modified, it follows the React/Redux guidelines.
  • If DevTools server code that should run in a worker is added/modified then it shouldn't use Services
• Test changes:
  • The feature or bug is tested by new tests, or a modification of existing tests.
  • Test logging is sufficient to help investigating test failures/timeouts.
  • Test is e10s compliant (doesn't try to access web content from the parent process, etc…).
  • Tests are clean and maintainable.
  • A try push has started (or even better, is green already).
• User facing changes:
  • If any user-facing interfaces are added/modified, double-check the changes with the UX mockups or specs, if available. If there's any confusion, need-info the UX designer.
  • If a user facing string has been added, it is localized and follows the localization guidelines.
  • If a user-facing string has changed meaning, the key has been updated.
  • If a new image is added, it is a SVG image or there is a reason for not using a SVG.
  • If a SVG is added/modified, it follows the SVG guidelines.
  • If a documented feature has been modified, the keyword dev-doc-needed is present on the bug.

Finalize the review

• R+: the code should land as soon as possible.
• R+ with comments: there are some comments, but they are minor enough, or don't require a new review once addressed, trust the author.
• R cancel / R- / F+: there is something wrong with the code, and a new review is required.