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

Justin Wood (Callek) callek at gmail.com
Fri Apr 1 03:11:35 UTC 2011


I wholeheartedly agree. And I also would like to bring this up to 
SeaMonkey council and adopt it for us.

The deal-breaker for me is "module owner or peer of code in question." 
since there are *some* people who could abuse this power if that wasn't 
the case.

~Justin Wood (Callek)

On 3/31/2011 9:58 PM, Andrew Sutherland wrote:
> 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.
>




More information about the tb-planning mailing list