468577 - mochitest leak test failures should TinderboxPrint a line for the failure

Status: VERIFIED FIXED

(Testing :: Mochitest, defect)

Description

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

When the mochitest (or chrome or browser) tests fail due to going over the leak threshold, they should TinderboxPrint a line like 

TinderboxPrint: mochitest <em class="testfail">leaks</em>

so that it's clear that the reason for the orange was leaks in the mochitest run.


I'm not sure whether this code should live on the buildbot side or in runtests.py.in, although it looks from existing precedent that it should probably be the former.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

Output if Mochitests crash:
  FAIL

Output if Mochitests have unexpected failures/passes:
  x/y/z

Output if Mochitests have unexpected failures/passes and leak:
  x/y/z LEAK

Untested, but not difficult to read and execute manually.

Note that output if the leak threshold isn't exceeded is like so and won't trigger the matching:

TEST-PASS | runtests-leaks | WARNING leaked 76 bytes during test execution (threshold set at 75000 bytes)

Comment 2

[[e-mail] (:bhearsum)]

Comment on attachment 352073 [details] [diff] [review]
Patch

>Index: mozilla/tools/buildbotcustom/unittest/steps.py
>===================================================================

>+            if "during test execution" in line and \
>+               "runtests-leaks" in line and \
>+               "TEST-UNEXPECTED-FAIL" in line:
>+               match = re.search(r"leaked (\d+) bytes during test execution", line)

Style nit: Don't indent continuation of the conditional the same amount as the following block, it makes it harder to read. Use a two space indent for continuation. Same thing applies to the ones below.


This looks fine to me. Coop knows more about this code than I though, so I'd like him to review it as well.

Comment 3

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

Comment on attachment 352073 [details] [diff] [review]
Patch

No issues here.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: With two-space indents

It doesn't seem to me that this makes it any easier to read, but I didn't mind the original anyway.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: [checked in] Er, this is what was really meant

Comment 6

[[e-mail] (:bhearsum)]

Comment on attachment 352172 [details] [diff] [review]
[checked in] Er, this is what was really meant

Going to test this on staging today.

Comment 7

[[e-mail] (:bhearsum)]

This hasn't caused any problems in staging - I think we can deploy this. Coop, any chance I can convince you to land this during the downtime on Friday? If not, I'll deploy it next week.

Comment 8

[[e-mail] (:bhearsum)]

Comment on attachment 352172 [details] [diff] [review]
[checked in] Er, this is what was really meant

Checking in steps.py;
/cvsroot/mozilla/tools/buildbotcustom/unittest/steps.py,v  <--  steps.py
new revision: 1.16; previous revision: 1.15
done

Comment 9

[[e-mail] (:bhearsum)]

Alright, the m-c/m-1.9.1 unittest master has this now - once I see a clean run I'll close this bug.

Comment 10

[[e-mail] (:bhearsum)]

Argh, I was wrong. This can't be deployed without a master restart - which takes down existing builds, or until we fix bug 469125..

Comment 11

[Serge Gautherie (:sgautherie)]

What is the current status ?

For an example I'm interested in:
{
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1229323080.1229327018.18968.gz
WINNT 5.2 mozilla-central moz2-win32-slave07 dep unit test on 2008/12/14 22:38:00

TinderboxPrint: browser<br/><em class="testfail">1239/0/5 LEAK</em>

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1229303626.1229307475.10785.gz
Linux comm-central dep unit test on 2008/12/14 17:13:46

TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 350823 bytes during test execution (threshold set at 8556 bytes)
but no "TinderboxPrint: ... LEAK".
}

Does "SeaMonkey" need to do/port something additional ? (maybe a simple restart ?)

Comment 12

[Robert Kaiser]

(In reply to comment #11)
> Does "SeaMonkey" need to do/port something additional ? (maybe a simple restart
> ?)

SeaMonkey needed an update of buildbotcustom for that, I've done that a few days ago and I just spotted a failure that states "5561/2/29 LEAK" on the Linux column, so this is done there as well.

Comment 13

[Serge Gautherie (:sgautherie)]

(In reply to comment #11)
> What is the current status ?

Jeff, any reason not to R/V.Fixed this bug ?

(In reply to comment #12)
> SeaMonkey needed an update of buildbotcustom for that, I've done that a few
> days ago and I just spotted a failure that states "5561/2/29 LEAK" on the Linux
> column, so this is done there as well.

Robert, I saw (you did) that too ;-)

Comment 14

[Jeff Walden [:Waldo]]

Yeah, I think we're finished here.

Comment 15

[Serge Gautherie (:sgautherie)]

V.Fixed, per waterfall output(s).