proposal for blanket authorized orange fixes for mail & mailnews meeting specific constraints

Andrew Sutherland asutherland at asutherland.org
Thu Mar 31 18:58:52 PDT 2011


I would propose that we formally introduce something like 
rs=simple-orange-fix for orange fixes in mail/ and mailnews/ that meet 
the following constraints:

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.

I would additionally propose a similar thing called rs=orange-debugging 
which has the following constraints:
1) Is trying to fix an intermittent orange test failure.
2) Only touches test logic.
3) Only adds/modifies logging statements, although the logs can be 
conditional.
4) 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) Patch is by module owner/peer for the affected test.
6) There must be an associated bug tracking the orange bug to be fixed.

The basic idea is that we want to make/keep the bar low to fix 
intermittent oranges.  In many cases, it may not even be obvious what is 
happening, so being able to get additional logging into the tree easily 
is useful.  The rationale for avoiding the review step is that in many 
cases the exact nuances of the failure may be complex and there's not a 
lot of benefit for a reviewer to either perform a cursory review or 
spend a lot of time getting up to speed on what the failure.  (And the 
idea is that silly errors like typos will get weeded out by having run 
the tests locally and/or on the try server.)

For example, I just (hopefully) fixed a gloda intermittent orange test 
failure bug whose failure was an artifact of the way the test was 
attempting to approximate the sequence of [user triggers compaction, 
user quits before compaction is processed, indexing sweep at next 
startup completes compaction processing].  While it would be awesome if 
there was someone who was sufficiently up-to-speed on the gloda 
internals to be able to easily know if I was screwing up with my fix or 
fixing it the wrong way, it would take a lot of research for them to get 
up-to-speed enough to do that, and that effort would be better spent by 
learning about gloda by fixing low-hanging bugs.

We would document these on 
https://developer.mozilla.org/en/Mailnews_and_Mail_code_review_requirements 
assuming we're all good with the changes.

Andrew


More information about the tb-planning mailing list