524457 - windows crashtest automation passes --leak-threshold=484

Status: RESOLVED WORKSFORME

(Testing :: Reftest, defect, P3)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

In a log for the windows debug crashtest (part of debug everythingelse), I noticed that the build automation is passing --leak-threshold=484.

I don't think it should be doing this; we should be maintaining the default leak thresholds in-tree (if anything nonzero is needed) rather than having them be part of the build automation.


I'm not sure if there are any other tests passing --leak-threshold; it would be interesting to know if there are.

Comment 1

[Chris AtLee [:catlee]]

This is currently enabled on a per-branch/platform basis.  So all the win32 tests on mozilla-central, mozilla-1.9.2, mozilla-1.9.1, tracemonkey, electrolysis and places currently have a leak threshold for mochitest-plain (and all of its chunks) of 484, as well as a threshold of 484 for crashtest.

I think we'd be more than happy to have this maintained in-tree!

The current value of 484 was set when we rolled out a new version of java (1.6) onto our build machines.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK, then at least this wasn't a mistake.

But it probably would still be good to move this into the tree.

Comment 3

[Aki Sasaki (not active)]

Who would be in charge of putting this in the tree?
Removing the buildbot leak-threshold argument is a simple config.py patch, but we should wait until it's in-tree.

Comment 4

[Aki Sasaki (not active)]

Attachment: stop sending --leak-threshold

This patch disables --leak-threshold on the buildbot side.
Dependent on someone landing the threshold in the tree.

Welcoming patches, otherwise I can dig for it at some later date.

Comment 5

[Chris Cooper [:coop] (he/him)]

dbaron: where would we set the leak threshold in the tree? I can take a stab at rolling those patches if I know where to look.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

There used to be in-tree default leak thresholds for various mochitest variants in the tree in runtests.py.in.  The right place for leak thresholds for reftest/crashtest would probably be runreftest.py; however, either we'd end up with:

 (a) a pretty hacky solution, because we'd have to do something like parse the manifest filename to figure out if it's reftests or crashtests (and only do the threshhold when the argument is the manifest for the full suite)

 (b) add arguments to runreftest.py for --reftests and --crashtests for running the whole suite, and do the leak thresholds in those cases

I did (a) for a day or two on the 1.9.2 branch and then backed it out, but I'm not sure if it makes sense as a permanent solution.  Then again, it's really not that bad.  (See http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1226d4feb6cb ; note that the runtests.py.in part of the patch was restoring old code, but the runreftests.py part was new code I wrote for the occasion.)

Comment 7

[Nick Thomas [:nthomas] (UTC+12)]

bug 470487 requires a threshold of 484 bytes on m-1.9.1, but on other branches we may be able to set the limit to 0.

Comment 8

[Chris AtLee [:catlee]]

Futuring until in-tree changes are made

Comment 9

[Chris Cooper [:coop] (he/him)]

Mass move of bugs from Release Engineering:Future -> Release Engineering. See
http://coop.deadsquid.com/2010/02/kiss-the-future-goodbye/ for more details.

Comment 10

[Chris Cooper [:coop] (he/him)]

(In reply to comment #8)
> Futuring until in-tree changes are made

Are we still waiting on in-tree changes? Are bugs even filed for that yet?

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I don't know of a bug filed, although doing it in this bug would probably be sufficient.

Comment 12

[Chris Cooper [:coop] (he/him)]

Attachment: stop sending --leak-threshold, v2

Fixing bitrot on Aki's patch.

Comment 13

[Chris Cooper [:coop] (he/him)]

I think this belongs in Testing:Reftest now, since that component didn't exist when this was initially filed. As Aki indicated in comment #3, the releng changes are dead simple once the reftest harness can handle leak thresholds on it's own.

Comment 14

[Andrew McCreight [:mccr8]]

crashtest leak thresholds are in-tree now, so presumably this was fixed at some point.