js tests, in-testsuite flag and bug verifications

[Bob Clary-2]

Hey folks.

Please remember to add tests when you check in a fix for a bug. It can
be in any of the supported test suites, but especially those which are
(or will be) executed on the unit test machines. For security sensitive
tests, simply attach the test to the bug until the bug is made public.

When you do check in a test, please set the in-testsuite flag to + and
include the test type and name or a path to the test in a comment to
help qa when searching bugzilla for a particular test. If there is no
way to test a particular patch, please set in-testsuite to -.

We have a very large back log of bugs fixed which do not have
in-testsuite set. Of these, there are many which have the testcase
keyword and are fairly quick fodder for adding tests. I would like to
ask each of you to review your fixed bugs which have the testcase
keyword and to add tests where appropriate. Please don't forget to set
the in-testsuite flag as appropriate.

There has been some discussion of whether bug verification is worth
while or not. Depending on whether you all think bug verifications are
useful, it would also be really cool if after you check in and the test
passes on the unit test boxes, you would go ahead and verify the fix. I
don't think that this is too much of a violation of the developer/qa
separation or is too much extra work since you should be following the
unit test boxes to make sure the patch hasn't regressed anything and has
indeed fixed the new test. Push back on this idea welcome.

Finally, I would like to change our culture a bit and to ask everyone to
immediately back out patches that regress any tests. Again, push back
welcome.

bc


_______________________________________________
dev-tech-js-engine mailing list
dev-tech-js-engine@...
https://lists.mozilla.org/listinfo/dev-tech-js-engine

[David Mandelin]

On 10/15/09 2:37 PM, Bob Clary wrote:
> Hey folks.
>
> Please remember to add tests when you check in a fix for a bug. It can
> be in any of the supported test suites, but especially those which are
> (or will be) executed on the unit test machines. For security sensitive
> tests, simply attach the test to the bug until the bug is made public.

Do you have any idea how well we are doing on this now? Do we generally
get tests with a few missing ones, or do we need to improve a lot on this?

> When you do check in a test, please set the in-testsuite flag to + and
> include the test type and name or a path to the test in a comment to
> help qa when searching bugzilla for a particular test. If there is no
> way to test a particular patch, please set in-testsuite to -.

I wasn't clear on what these flags meant before and who was expected to
set them. (In particular, I wasn't aware that set included me! ;-D) So
thanks for explaining that again.

> There has been some discussion of whether bug verification is worth
> while or not. Depending on whether you all think bug verifications are
> useful, it would also be really cool if after you check in and the test
> passes on the unit test boxes, you would go ahead and verify the fix. I
> don't think that this is too much of a violation of the developer/qa
> separation or is too much extra work since you should be following the
> unit test boxes to make sure the patch hasn't regressed anything and has
> indeed fixed the new test. Push back on this idea welcome.

I'm not familiar with that discussion, but I just reviewed the status
flags tutorial [1]. According to that, "FIXED" means QA has found that
the original issue is fixed, and "VERIFIED" means QA has signed off on
the fix (presumably meaning no new bugs introduced, etc). So I
personally haven't been setting those statuses.

It sounds like you are talking about "FIXED" meaning a patch has been
pushed that the dev thinks will fix the issue, and "VERIFIED" meaning
the dev has confirmed the landing stuck. Did I get that right?

That seems reasonable, but I wonder what to do about the tracemonkey
repo. Most JS fixes land there, and normally the flags would not be set
until it lands in m-c. So presumably when the fix lands in the TM repo,
the bug is marked 'fixed-in-tracemonkey'. And then the flags are set to
something else when the merge is done. FIXED? VERIFIED?

> Finally, I would like to change our culture a bit and to ask everyone to
> immediately back out patches that regress any tests. Again, push back
> welcome.

That seems like the right thing to do. Are we not already doing that? I
pushed a patch to TM a couple weeks ago that regressed tsspider perf. I
backed it out as soon as I found out about that, which was the same day
(but not the same hour) the perf regression was recorded.

[1] http://quality.mozilla.org/bugs-life-walkthrough

--
Dave
_______________________________________________
dev-tech-js-engine mailing list
dev-tech-js-engine@...
https://lists.mozilla.org/listinfo/dev-tech-js-engine

[Bob Clary-2]

On 10/20/09 11:21 AM, David Mandelin wrote:

> On 10/15/09 2:37 PM, Bob Clary wrote:
>> Hey folks.
>>
>> Please remember to add tests when you check in a fix for a bug. It can
>> be in any of the supported test suites, but especially those which are
>> (or will be) executed on the unit test machines. For security sensitive
>> tests, simply attach the test to the bug until the bug is made public.
>
> Do you have any idea how well we are doing on this now? Do we generally
> get tests with a few missing ones, or do we need to improve a lot on this?
>

Just looking at the tracemonkey changesets for October I see 19 with
changes to
analysis-tests|jsapi-tests|lirasm/tests|js/src/tests/trace-test/crashtests/xpconnect/tests|mochitest
and 123 without. It's probably not as bad as that seems, but I would say
some improvement would be good.

>> When you do check in a test, please set the in-testsuite flag to + and
>> include the test type and name or a path to the test in a comment to
>> help qa when searching bugzilla for a particular test. If there is no
>> way to test a particular patch, please set in-testsuite to -.
>
> I wasn't clear on what these flags meant before and who was expected to
> set them. (In particular, I wasn't aware that set included me! ;-D) So
> thanks for explaining that again.
>

I originally created them pretty much for my own use but they are in use
by the rest of the modules these days. In the event that someone like me
is following behind developers and creating tests, the flag is very
useful for determining which bugs need tests.

When developers begin adding tests as a regular part of check ins, the
flag will still have a good use when trying to verify bugs. A bug with
in-testsuite+ should be verifiable just by looking at the results of the
unit tests.

>> There has been some discussion of whether bug verification is worth
>> while or not. Depending on whether you all think bug verifications are
>> useful, it would also be really cool if after you check in and the test
>> passes on the unit test boxes, you would go ahead and verify the fix. I
>> don't think that this is too much of a violation of the developer/qa
>> separation or is too much extra work since you should be following the
>> unit test boxes to make sure the patch hasn't regressed anything and has
>> indeed fixed the new test. Push back on this idea welcome.
>
> I'm not familiar with that discussion, but I just reviewed the status
> flags tutorial [1]. According to that, "FIXED" means QA has found that
> the original issue is fixed, and "VERIFIED" means QA has signed off on
> the fix (presumably meaning no new bugs introduced, etc). So I
> personally haven't been setting those statuses.
>
> It sounds like you are talking about "FIXED" meaning a patch has been
> pushed that the dev thinks will fix the issue, and "VERIFIED" meaning
> the dev has confirmed the landing stuck. Did I get that right?
>
> That seems reasonable, but I wonder what to do about the tracemonkey
> repo. Most JS fixes land there, and normally the flags would not be set
> until it lands in m-c. So presumably when the fix lands in the TM repo,
> the bug is marked 'fixed-in-tracemonkey'. And then the flags are set to
> something else when the merge is done. FIXED? VERIFIED?
>

You have a good point about tracemonkey vs mozilla-central. I'm not sure
what we should do about that. Maybe verified-in-tracemonkey? Or maybe we
just don't care. See below.

With regard to QA verifications, I normally don't count regressions
against a bug unless it is reopened. So it might be conceivable to mark
a bug as verified if it passed its test, even if it caused an open
regression.

The discussion was "Usefulness of the VERIFIED state?"
<http://groups.google.com/group/mozilla.dev.quality/search?group=mozilla.dev.quality&q=usefulness+of+the+verified+state&qt_g=Search+this+group>

The perspective I took from that discussion is that people didn't value
the effort required to do bug verifications and considered it a waste of
time. If we do immediately back out patches that fail the unit tests,
then I suppose even having the developer flip the bug to verified is not
useful either.

>> Finally, I would like to change our culture a bit and to ask everyone to
>> immediately back out patches that regress any tests. Again, push back
>> welcome.
>
> That seems like the right thing to do. Are we not already doing that? I
> pushed a patch to TM a couple weeks ago that regressed tsspider perf. I
> backed it out as soon as I found out about that, which was the same day
> (but not the same hour) the perf regression was recorded.
>
> [1] http://quality.mozilla.org/bugs-life-walkthrough

It depends on the case. I suppose it isn't as bad as it used to be. I
just wanted to reinforce the idea.
_______________________________________________
dev-tech-js-engine mailing list
dev-tech-js-engine@...
https://lists.mozilla.org/listinfo/dev-tech-js-engine