Mailnews and Mail code review requirements

This document describes the process for reviewing patches to the mozilla/mailnews  and mozilla/mail directories. Thunderbird and SeaMonkey are the primary users of the code in mozilla/mailnews. Thunderbird's front-end code is stored in mozilla/mail. (A fair amount of this code is forked from code in the mailnews/ directory).

For background on the review process, see the Code Review FAQ. See the comm-central page for a detailed description of the code repository. Please direct questions or concerns to the dev-planning mailing list/newsgroup.

Mailnews and Mail review rules

Patches affecting Thunderbird User Experience or Interfaces

Getting early ui-reviews this reduces the chance that work is rejected due to needing further user experience changes, and hence is aimed at saving time for the developer and reviewers.

• All patches that affect user experience or interfaces on Thunderbird should have ui-review on those patches in addition to the reviews required below.
  • To obtained a ui-review on an attachment, set ui-review to '?' and enter :bwinton (for Blake Winton) as the requestee.  Mike Conley (:mconley) can also provide useful feedback, but Blake is the current ultimate decider for UI choices in Thunderbird.

Rules for all patches affecting mailnews/ and mail/

• Wherever possible, significant patches in a certain area should be reviewed by one of the listed sub-module owner/reviewers.
• Peers may grant review in any area covered by mailnews/, provided they feel confident in their knowledge of the code being changed.
• Sub-module peers may only grant review in the listed sub-module.
• If a reviewer feels that the patch would benefit from additional reviews, they should request a second review from an appropriate person.
• Super-review is required in certain situations: significant architectural refactoring, any change to any API, all changes that affect how code modules interact. See the full super-review policy for more details.

Unit test rules

• Patches are required to include automated tests which are run during make check or via MozMill in Thunderbird, but submitters are encouraged to request exceptions from reviewers in cases where the cost is believed to outweigh the benefit. Once unit tests are committed, the in-testsuite+ flag should be set on the bug.
• Unit tests must be included in patches and reviewed just like any other code.
• If an automated test framework is needed but is not yet available, the developer is encouraged to write appropriate test code and commit it. A bug should be filed on the needed test framework. The in-testsuite? flag should be set on the bug until the framework has been completed and the test code is running automatically.
• Certain build-config or tooling bugs which don't affect the actual product cannot be usefully tested. These bugs should have the in-testsuite- flag set.

Rubber-Stamp Approvals for Intermittently Failing ("Orange") Test Fixes/Debugging

In order to make it easier to debug and fix tests that fail intermittently ("intermittent orange" tests), we have created the following two rubber-stamps based on this proposal on the tb-planning mailing list.  The procedure to use these is to be sure to:

1. include "rs=simple-orange-fix" or "rs=orange-debugging" in the first line of the commit message
2. paste a link to the pushed commit with the rubber stamp in the bug
3. make sure you pasted a link to any try-server pushes of the patch in the bug

rs=simple-orange-fix

Requirements:

1. The patch is fixing an intermittent orange test failure.
2. The patch is a change exclusively to test logic.
3. The patch has identified a likely problem and fixes it; no commenting  out tests/changing what a test tests/adding huge timeouts.  (You could still ask a reviewer to approve such things, though.)
4. The patch does not change public test frameworks, specifically, nothing in mailnews/test/resources/ or mail/test/mozmill/shared-modules/.
5. For mozmill tests, the patch has passed on the try server.  For xpcshell tests, it has either passed on try or has been run locally on the appropriate platform(s).  Specifically, if the xpcshell test will do different things on Windows than it will do on OS X or Linux, it must be run on Windows and one of OS X / Linux.
6. The author of the patch is the module owner or a peer of the module to which the test corresponds.
7. A bug is still required, even if it is immediately marked fixed.

rs=orange-debugging

Requirements:

1. The patch is trying to fix an intermittent orange test failure.
2. The patch only touches test logic.
3. The patch only adds/modifies logging statements, although the logs can be conditional.
4. The patch must have been run at least once on the try server.  (I have done some mozmill orange fixing where the try server would not duplicate the orange failures of the trunk and so in order to get the enhanced logging, the changes need to go into the trunk.)
5. The patch is authored by the module owner/a peer for the affected test.
6. There must be an associated bug tracking the orange bug to be fixed.

Mailnews individuals and roles

Mail individuals and roles