DevTools/Code Review Checklist
From MozillaWiki
< DevTools
This checklist is primarily useful for DevTools reviewers as it lists all points potentially important to check while reviewing code, but it can serve as a set of guidelines for patch authors too.
Contents
Bug status and patch file
- Bug status is assigned, and assignee is correctly set
- Commit title and message follow the convention at : https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions
- Commit message says what is being changed and why as described here : http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/commits.html#write-detailed-commit-messages
- Patch applies locally to current sources with no merge required
- Check that every new file introduced by the patch has proper Mozilla license header : https://www.mozilla.org/en-US/MPL/headers/
Manual testing
- Verify the feature or fix
- Report problems in the global review comment
- Decide which bugs need to block landing the change, and which can be fixed as follow-ups instead
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
Code review
- Code changes
- Review only what was changed by the contributor
- Code formatting follows our ES Lint rules, see CodeStyle documentation at : https://wiki.mozilla.org/DevTools/CodingStandards
- Code makes use of ES features when relevant
- Code is properly commented, JSDoc is updated, new "public" methods all have JSDoc, see Comment guidelines at : https://wiki.mozilla.org/DevTools/CodingStandards#Comments
- If Promise code was added/modified, the right promise syntax is used, rejection are handled and Tasks are used when possible : https://wiki.mozilla.org/DevTools/CodingStandards#Asynchronous_Code
- If a CSS file is added/modified it follows the guidelines at : https://wiki.mozilla.org/DevTools/CSSTips
- If a React or Redux module is added/modified it follows the guidelines at : https://wiki.mozilla.org/DevTools/CodingStandards#React_.26_Redux
- 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 : https://wiki.mozilla.org/DevTools/Hacking#Writing_Tests
- Test logging is sufficient to help investigating test failures/timeouts : https://wiki.mozilla.org/DevTools/mochitests_coding_standards#Logs_and_comments
- Test is e10s compliant (no CPOWs, no content etc...). See : https://wiki.mozilla.org/DevTools/mochitests_coding_standards#E10S_.28Electrolysis.29
- Tests follow the guidelines at : https://wiki.mozilla.org/DevTools/mochitests_coding_standards#Writing_clean.2C_maintainable_test_code
- A try push has started (or even better, is green already)
- User facing changes
- If a new piece of UI or new user interaction is added/modified, then Helen is ui-review? on the bug
- If a user facing string has been added, it is localized and follows the localization guidelines at : https://wiki.mozilla.org/DevTools/Hacking#Guidelines
- If a user-facing string has changed meaning, the key has been updated : https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings
- 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 guidelines at : https://dxr.mozilla.org/mozilla-central/source/devtools/docs/svgs.md
- 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