Gerrit/Code review/Getting reviews


 * To learn about reviewing code from others check the tutorial and the Code review guide.

How to get your code reviewed faster?

Introduce yourself well
Commit message should describe what and why. What was the problem? How does the fix resolve it? How to test that it actually works? See Gerrit/Commit message guidelines for more.

Also, proofread. Having typos in your commit message and code is ugly and makes your commit smell of carelessness.

Respond to test failures and feedback
Check your Gerrit settings and make sure you're getting email notifications. If your code fails automated tests, or you got some review already, respond to it in a comment or resubmission. Or hit the Abandon button to remove your commit from the review queue while you revise it.

(To see why automated tests fail, click on the link in the "failed" comment in Gerrit, hover over the failed test's red dot, wait for the popup to show, and then click "console output.")

Sometimes you'll receive reviews which you'll perceive as irrelevant, for instance merely cosmetic. You must treasure such reviews. Amend your patch to satisfy trivial requests: the reviewer will be more likely to feel the patch as "theirs" and to +1 or +2 it in "return"; you may disagree, but discussing costs time and is more expensive than conceding a point. Don't be negative and don't try to convince reviewers their contribution was null (or negative), that doesn't bring you anywhere.

Add reviewers
Right after you commit, add someone's name to the changeset as Reviewer. (These are requests – there's no way to assign a review to someone in Gerrit.) Experienced developers should try to help with this: if you notice an unreviewed changeset lingering, then please add reviewers. To find reviewers:


 * Check the main maintainers list, or the maintainers listed in the extension's page, to find who's currently maintaining that part of the code, or is in maintainer training.
 * Click the magnifying glass in the "Project" row of your gerrit patch. Now find other changesets in that repository: the people who write and review those changesets would be good candidates to add as reviewers. Or see who can approve your patch: click "Access" in the top navigation bar, click the link(s) in "Owner" rows, see the list of names.
 * To find out who added a system message and why, see Gerrit/Navigation for specific advice.
 * Search through other commit summaries and changesets. Navigate the repository tree to your repo/directory and click "history" to see who is active in the area, for instance core DB. Or search on gerrit: Matma Rex and Foxtrott are interested in reviewing frontend changes, so I search for "message:css" to find changesets that mention CSS in their commit summaries to add them to. You can use this and regexes to find changes that touch the same components you're touching, to find likely reviewers (search docs).

Review more
Many eyes make bugs shallow. Read the Code review guide and help other authors by praising or criticizing their commits. Comments are nonbinding, won't cause merges or rejections, and have no formal effect on the code review. But you'll learn, gain reputation, and get people to return the favor by reviewing you in the future. "How to review code in Gerrit" has the step-by-step explanation. Example Gerrit search for MediaWiki commits that have not had +1, +2, -1, or -2 reviews yet.

Don't mix rebases with changes
When rebasing, only rebase. That makes it easier to use the "Old Version History" dropdown, which greatly quickens reviews. If non-rebase changes are made inside a rebase changeset, you have to read through a lot more code to find it and it's non-obvious. When you're making a real change, leave a Gerrit comment explaining your change, and revise your commit summary to make sure it's still accurate.

Write small commits
It's easier for other people to review small changes that only change one thing. We'd rather see five small commits than one big one.

If your commits are going to be touching the same files repeatedly, bundle them up into one large commit (using either  or squashing after the fact).

If your commits are going to be touching separate files and there's not a lot of dependency between them, it's probably best to keep them as smaller discrete commits.

Adhere to the coding conventions
The checklists above are meant to be quick; there is more in the coding conventions.

When you adhere to the coding conventions, the person reviewing your code can focus on the meaning of your change, rather than be distracted by its superficial qualities. You'll also show care and be more likely to get constructive reviews.

Don't cause security worries

 * from Security checklist for developers

Challenges
Code review remains challenging nevertheless. Known issues include:
 * Having tags in Gerrit would be nice. Commit summary lines are sometimes abused for this.
 * Post-commit review does not yet have a process, but cases seem to be relatively rare.
 * We are concerned that dashboards will become private in future. Currently it's possible to see everyone's dashboards.
 * Time is wasted scanning for stuff to review in different Gerrit listings.
 * Unowned code rotting around. Result of bad bus factor.
 * Lack of tools to effectively debug PHP code without print/var_dump/error_log etc.
 * Use wfProfileIn/Out is typically used for things like reading or writing to files, shell calls, http calls.
 * Reviewers need to own/watch certain extensions to effectively review new patches against them.
 * JavaScript code needs more documentation than PHP.
 * Sadly, automatic code formatting tools are rarely used.