Adopting the mozilla-central superreview policy in comm-central
Mark Banner
propose that we adopt the mozilla-central super review policy for at
least the Thunderbird code in comm-central. The current policy is here:
http://www.mozilla.org/hacking/reviewers.html
I think this has various advantages, including not all patches in
mailnews requiring sr - especially the small ones where we tend to use
the same reviewers anyway - whilst ensuring that API changes get the
extra consideration that they sometimes need.
The policy would not stop reviewers asking for additional reviews if
they felt it necessary e.g. due to lack of knowledge of areas of the
code base.
I would recommend that the same policy covers all of our Thunderbird
areas - editor/ directory/ mailnews/ mail/.
If we get general agreement here, I'm planning on taking this out to the
newsgroups and seeing if at least suite/ wants to join as well (I'm not
sure calendar is in a position to) - so this would minimise the
exceptions that we need to specify.
Please respond with questions, comments, etc. If there's no responses,
I'll assume everyone is in agreement and start moving the discussion
forward to the newsgroups.
Mark.
_______________________________________________
tb-planning mailing list
tb-pl...@mozilla.org
https://mail.mozilla.org/listinfo/tb-planning
David Bienvenu
- David
On 6/10/2010 1:54 AM, Mark Banner wrote:
> Having thought about it on and off for a while now, I'd like to propose that we adopt the mozilla-central super review policy for at least the Thunderbird code in
> comm-central. The current policy is here:
>
> http://www.mozilla.org/hacking/reviewers.html
>
_______________________________________________
Dan Mosedale
Dan
Kent James
Previously, I interpreted the policy (perhaps incorrectly) that all
/mailnews needed sr and all /mail did not.
As I understand the change, you are saying that neither /mailnews nor
/mail will require sr, but sr will be added when the complexity tests of
the new review process is met. So this would result in more sr in /mail,
and less in /mailnews.
Is that roughly correct?
rkent
David Bienvenu
> What does this actually mean in practice?
>
> Previously, I interpreted the policy (perhaps incorrectly) that all /mailnews needed sr and all /mail did not.
>
> As I understand the change, you are saying that neither /mailnews nor /mail will require sr, but sr will be added when the complexity tests of the new review process is
> met. So this would result in more sr in /mail, and less in /mailnews.
>
> Is that roughly correct?
- David
Karsten Düsterloh
> Having thought about it on and off for a while now, I'd like to
> propose that we adopt the mozilla-central super review policy for at
> least the Thunderbird code in comm-central. The current policy is here:
>
> http://www.mozilla.org/hacking/reviewers.html
>
> I think this has various advantages, including not all patches in
> mailnews requiring sr - especially the small ones where we tend to use
> the same reviewers anyway - whilst ensuring that API changes get the
> extra consideration that they sometimes need.
I'm wary, to be honest.
Especially /mailnews is a precious and in parts rather complex beast,
which IMO has seen too few serious reviewing, not too much.
> If we get general agreement here, I'm planning on taking this out to the
> newsgroups and seeing if at least suite/ wants to join as well
Note that /suite/mailnews even has a stricter policy than the rest of
/suite, and I don't think this will change
(<http://www.seamonkey-project.org/dev/review-and-flags>).
> Please respond with questions, comments, etc. If there's no responses,
> I'll assume everyone is in agreement and start moving the discussion
> forward to the newsgroups.
Actually, I find this policy change strange, to say the least. I'd say
that TB needs - no offence meant! - more reviewing, not less.
I'm aware that reviewers as such are a scarce resource, but (as I said
before) you can't exchange competence by automation...
Karsten
Ben Bucksch
> I'd like to propose that we adopt the mozilla-central super review policy
++
Cuts down one review, and focuses the sr more on the APIs, which is the
most important for use of the codebase.
Ben Bucksch
> Especially /mailnews is a precious and in parts rather complex beast,
> which IMO has seen too few serious reviewing, not too much.
Agreed. But we did have sr for it, and it didn't help. You can't help
that with more review steps, though, but the one review being performed
more "seriously" (your word), or carefully, or mindful.
In other words, replace "more" with "better", and I'd agree ;-)
> Actually, I find this policy change strange, to say the least. I'd say that TB needs - no offence meant! - more reviewing, not less.
If the rules are fine for Gecko (netwerk, renderer etc.), they should be
fine for TB.
Dan Mosedale
>> Please respond with questions, comments, etc. If there's no responses,
>> I'll assume everyone is in agreement and start moving the discussion
>> forward to the newsgroups.
> Actually, I find this policy change strange, to say the least. I'd say
> that TB needs - no offence meant! - more reviewing, not less.
> I'm aware that reviewers as such are a scarce resource, but (as I said
> before) you can't exchange competence by automation...
nuance of the change that's on the table here. In particular, it seems
to me an attempt to focus the reviewing eyes where they're _most_
needed, rather than trying to blanket a too-large-code-base using
insufficient resources. Given that anyone (patch author or reviewer)
can and should be asking for second reviews when appropriate, this
strikes me as likely to still result in good general coverage. If you
disagree, I'm curious to understand in more detail...
Dan
Andrew Sutherland
> Actually, I find this policy change strange, to say the least. I'd say
> that TB needs - no offence meant! - more reviewing, not less.
> I'm aware that reviewers as such are a scarce resource, but (as I said
> before) you can't exchange competence by automation...
Can you cite specific examples where you feel that the reviewing
performed was insufficient? (I'm asking for a specific feature or
regression; you don't need to trawl the hg log for a specific commit.)
Many times we have intentionally landed (large) things that were not
completely baked intentionally for a mixture of reasons, time frequently
being a concern.
In the hypothetical, I'd be more inclined to blame a lack of tests than
a lack of reviewing. (Although obviously the reviewer can request
additional tests.)
Andrew
Justin Wood (Callek)
to make.
The mozilla-central current rule exists after THOUSANDS of tests have
been written and in use. And have (for a lot longer than us) REQUIRED
tests for every bug patch.
Now I know TB has required tests, but we are still in a way "O, orange;
must be a perma orange, likely not my fault" general mentality on trunk.
Suite and TB coverage here would help my level of confidence, even more
so if we are sure to get a much larger and more redundant level of code
coverage with our tests.
That said, additional reviews for the sake of additional reviews doesn't
help much; I know that MANY of my changes are not api changes, and I
will r? and sr? the same person when the section of code needs sr.
Though I also will review others if I think multiple expertise will help
the code itself, (say a build config thing that affects l10n as well as
TB I'd of course r? KaiRo, and Mark). If it affects TB Buildbot, gozer;
etc. but I don't feel a sr+ is required on many levels, based on my
understanding of sr.
Second-Review could be a whole another story, and should be used if the
reviewer doesn't fully grasp the code, etc.
--
~Justin Wood (Callek)
Karsten Düsterloh
>> Actually, I find this policy change strange, to say the least. I'd
>> say that TB needs - no offence meant! - more reviewing, not less.
>> I'm aware that reviewers as such are a scarce resource, but (as I
>> said before) you can't exchange competence by automation...
> I guess I'm not convinced that "more" versus "less" really captures
> the nuance of the change that's on the table here.
Well, not in the strict sense of words. 10 people looking at something
they don't fully grasp instead of 1 surely won't help.
Certain classes of potential bugs are just invisible to the less
experienced. Hence they _can't_ and won't request further
(super-)review, because they don't even know there's a problem!
> In particular, it seems to me an attempt to focus the reviewing eyes
> where they're _most_ needed, rather than trying to blanket a
> too-large-code-base using insufficient resources.
I just doubt that exchanging quality by throughput is helping.
> Given that anyone (patch author or reviewer) can and should be asking
> for second reviews when appropriate,
How would they know? (See above.)
Karsten
Dan Mosedale
> Ok, I'm not inherently against this, BUT there are a few points I
> want to make.
>
> The mozilla-central current rule exists after THOUSANDS of tests have
> been written and in use. And have (for a lot longer than us) REQUIRED
> tests for every bug patch.
did for mozilla-central." That's certainly possible. I don't see
evidence that we shouldn't accept some amount of risk in order to be
able to move faster.
> Now I know TB has required tests, but we are still in a way "O,
> orange; must be a perma orange, likely not my fault" general mentality
> on trunk.
gradually back into shape.
> Suite and TB coverage here would help my level of confidence, even
> more so if we are sure to get a much larger and more redundant level
> of code coverage with our tests.
beginning of the year, I think we're on our way there! If you
disagree, I'd be interested to understand why.
Dan
Dan Mosedale
> Dan Mosedale aber hob zu reden an und schrieb:
>>> Actually, I find this policy change strange, to say the least. I'd
>>> say that TB needs - no offence meant! - more reviewing, not less.
>>> I'm aware that reviewers as such are a scarce resource, but (as I
>>> said before) you can't exchange competence by automation...
>> I guess I'm not convinced that "more" versus "less" really captures
>> the nuance of the change that's on the table here.
> Well, not in the strict sense of words. 10 people looking at something
> they don't fully grasp instead of 1 surely won't help.
way it applies to the situation at hand.
> Certain classes of potential bugs are just invisible to the less
> experienced. Hence they _can't_ and won't request further
> (super-)review, because they don't even know there's a problem!
review is advantageous, I would expect that most of the time, reviewers
will be.
>> In particular, it seems to me an attempt to focus the reviewing eyes
>> where they're _most_ needed, rather than trying to blanket a
>> too-large-code-base using insufficient resources.
> I just doubt that exchanging quality by throughput is helping.
>
trade acceptance of more risk in areas that don't have broad effects
(localized changes) in exchange for less risk in areas that do
(cross-module, API, and refactoring changes).
>> Given that anyone (patch author or reviewer) can and should be asking
>> for second reviews when appropriate,
> How would they know? (See above.)
of the time. But the fact that we're interested in accepting risk only
in areas with less far-reaching changes is what makes this change a good
idea.
Dan
Justin Wood (Callek)
> On 6/10/10 4:59 PM, Justin Wood (Callek) wrote:
>> Ok, I'm not inherently against this, BUT there are a few points I
>> want to make.
>>
>> The mozilla-central current rule exists after THOUSANDS of tests have
>> been written and in use. And have (for a lot longer than us) REQUIRED
>> tests for every bug patch.
> It sounds like you might be saying "this has somewhat more risk than
> it did for mozilla-central." That's certainly possible. I don't see
> evidence that we shouldn't accept some amount of risk in order to be
> able to move faster.
Basically yes; I am saying that. I am not however saying that it is an
unacceptable amount of additional risk.
>> Now I know TB has required tests, but we are still in a way "O,
>> orange; must be a perma orange, likely not my fault" general
>> mentality on trunk.
> Now that 3.1 is in release candidate mode, the trunk is being whipped
> gradually back into shape.
Of course, but TB also runs much less tests than SeaMonkey, even with
its added MozMill tests (That SM does not yet run). I don't know of any
way to get TB to run the relevant tests reliably, but that does make it
less tests as well that are run relating to core code.
I also note that *my* focus on test side is both TB and SM, so my margin
of thought stretches a bit further than is relevant in this thread on
this point
>> Suite and TB coverage here would help my level of confidence, even
>> more so if we are sure to get a much larger and more redundant level
>> of code coverage with our tests.
> Given the new test requirement policies that went into effect at the
> beginning of the year, I think we're on our way there! If you
> disagree, I'd be interested to understand why.
I certainly agree that we have made GREAT strides, and are headed in the
right direction, so I don't disagree that the right choices _are_ being
made.
I just presented my points more as a "If we chose to do this, lets just
be sure to acknowledge these differences between us adopting this policy
and m-c"
--
~Justin Wood (Callek)
Mark Banner
> On 6/11/2010 1:39 PM, Dan Mosedale wrote:
>> On 6/10/10 4:59 PM, Justin Wood (Callek) wrote:
>>> Now I know TB has required tests, but we are still in a way "O,
>>> orange; must be a perma orange, likely not my fault" general
>>> mentality on trunk.
>> Now that 3.1 is in release candidate mode, the trunk is being whipped
>> gradually back into shape.
>
> Of course, but TB also runs much less tests than SeaMonkey, even with
> its added MozMill tests (That SM does not yet run). I don't know of
> any way to get TB to run the relevant tests reliably, but that does
> make it less tests as well that are run relating to core code.
all. If so, that has basically been a conscious decision that we do not
need to run those tests constantly because Firefox is running them for
us. Yes, some of the core code base is slightly different, and there may
be one or two areas that running the tests against Thunderbird may
reveal something, but on the whole, we're just letting the Firefox
builders do the work for us there.
There was talk about running reftests occasionally (because we can),
e.g. near/at release times, I expect that may come once we do packaged
tests, as it would be easier to do.
Re getting TB tests run reliably. AFAIK apart from one intermittent
failure (bug 552804), all the MozMill tests for Thunderbird are pretty
stable on Windows and Mac - Linux we know has various issues.
xpcshell-tests should also be pretty stable. So if you're getting
problems running tests, then that's something you should be filing a bug
on (or a new thread) and getting it resolved.
Mark.
Justin Wood (Callek)
> On 12/06/2010 03:00, Justin Wood (Callek) wrote:
>> On 6/11/2010 1:39 PM, Dan Mosedale wrote:
>>> On 6/10/10 4:59 PM, Justin Wood (Callek) wrote:
>>>> Now I know TB has required tests, but we are still in a way "O,
>>>> orange; must be a perma orange, likely not my fault" general
>>>> mentality on trunk.
>>> Now that 3.1 is in release candidate mode, the trunk is being
>>> whipped gradually back into shape.
>>
>> Of course, but TB also runs much less tests than SeaMonkey, even with
>> its added MozMill tests (That SM does not yet run). I don't know of
>> any way to get TB to run the relevant tests reliably, but that does
>> make it less tests as well that are run relating to core code.
> I assume by "much less tests" you're referring to mochitest, reftest
> and all. If so, that has basically been a conscious decision that we
> do not need to run those tests constantly because Firefox is running
> them for us. Yes, some of the core code base is slightly different,
> and there may be one or two areas that running the tests against
> Thunderbird may reveal something, but on the whole, we're just letting
> the Firefox builders do the work for us there.
For 99.9% of the tests, I agree; for the other .1% I don't think the
effort to try and run them is worth it.
> There was talk about running reftests occasionally (because we can),
> e.g. near/at release times, I expect that may come once we do packaged
> tests, as it would be easier to do.
I _think_ reftests are the most likely to continue to pass between
Firefox and Thunderbird/SeaMonkey, due to reftests primarily testing
Gecko Rendering stuff not preferences/code of other parts of the app
platform.
> Re getting TB tests run reliably. AFAIK apart from one intermittent
> failure (bug 552804), all the MozMill tests for Thunderbird are pretty
> stable on Windows and Mac - Linux we know has various issues.
> xpcshell-tests should also be pretty stable. So if you're getting
> problems running tests, then that's something you should be filing a
> bug on (or a new thread) and getting it resolved.
I agree TB is in a MUCH MUCH better state than SM as far as reliable
tests, that said I am starting to find time to sneak in work on test
stability as well, which I am turning into bugs and patches where
appropriate. though I have not yet looked too deeply into MozMill tests
(which is the majority of TB ones).
--
~Justin Wood (Callek)
Ludovic Hirlimann
> On 6/12/2010 5:52 AM, Mark Banner wrote:
>> On 12/06/2010 03:00, Justin Wood (Callek) wrote:
>>> On 6/11/2010 1:39 PM, Dan Mosedale wrote:
>>>> On 6/10/10 4:59 PM, Justin Wood (Callek) wrote:
>>>>> Now I know TB has required tests, but we are still in a way "O,
>>>>> orange; must be a perma orange, likely not my fault" general
>>>>> mentality on trunk.
>>>> Now that 3.1 is in release candidate mode, the trunk is being
>>>> whipped gradually back into shape.
>>>
>>> Of course, but TB also runs much less tests than SeaMonkey, even
>>> with its added MozMill tests (That SM does not yet run). I don't
>>> know of any way to get TB to run the relevant tests reliably, but
>>> that does make it less tests as well that are run relating to core
>>> code.
>> I assume by "much less tests" you're referring to mochitest, reftest
>> and all. If so, that has basically been a conscious decision that we
>> do not need to run those tests constantly because Firefox is running
>> them for us. Yes, some of the core code base is slightly different,
>> and there may be one or two areas that running the tests against
>> Thunderbird may reveal something, but on the whole, we're just
>> letting the Firefox builders do the work for us there.
>
> For 99.9% of the tests, I agree; for the other .1% I don't think the
> effort to try and run them is worth it.
on release builds.
Ludo
--
Ludovic Hirlimann MozillaMessaging QA lead
http://flic.kr/photos/lhirlimann
http://www.spreadthunderbird.com/aff/79/2
Ludovic Hirlimann
Greemlins will always krept-in where you don't look. Having 100%
coverage is better than 99,9% - and that's why I only care about release
builds.
> Do you assert that Core will break in odd ways that will not also be
> covered by SeaMonkey and/or Firefox running those test suites?
No. I'm just saying I'll sleep better if we test 100% on release builds
than 99,9%. And we might end up in issues not being visible in the
eamonkey branch , nor the Ff, because of some weird and yet undiscovered
HG bug. So I think It make sense to do it on our release builds.
Justin Wood (Callek)
Why? Data?
Do you assert that Core will break in odd ways that will not also be
covered by SeaMonkey and/or Firefox running those test suites?
--