469523 - xpcshell-tests: enable leak log in tinderbox (log)

Status: RESOLVED DUPLICATE of bug 1341961

(Testing :: XPCShell Harness, enhancement)

Description

[Serge Gautherie (:sgautherie)]

Currently, I use
|env XPCOM_MEM_LEAK_LOG=1 make -C objdir check|
just fine on my local Windows, from a MSys prompt...

But I don't know how to properly add it to the tinderbox config.

I guess Linux and MacOSX "could" simply use |env XPCOM_MEM_LEAK_LOG=1 ...|;
but what about Windows, which needs a separate |set XPCOM_MEM_LEAK_LOG=1| command ?

NB: This bug focuses on generating the log asap, automated parsing it could be dealt with later.

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av0) Minimal detection script

This (minimal) script produces an output like
{
TEST-PASS | TUnit-leaks | no leaks detected!
}
or
{
...
TEST-UNEXPECTED-FAIL | TUnit-leaks | leaks from test_xtf\unit\test_xtf.js.log
>>>
[test_xtf.js.log content]
<<<
TEST-UNEXPECTED-FAIL | TUnit-leaks | 72 tests leak
}

It could be enhanced later, as needed,
but the main point of this bug is to get (asap) at least some kind of report.

If you agree with this approach, I'll look into how it can be hooked up to the test/tinderbox process.

Comment 2

[(not currently active) Ted Mielczarek]

Comment on attachment 354943 [details]
(Av0) Minimal detection script

How do you intend to have this used? Feels like we should put this in the test_{one,all} script. (Which I've wanted rewritten in Python anyway for a while.) Plus we could split the leak checking code out into a separate module and import it into both mochitest and the xpcshell harness.

Comment 3

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1-MC) Run it at end of 'rules.mk > check' global target

(In reply to comment #2)

I have noted your points, and I have others, like handling the C++ tests too,
but I would like to keep this (very) first step as simple/separated as possible, for now.

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 355768 [details] [diff] [review]
(Bv1-MC) Run it at end of 'rules.mk > check' global target

No, I'd rather have this called from the test_all script.

Comment 5

[Serge Gautherie (:sgautherie)]

(In reply to comment #4)
> I'd rather have this called from the test_all script.

pros: the leak log is immediately after the test run.
cons: I can't count the total number of leaking tests, can I ?

Comment 6

[(not currently active) Ted Mielczarek]

If you stick it at the bottom of this loop:
http://mxr.mozilla.org/mozilla-central/source/tools/test-harness/xpcshell-simple/test_all.sh#111

then you could run it after each individual test, which seems like the best possible way to do it.

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 354943 [details]
(Av0) Minimal detection script

I'd rather have Waldo look at this, since he wrote the leak detection for mochitest anyway. (Also, he's a stricter Python reviewer than I am!)

Comment 8

[Serge Gautherie (:sgautherie)]

Attachment: (Cv0) Add leak detection after each test run

ted, you wrote iirc that you preferred a test_*.sh based solution rather than one relying on rules.mk.

Here it is.

2 "ToDo"s in this patch
*Add a (= which ?) license to the Python script.
*Should the direct call to |python| be replaced (and how) by some "variable", like |$(PYTHON)| ?

***

We also agreed that this would mean that totalizing the number of leaking tests would then have to be done in the buildbot script, if wanted.
(And same, if we would want to add a threshold on that number.)

Whilst testing, I noticed that this can't "as easily" be extended to the CPP tests, can it ?

I understand your concern about my rules.mk approach and I prefer to have yours than nothing;
yet, I really have the feeling that this is rather limiting its features :-/

Comment 9

[Serge Gautherie (:sgautherie)]

Comment on attachment 356450 [details] [diff] [review]
(Cv0) Add leak detection after each test run


PS:
*Do s/+1,42/+1,43/ before applying this patch :-<
*This patch contains some nit fixes, synchronizations, etc, too :->

Comment 10

[Serge Gautherie (:sgautherie)]

(In reply to comment #8)
> yet, I really have the feeling that this is rather limiting its features :-/

In the future, I would also want to continue running the tests even when one fails, instead of exiting at the end of the "current" directory.
For this too, a global count of failing tests would be nice...

Comment 11

[(not currently active) Ted Mielczarek]

C++ unit tests (at least the ones using Waldo's CPP_UNIT_TESTS) are run via a central rule in rules.mk:
http://mxr.mozilla.org/mozilla-central/source/config/rules.mk#203

So you could easily add leak detection there. I agree about being able to run all the tests without stopping for failures, but that's beyond the scope of this bug. Basically, the way we do it (via a recursive 'check' rule) it just isn't possible at the moment to run all the tests and provide a summary at the end. We'll have to change the way tests are run, centralizing the test harness somehow. (Perhaps like Mochitest, providing an objdir/_tests/xpcshell/runtests.py or something.)

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 356450 [details] [diff] [review]
(Cv0) Add leak detection after each test run

Use the MPL tri-license on the code, I think that's easiest:
http://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-sh

+# Returns	boolean	Whether the test leaked or not.

The spacing there looks weird. Is that intentional?

checkLeak returns a boolean, but you don't use it in main(). Do you think it's necessary?

+  try:
+    logFullPath = sys.argv[1]
+  except:
+    print "TUnit-leaks, ERROR: missing (logFullPath) argument !"
+    return 1

I'd prefer to check len(sys.argv), and use sys.exit(1) if you don't have an argument. Also, print the error message to stderr, like:
print >>sys.stderr, "TUnit-leaks: ..."

+  checkLeak(logFullPath)
+  return 0

You don't need that return 0.

+# Enable leak log, to stderr.
+XPCOM_MEM_LEAK_LOG=2; export XPCOM_MEM_LEAK_LOG

This file is explicitly using bash, so just do this in one step:
export XPCOM_MEM_LEAK_LOG=2

+        # Remove all directories and separators: keep the filename only.
+        # Remove the fixed part of the path.

Is this a necessary change? What's the intent behind this?

I don't actually know the leak log format very well, so I'm going to tag Waldo to review that part of the Python script. With those nits addressed, r=me on the rest of it.

Comment 13

[Jeff Walden [:Waldo]]

Comment on attachment 356450 [details] [diff] [review]
(Cv0) Add leak detection after each test run

A general comment: the output cleanups generally look fine, but it's really much better to do that sort of thing in separate patches, at least when you're not doing a driveby one-liner (small) change.

Also, you really know way too many shell substringing tricks.  :-P

I take it the plan is to do this and then fix up tests incrementally until we can change main() to return int(checkLeaks())?


>+"""
>+Report leaks from TUnit tests.
>+
>+Serge Gautherie, 2009.01.11.
>+"""

Seems like this'd be better as a license header for the name part followed by a docstring for the description part.


>+# Check if a test leaked.
>+# If so, report it.
>+# Returns	boolean	Whether the test leaked or not.
>+def checkLeak(logFullPath):

Use a docstring for this rather than a comment, preserves association better.


>diff --git a/tools/test-harness/xpcshell-simple/test_all.sh b/tools/test-harness/xpcshell-simple/test_all.sh

>+# Enable leak log, to stderr.
>+XPCOM_MEM_LEAK_LOG=2; export XPCOM_MEM_LEAK_LOG

Comma there is unnecessary.


>diff --git a/tools/test-harness/xpcshell-simple/test_one.sh b/tools/test-harness/xpcshell-simple/test_one.sh

>+    # Remove ending dir separator, if there is one.

>+# Remove last dir separator and following file/dir name, if there is one.

>+    # By default, use 'unit' directory.

Don't need a comma here either.


>+python $topsrcdir/tools/test-harness/xpcshell-simple/TUnit-leaks.py \
>+    $testdir/$target_dir/$target_js.log

Put the path on the same line, please; I'm guessing this is over 80 characters, but I think it's probably better to do that in this case.

Comment 14

[(not currently active) Ted Mielczarek]

Comment on attachment 356450 [details] [diff] [review]
(Cv0) Add leak detection after each test run

I bitrotted the heck out of this in bug 482084.

On the plus side, now the harness is Python.

Comment 15

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1) Enable (bare) leak log after each test run

Comment 16

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1a) Enable (bare) leak log after each test run

Dv1, unbitrotted.

Comment 17

[(not currently active) Ted Mielczarek]

Comment on attachment 368851 [details] [diff] [review]
(Dv1a) Enable (bare) leak log after each test run

I'd prefer that you dump the leak log to a file, not to stdout, since when you actually want to parse it later, intermingling it with the test stdout doesn't feel like the right thing to do. Also, your logic feels a little tortured already due to the intermingling.

-      stdout, stderr = proc.communicate()
+      # |stderrLog == None| as |pstderr| was either |None| or redirected to |stdout|.
+      stdoutLog, stderrLog = proc.communicate()

Please don't rename these variables, there's no need.

Comment 18

[Serge Gautherie (:sgautherie)]

Attachment: (Dv2) Enable (bare) leak log after each test [Checkin: See comment 21+22 & 23]

Dv1a, with comment 17 suggestion(s).


(In reply to comment #17)
> I'd prefer that you dump the leak log to a file, not to stdout

Agreed, I did that only because you had removed the (individual) log files.

Comment 19

[(not currently active) Ted Mielczarek]

Comment on attachment 368962 [details] [diff] [review]
(Dv2) Enable (bare) leak log after each test
[Checkin: See comment 21+22 & 23]

+    raise Exception("No test dirs or test manifest specified!")

Could you fix this for me while you're here? I don't know why I wrote that like that, it's silly. Could you just make that a print >>sys.stderr, and return False?

+  leakLogFile = os.path.join(tempfile.gettempdir(), "runxpcshelltests_leaks.log")

You should save the result of tempfile.gettempdir() and remove it at the end. Alternately, you could just use NamedTemporaryFile() here, and it will auto-remove itself when it goes out of scope. (Doesn't really matter what the leak log filename is, right?)

Looks good otherwise.

Comment 20

[Serge Gautherie (:sgautherie)]

(In reply to comment #19)
> Could you just make that a print >>sys.stderr, and return False?

Done.

> +  leakLogFile = os.path.join(tempfile.gettempdir(),
> "runxpcshelltests_leaks.log")
> 
> You should save the result of tempfile.gettempdir() and remove it at the end.

Why should I remove that dir (not file), which I'm not creating ?

> Alternately, you could just use NamedTemporaryFile()

NamedTemporaryFile() actually creates/(opens)/deletes a file ... whereas we need a filename "only", that the application will create itself ... and that we have/want to (open)/remove ourselves afterward.

Comment 21

[Serge Gautherie (:sgautherie)]

Comment on attachment 368962 [details] [diff] [review]
(Dv2) Enable (bare) leak log after each test
[Checkin: See comment 21+22 & 23]


http://hg.mozilla.org/mozilla-central/rev/26dd2edb1be9
Dv2, with comment 20 suggestion(s),
plus 2 nits.

NB: After irc discussion with ted, the temp dir/file handling is just fine as it is ;->

Comment 22

[Serge Gautherie (:sgautherie)]

Comment on attachment 368962 [details] [diff] [review]
(Dv2) Enable (bare) leak log after each test
[Checkin: See comment 21+22 & 23]


http://hg.mozilla.org/mozilla-central/rev/46a0b3d6c672
(Ev1) Support xpcshell built without leak logging

Comment 23

[Serge Gautherie (:sgautherie)]

Comment on attachment 368962 [details] [diff] [review]
(Dv2) Enable (bare) leak log after each test
[Checkin: See comment 21+22 & 23]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e24545d950ef
(Dv2a) Enable (bare) leak log after each test
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/65d2878e86bb
(Ev1) Support xpcshell built without leak logging

Comment 24

[(not currently active) Ted Mielczarek]

Note that my fix for bug bug 483062 added a file "automationutils.py" which contains code that can be shared between runtests.py/runreftest.py/runxpcshelltests.py.

Comment 25

[Serge Gautherie (:sgautherie)]

Attachment: (Fv1) Move code to automationutils.py from automation.py.in [Checkin: Comment 46 & 48 & 54]

Per comment 24 ;-)

While there:
*Add license.
*Fix indentation.

No behavior changes.

Comment 26

[Serge Gautherie (:sgautherie)]

Comment on attachment 396112 [details] [diff] [review]
(Fv1) Move code to automationutils.py from automation.py.in
[Checkin: Comment 46 & 48 & 54]


http://hg.mozilla.org/mozilla-central/rev/f6bf83b50648

Comment 27

[Serge Gautherie (:sgautherie)]

Comment on attachment 396112 [details] [diff] [review]
(Fv1) Move code to automationutils.py from automation.py.in
[Checkin: Comment 46 & 48 & 54]


http://hg.mozilla.org/mozilla-central/rev/ada2316e491e
http://hg.mozilla.org/mozilla-central/rev/c708d5a7697f
{
Merge for "Backed out changeset: f6bf83b50648" of
Bug 469523 - xpcshell-tests: enable leak log in tinderbox (log);  (Fv1) Move code to automationutils.py from automation.py.in
which fails on Windows boxes (though works locally :-/)
}

***

{
Traceback (most recent call last):
  File "mochitest/runtests.py", line 17, in <module>
    from automationutils import addCommonOptions, processLeakLog
ImportError: cannot import name processLeakLog
}

Passes on Linux and MacOSX, passes for Windows reftests,
but fails for Windows mochitests:
helpwanted! :-<

Comment 28

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

I suspect what is happening here is a build system dependency problem, which only shows up on windows because nsinstall cannot use symlinks to point back to the source dir. There seems to be a lot of nsinstalling automationutils.py out of objdir locations rather than srcdir, so I'm guessing those objdir locations never got updated properly for the mochi-* suites.

I suggest you dig through a build log to verify that hypothese. You can also d/l
http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32-unittest/1251309031/
to inspect the assorted copies of automationutils.py - that was the first win32 unit build with your change.

Comment 29

[Serge Gautherie (:sgautherie)]

(In reply to comment #28)

> only shows up on windows because nsinstall cannot use symlinks to point back to

Right, Ted suggested it might be because I still use mozbuild 1.3 and not yet 1.4!
(I will have to actually try and upgrade soon...)

> the source dir. There seems to be a lot of nsinstalling automationutils.py out
> of objdir locations rather than srcdir, so I'm guessing those objdir locations
> never got updated properly for the mochi-* suites.

Fwiw, my local (clobber) objdir has 8 times this file, all identical.

> http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32-unittest/1251309031/

Right, didn't think about that :-/ Thanks!

Confirming: automationutils.py is in tests:
xpcshell & reftest: up to date :-)
mochitest: old file :-(

Comment 30

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

(In reply to comment #29)
> Fwiw, my local (clobber) objdir has 8 times this file, all identical.

And in the dep case ?

Comment 31

[Serge Gautherie (:sgautherie)]

(In reply to comment #28)
> I suggest you dig through a build log to verify that hypothese.

Fwiw, I don't have a local log to compare with, but
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1251308606.1251315641.21803.gz&fulltext=1
seems fine to me: I can find all the 8 paths and no error reported.

Comment 32

[Serge Gautherie (:sgautherie)]

(In reply to comment #30)
> (In reply to comment #29)
> > Fwiw, my local (clobber) objdir has 8 times this file, all identical.
> 
> And in the dep case ?

No idea: I'm not used to doing dep builds, as it never seemed to do me much good.
(Though I would so much like it would ... I'll see if I can try that...)

Comment 33

[Serge Gautherie (:sgautherie)]

(In reply to comment #31)
> (In reply to comment #28)
> > I suggest you dig through a build log to verify that hypothese.
> 
> Fwiw, I don't have a local log to compare with, but

I tried to compare with the following nightly
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1251280920.1251292108.3930.gz&fulltext=1
and it does look like the nightly did some initial nsinstall to the objdir, which the dep build miss. (as you suggested)

Comment 34

[Serge Gautherie (:sgautherie)]

(In reply to comment #32)
> No idea: I'm not used to doing dep builds, as it never seemed to do me much
> good.
> (Though I would so much like it would ... I'll see if I can try that...)

I clobbered and built previous rev, then just added my patch.

That's it: (with mozbuild 1.3)
*updated:
 layout/tools/reftest and _tests/reftest
*not updated:
 build _leaktest
 build/pgo _profile/pgo
 testing/mochitest _tests/testing/mochitest

Ted, helpwanted!

Comment 35

[(not currently active) Ted Mielczarek]

I'm in mountain view this week without access to my Windows machine, remind me on monday when I'm back home and I'll investigate.

Comment 36

[Serge Gautherie (:sgautherie)]

Ted, ping.

Comment 37

[(not currently active) Ted Mielczarek]

Sorry, got dragged off to a few other bugs, but this is on my TODO list.

Comment 38

[(not currently active) Ted Mielczarek]

Attachment: (Gv1) fix automationutils.py build dependencies [Checkin: See comment 45 & 47 & 53]

This seems to fix the problem. Silly mistake. Serge: try doing a dep build without your patch, apply this patch + your patch, then rebuild? It should fix things, seems to work ok here on Windows.

Comment 39

[(not currently active) Ted Mielczarek]

Serge: if that patch works for you, feel free to commit it along with your patch when you have time to re-land.

Comment 40

[Serge Gautherie (:sgautherie)]

First attempt at |make -C objdir| failed:
{
[...]
objdir/config/nsinstall [...]/build/automationutils.py ../../../_tests/reftest .
nsinstall: ..\..\..\_tests\reftest is a directory
make[1]: *** [objdir/layout/tools/reftest/automationutils.py] Error 3
}

Comment 41

[Serge Gautherie (:sgautherie)]

(In reply to comment #40)
> First attempt at |make -C objdir| failed:
> {
> [...]
> objdir/config/nsinstall [...]/build/automationutils.py ../../../_tests/reftest
> .
> nsinstall: ..\..\..\_tests\reftest is a directory
> make[1]: *** [objdir/layout/tools/reftest/automationutils.py] Error 3
> }

I have no idea where that '../../../_tests/reftest' comes from :-(

Comment 42

[Serge Gautherie (:sgautherie)]

Comment on attachment 398717 [details] [diff] [review]
(Gv1) fix automationutils.py build dependencies
[Checkin: See comment 45 & 47 & 53]


Same on Try, with just this patch:
{
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1252184341.1252188111.18360.gz
WINNT 5.2 try hg build on 2009/09/05 13:59:01

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1252184341.1252189441.783.gz
WINNT 5.2 try hg unit test on 2009/09/05 13:59:01
}

Comment 43

[Serge Gautherie (:sgautherie)]

(In reply to comment #42)
> (From update of attachment 398717 [details] [diff] [review])
> 
> Same on Try, with just this patch:

Ftr, it passed on Linux and MacOSX :-|

Comment 44

[(not currently active) Ted Mielczarek]

+	$(INSTALL) $^ .

Ah, crap, that $^ should be $<.

Comment 45

[Serge Gautherie (:sgautherie)]

Comment on attachment 398717 [details] [diff] [review]
(Gv1) fix automationutils.py build dependencies
[Checkin: See comment 45 & 47 & 53]


http://hg.mozilla.org/mozilla-central/rev/0c679f4535d6
Gv1, with comment 44 suggestion(s).

Comment 46

[Serge Gautherie (:sgautherie)]

Comment on attachment 396112 [details] [diff] [review]
(Fv1) Move code to automationutils.py from automation.py.in
[Checkin: Comment 46 & 48 & 54]


http://hg.mozilla.org/mozilla-central/rev/36cbdd2b247b

Comment 47

[Serge Gautherie (:sgautherie)]

Comment on attachment 398717 [details] [diff] [review]
(Gv1) fix automationutils.py build dependencies
[Checkin: See comment 45 & 47 & 53]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5dab33c22495

Comment 48

[Serge Gautherie (:sgautherie)]

Comment on attachment 396112 [details] [diff] [review]
(Fv1) Move code to automationutils.py from automation.py.in
[Checkin: Comment 46 & 48 & 54]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/fe408191c468

Comment 49

[(not currently active) Ted Mielczarek]

Did you skip the 1.9.2 branch with these?

Comment 50

[Serge Gautherie (:sgautherie)]

(In reply to comment #49)
> Did you skip the 1.9.2 branch with these?

Yes: all my patches are checkin-needed for 1.9.2...

Comment 51

[Serge Gautherie (:sgautherie)]

Attachment: (Hv1) Use automationutils.dumpLeakLog(), fix/update automationutils.processLeakLog() log messages [Wrong patch]

Rewrite part of bug 484298, per discussion there...

Comment 52

[Serge Gautherie (:sgautherie)]

Attachment: (Hv1) Use automationutils.dumpLeakLog(), fix/update automationutils.processLeakLog() log messages [Checkin: Comment 55 & 56 & 57]

With 'hg qref' :->

Comment 53

[Serge Gautherie (:sgautherie)]

Comment on attachment 398717 [details] [diff] [review]
(Gv1) fix automationutils.py build dependencies
[Checkin: See comment 45 & 47 & 53]


http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8899d4512ae2

Comment 54

[Serge Gautherie (:sgautherie)]

Comment on attachment 396112 [details] [diff] [review]
(Fv1) Move code to automationutils.py from automation.py.in
[Checkin: Comment 46 & 48 & 54]


http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e91c664f06eb

Comment 55

[Serge Gautherie (:sgautherie)]

Comment on attachment 398946 [details] [diff] [review]
(Hv1) Use automationutils.dumpLeakLog(), fix/update automationutils.processLeakLog() log messages
[Checkin: Comment 55 & 56 & 57]


http://hg.mozilla.org/mozilla-central/rev/bb40c09d164a

Comment 56

[Serge Gautherie (:sgautherie)]

Comment on attachment 398946 [details] [diff] [review]
(Hv1) Use automationutils.dumpLeakLog(), fix/update automationutils.processLeakLog() log messages
[Checkin: Comment 55 & 56 & 57]


http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d7a78b5730e0

Comment 57

[Serge Gautherie (:sgautherie)]

Comment on attachment 398946 [details] [diff] [review]
(Hv1) Use automationutils.dumpLeakLog(), fix/update automationutils.processLeakLog() log messages
[Checkin: Comment 55 & 56 & 57]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/56320c5427e3

Comment 58

[Serge Gautherie (:sgautherie)]

(In reply to comment #8)

The situation is that we have leaking tests:
*unfixable leaks: a few tests trigger (wontfix) bug 203764.
*persistent leaks: need a threshold until they're fixed.
*intermittent leaks: will be catched as failing orange.

> We also agreed that this would mean that totalizing the number of leaking tests
> would then have to be done in the buildbot script, if wanted.
> (And same, if we would want to add a threshold on that number.)

What do we want as a leak threshold?

Counting the number of leaking tests (in buildbot) seems the easiest but can fail to notice different cases (like increasing leaks or leak test changes).

I would suggest to add a --leak-threshold-file option to runxpcshelltests.py, and add files (per product+version+...) with lines like
"test_uriloader_exthandler/unit/test_punycodeURIs.js = 24"
to buildbot-config.
(runxpcshelltests.py would process the file much like it does the manifest.)

Ted, what do you think?

Comment 59

[(not currently active) Ted Mielczarek]

I think since we can detect leaks per-test, we should have a way to indicate a leak threshold per-test. Currently we're generating the test manifest on the fly from rules.mk, but we could store some information in a separate manifest, and indicate the leak threshold per-test there. Specific implementation details don't matter that much to me, but keeping the thresholds in the source tree makes it easy to change them as we fix things.

Comment 60

[Serge Gautherie (:sgautherie)]

Attachment: (Iv0-WIP) Use automationutils.processLeakLog(), Support leak thresholds

The python script works.
The Firefox threshold list needs to be completed before checkin.

Helpwanted on the config/makefile parts!

Comment 61

[(not currently active) Ted Mielczarek]

Comment on attachment 403398 [details] [diff] [review]
(Iv0-WIP) Use automationutils.processLeakLog(), Support leak thresholds

This looks like a good start, but not quite right. Some comments:

+# HELPWANTED: I don't know how to achieve such a behavior!
+# Copy this to js/src too.
+libs:: $(DEPTH)/$(project = browser or mail or suite or ...)/config/tests-leakthresholds.py
+	$(INSTALL) $(IFLAGS1) $^ $(testxpcobjdir)

If you want this to be configurable per-app, you should use $(topsrcdir)/$(MOZ_BUILD_APP), like how we include build.mk:
http://mxr.mozilla.org/mozilla-central/source/Makefile.in#72

Do you really expect them to vary that much per-app? If not, it might be simpler to just keep the global list in a file in testing/xpcshell. Alternately, you could support passing in more than one file, and have a global list, and a per-app list (which could override things, if needed).

+LEAK_THRESHOLD_FILE := $(DEPTH)/_tests/xpcshell/tests-leakthresholds.py
+# HELPWANTED: I don't know how to achieve such a behavior!
+if test -r $(LEAK_THRESHOLD_FILE) ; then \
+  LEAK_THRESHOLD_FILE := --leak-threshold-file=$(LEAK_THRESHOLD_FILE); \

You can do something like:
$(if $(wildcard $(LEAK_THRESHOLD_FILE),--leak-threshold-file=$(LEAK_THRESHOLD_FILE))
sort of like how we do QUOTED_WILDCARD:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/packager.mk#507

+    execfile(leakThresholdFile, dict(globals()), localsDict)

I'm not wild about this being a Python file that gets execed. Could you just make it a text file, with "filename:threshold", one test per line?

+      # Use absolute paths to match later |testfiles|.
+      # Do not use os.path.join() here.

Why aren't you using os.path.join? To avoid backslashes on Windows? You wind up replacing backslashes in the test filename later, so maybe just os.path.normalize() the path here, and then you can string compare them later.

+  if options.manifest and not os.path.exists(options.manifest):
+    print >>sys.stderr, "Error: %s does not exists!" % options.manifest
+    sys.exit(1)

exist, not exists, in the error message.

Comment 64

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

What remains to be done here?

Comment 65

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Now that we have manifests for xpcshell tests it should be pretty easy to annotate those that leak and make new leaks fatal.

Comment 66

[Mark Banner (:standard8)]

Attachment: WIP - Fail on unexpected leaks, but allow no-leaks

I think this might do what we want.

If it detects a leak, it'll shout TEST-UNEXPECTED-FAIL. However, if the xpcshell.ini file for example contains:

 [test_mailGlue_distribution.js]
+leak = bug 123456

Then it'll output TEST-KNOWN-FAIL with the bug number (assuming that's what was put in the attribute value).

Unfortunately I don't think we can determine debug versus opt as we don't have leak output in opt mode. So we'll have to go to use "known" for random oranges, and we won't be able to detect oranges that disappear.

Having said that, I think it's still a big improvement if we can at least detect new oranges.

If this looks about right, I'll do some runs through try server and see if we can get a list of the existing would-be-known oranges ready for landing this (once bug 723064 is fixed).

Comment 67

[(not currently active) Ted Mielczarek]

You should be able to tell if you're running in a debug build via mozinfo:
http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/runxpcshelltests.py#438
It should have a "debug" key.

Comment 68

[Mark Banner (:standard8)]

Attachment: WIP v2 - Fail on unexpected leaks, but allow no-leaks

Ok, now I know about debug, this is a slightly better version. I've changed the acceptable parameters to 'known' or 'random', and updated the output accordingly.

Comment 69

[(not currently active) Ted Mielczarek]

Do we want to allow specifying the actual leaked amounts, so that we can set thresholds and then lower them to zero, or are our leaks low enough that it doesn't matter that much?

Comment 70

[Mark Banner (:standard8)]

(In reply to Ted Mielczarek [:ted, :luser] from comment #69)
> Do we want to allow specifying the actual leaked amounts, so that we can set
> thresholds and then lower them to zero, or are our leaks low enough that it
> doesn't matter that much?

We could do, but I'm not sure it matters that much. I'd rather people just got this down to zero. In the Linux logs before bug 723064, there are 16 failures that are quite different, and a set of 26 failures all in the ipc tests (out of 1336). Whilst some of the 16 are in the same area, I suspect that they each one is caused by the same leak, rather than different leaks.

Comment 71

[Serge Gautherie (:sgautherie)]

Comment on attachment 593444 [details] [diff] [review]
WIP v2 - Fail on unexpected leaks, but allow no-leaks

Review of attachment 593444 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Mark Banner (:standard8) from comment #70)
> (In reply to Ted Mielczarek [:ted, :luser] from comment #69)
> > Do we want to allow specifying the actual leaked amounts, so that we can set
> > thresholds and then lower them to zero

We used to manage a (pass) threshold like that for mochitests and reftests.

> We could do, but I'm not sure it matters that much. I'd rather people just
> got this down to zero.

I think it would be better: having a reference threshold helps to notice (even if manually) when things improve (or get worse).
It should be simple to use "leak = <threshold>. Bug 123456", or two properties if need be.

::: build/automationutils.py
@@ +289,4 @@
>    return debuggerInfo
>  
>  
> +def dumpLeakLog(leakLogFile, leakExpected = False, filter = False):

New 'leakExpected' is unused (yet).

::: testing/xpcshell/runxpcshelltests.py
@@ +560,2 @@
>          for log in leakLogs:
> +          leakDetected = leakDetected or dumpLeakLog(log, True)

Can't processLeakLog() be reused (somehow), instead of "duplicating" its logic here? (See my patch Iv0-WIP.)

http://mxr.mozilla.org/mozilla-central/source/build/automationutils.py#408

@@ +565,5 @@
> +            if not 'leak' in test:
> +              self.log.error("TEST-UNEXPECTED-FAIL | %s | test leaked, see above log." % (name))
> +              self.failCount += 1
> +            elif test['leak'] == 'known' or test['leak'] == 'random':
> +              self.log.error("TEST-KNOWN-FAIL | %s | expected leak detected" % (name))

"KNOWN-FAIL / ToDo" are not supported for Xpcshell tests yet, are they?

http://mxr.mozilla.org/build/source/buildbotcustom/steps/unittest.py#160

Comment 72

[(not currently active) Ted Mielczarek]

Comment on attachment 593444 [details] [diff] [review]
WIP v2 - Fail on unexpected leaks, but allow no-leaks

Review of attachment 593444 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/xpcshell/runxpcshelltests.py
@@ +560,2 @@
>          for log in leakLogs:
> +          leakDetected = leakDetected or dumpLeakLog(log, True)

I agree with Serge here. We've got automationutils.processLeakLog, we might as well just use that.

Comment 73

[Mark Banner (:standard8)]

(In reply to Serge Gautherie (:sgautherie) from comment #71)
> @@ +565,5 @@
> > +            if not 'leak' in test:
> > +              self.log.error("TEST-UNEXPECTED-FAIL | %s | test leaked, see above log." % (name))
> > +              self.failCount += 1
> > +            elif test['leak'] == 'known' or test['leak'] == 'random':
> > +              self.log.error("TEST-KNOWN-FAIL | %s | expected leak detected" % (name))
> 
> "KNOWN-FAIL / ToDo" are not supported for Xpcshell tests yet, are they?
> 
> http://mxr.mozilla.org/build/source/buildbotcustom/steps/unittest.py#160

Correct, but that can be a follow-up bug. It doesn't stop us landing this bug - the todo lines just won't get out to tbpl, nothing major.

Comment 74

[Mark Banner (:standard8)]

(In reply to Serge Gautherie (:sgautherie) from comment #71)
> > We could do, but I'm not sure it matters that much. I'd rather people just
> > got this down to zero.
> 
> I think it would be better: having a reference threshold helps to notice
> (even if manually) when things improve (or get worse).
> It should be simple to use "leak = <threshold>. Bug 123456", or two
> properties if need be.

...

> ::: testing/xpcshell/runxpcshelltests.py
> @@ +560,2 @@
> >          for log in leakLogs:
> > +          leakDetected = leakDetected or dumpLeakLog(log, True)
> 
> Can't processLeakLog() be reused (somehow), instead of "duplicating" its
> logic here? (See my patch Iv0-WIP.)


What I didn't like about processLeakLog was:

- It doesn't account for when leaks are fixed, i.e. to ensure that the threshold gets set to zero.
- Hence it also doesn't account for random leaks.

We could probably fix that in follow-up bugs though, but we should probably get the manifest structure narrowed down in this bug if we want to support it.


Also just to note that I'm not going to have time to work on this for the next few weeks, though I'd really like to see it completed.

Comment 75

[Serge Gautherie (:sgautherie)]

(In reply to Mark Banner (:standard8) from comment #74)
> (In reply to Serge Gautherie (:sgautherie) from comment #71)
> > > We could do, but I'm not sure it matters that much. I'd rather people just
> > > got this down to zero.
> 
> What I didn't like about processLeakLog was:
> 
> - It doesn't account for when leaks are fixed, i.e. to ensure that the
> threshold gets set to zero.
> - Hence it also doesn't account for random leaks.

Well, initially you didn't care about thresholds, now you want to automatically handle fixed or random leaks too :-|

> We could probably fix that in follow-up bugs though, but we should probably

We managed without that for other suites and it might not be worth the effort for xpcshell either. (See your figures in comment 70.)
Yet, I agree we could.

> get the manifest structure narrowed down in this bug if we want to support
> it.

Nonetheless, even for humans, it can't hurt to record "random/perma, max-threshold, comment".

Comment 76

[Mark Banner (:standard8)]

(In reply to Serge Gautherie (:sgautherie) from comment #75)
> (In reply to Mark Banner (:standard8) from comment #74)
> > (In reply to Serge Gautherie (:sgautherie) from comment #71)
> > > > We could do, but I'm not sure it matters that much. I'd rather people just
> > > > got this down to zero.
> > 
> > What I didn't like about processLeakLog was:
> > 
> > - It doesn't account for when leaks are fixed, i.e. to ensure that the
> > threshold gets set to zero.
> > - Hence it also doesn't account for random leaks.
> 
> Well, initially you didn't care about thresholds, now you want to
> automatically handle fixed or random leaks too :-|

No. I cared about leak versus no leak, and random leak. That's what my patch handled. I didn't care about intermediate thresholds.

Comment 77

[Andrew McCreight [:mccr8]]

I'm going to forward-dupe this to bug 1341961 because there's a lot of comments in this bug, but I suspect that at this point any work would have to start from scratch.