Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

ASSIGNED
Assigned to

Status

Testing
XPCShell Harness
--
enhancement
ASSIGNED
9 years ago
5 years ago

People

(Reporter: sgautherie, Assigned: sgautherie)

Tracking

(Depends on: 3 bugs, Blocks: 5 bugs, {autotest-issue, fixed1.9.1})

Trunk
mozilla1.9.3a1
autotest-issue, fixed1.9.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Dv2a+Ev1: fixed1.9.1b4; Gv1a+Fv1, Hv1: fixed1.9.2b1 fixed1.9.1.4], URL)

Attachments

(8 attachments, 5 obsolete attachments)

981 bytes, text/plain
Details
1.16 KB, patch
ted
: review-
Details | Diff | Splinter Review
5.98 KB, patch
ted
: review+
Details | Diff | Splinter Review
20.02 KB, patch
ted
: review+
Details | Diff | Splinter Review
812 bytes, patch
bsmedberg
: review+
Details | Diff | Splinter Review
6.64 KB, patch
ted
: review+
Details | Diff | Splinter Review
15.32 KB, patch
ted
: review-
Details | Diff | Splinter Review
2.72 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

9 years ago
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.
(Assignee)

Comment 1

9 years ago
Created attachment 354943 [details]
(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.
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #354943 - Flags: review?
(Assignee)

Updated

9 years ago
Attachment #354943 - Flags: review? → review?(ted.mielczarek)
(Assignee)

Updated

9 years ago
Blocks: 471788
(Assignee)

Updated

9 years ago
Blocks: 469062
(Assignee)

Updated

9 years ago
Blocks: 470455
(Assignee)

Updated

9 years ago
Blocks: 453146
(Assignee)

Updated

9 years ago
Blocks: 454964
(Assignee)

Updated

9 years ago
Blocks: 470235
(Assignee)

Updated

9 years ago
Blocks: 470241
(Assignee)

Updated

9 years ago
Blocks: 470239
(Assignee)

Updated

9 years ago
Blocks: 470244
(Assignee)

Updated

9 years ago
Blocks: 464909
(Assignee)

Updated

9 years ago
Blocks: 462955
(Assignee)

Updated

9 years ago
Blocks: 454854
(Assignee)

Updated

9 years ago
Blocks: 454851
(Assignee)

Updated

9 years ago
Blocks: 465376
(Assignee)

Updated

9 years ago
Blocks: 449240
(Assignee)

Updated

9 years ago
Blocks: 464816
(Assignee)

Updated

9 years ago
Blocks: 464114
(Assignee)

Updated

9 years ago
Blocks: 324121
(Assignee)

Updated

9 years ago
Blocks: 471792
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.
(Assignee)

Comment 3

9 years ago
Created attachment 355768 [details] [diff] [review]
(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.
Attachment #355768 - Flags: review?(ted.mielczarek)
Attachment #355768 - Flags: review?(ted.mielczarek) → review-
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.
(Assignee)

Comment 5

9 years ago
(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 ?
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.
Attachment #354943 - Flags: review?(ted.mielczarek) → review?(jwalden+bmo)
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!)
(Assignee)

Comment 8

9 years ago
Created attachment 356450 [details] [diff] [review]
(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 :-/
Attachment #356450 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 9

9 years ago
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 :->
(Assignee)

Comment 10

9 years ago
(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...
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 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.
Attachment #356450 - Flags: review?(ted.mielczarek)
Attachment #356450 - Flags: review?(jwalden+bmo)
Attachment #356450 - Flags: review+
(Assignee)

Updated

9 years ago
Blocks: 473259
(Assignee)

Updated

9 years ago
Depends on: 468033
(Assignee)

Updated

9 years ago
Depends on: 446300
Attachment #354943 - Flags: review?(jwalden+bmo)
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.
Attachment #356450 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Updated

9 years ago
No longer depends on: 468033
(Assignee)

Updated

9 years ago
Blocks: 477927
(Assignee)

Updated

9 years ago
Blocks: 477930
(Assignee)

Updated

9 years ago
Severity: normal → enhancement
Keywords: autotest-issue
(Assignee)

Updated

9 years ago
Depends on: 384339
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.
Attachment #356450 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Depends on: 482084
(Assignee)

Comment 15

9 years ago
Created attachment 367141 [details] [diff] [review]
(Dv1) Enable (bare) leak log after each test run
Attachment #367141 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

8 years ago
Blocks: 484747
(Assignee)

Comment 16

8 years ago
Created attachment 368851 [details] [diff] [review]
(Dv1a) Enable (bare) leak log after each test run

Dv1, unbitrotted.
Attachment #367141 - Attachment is obsolete: true
Attachment #368851 - Flags: review?(ted.mielczarek)
Attachment #367141 - Flags: review?(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.
Attachment #368851 - Flags: review?(ted.mielczarek) → review-
(Assignee)

Comment 18

8 years ago
Created attachment 368962 [details] [diff] [review]
(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.
Attachment #368851 - Attachment is obsolete: true
Attachment #368962 - Flags: review?(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.
Attachment #368962 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 20

8 years ago
(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.
(Assignee)

Comment 21

8 years ago
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 ;->
Attachment #368962 - Attachment description: (Dv2) Enable (bare) leak log after each test → (Dv2) Enable (bare) leak log after each test [Checkin: See comment 21]
(Assignee)

Updated

8 years ago
Depends on: 485583
(Assignee)

Updated

8 years ago
Blocks: 485583
No longer depends on: 485583
(Assignee)

Updated

8 years ago
Blocks: 485648
(Assignee)

Comment 22

8 years ago
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
Attachment #368962 - Attachment description: (Dv2) Enable (bare) leak log after each test [Checkin: See comment 21] → (Dv2) Enable (bare) leak log after each test [Checkin: See comment 21+22]
(Assignee)

Comment 23

8 years ago
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
Attachment #368962 - Attachment description: (Dv2) Enable (bare) leak log after each test [Checkin: See comment 21+22] → (Dv2) Enable (bare) leak log after each test [Checkin: See comment 21+22 + 23]
(Assignee)

Updated

8 years ago
Keywords: fixed1.9.1
Whiteboard: [Leave open till fully resolved] [fixed1.9.1b4: Dv2a, Ev1]
Target Milestone: --- → mozilla1.9.2a1
(Assignee)

Updated

8 years ago
Blocks: 484579
(Assignee)

Updated

8 years ago
Blocks: 489078
(Assignee)

Updated

8 years ago
No longer blocks: 484579
(Assignee)

Updated

8 years ago
Blocks: 490251
(Assignee)

Updated

8 years ago
Blocks: 472773
(Assignee)

Updated

8 years ago
Blocks: 490255
(Assignee)

Updated

8 years ago
No longer blocks: 490255
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.
(Assignee)

Updated

8 years ago
Depends on: 495730
(Assignee)

Updated

8 years ago
Blocks: 498147
No longer blocks: 465376
(Assignee)

Updated

8 years ago
No longer blocks: 462955
(Assignee)

Updated

8 years ago
No longer blocks: 485648
No longer blocks: 498147
(Assignee)

Updated

8 years ago
No longer blocks: 490251
(Assignee)

Updated

8 years ago
Blocks: 511735
(Assignee)

Comment 25

8 years ago
Created attachment 396112 [details] [diff] [review]
(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.
Attachment #396112 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

8 years ago
Depends on: 483062
(Assignee)

Updated

8 years ago
Attachment #396112 - Attachment description: (Ev1) Move code to automationutils.py from automation.py.in → (Fv1) Move code to automationutils.py from automation.py.in
(Assignee)

Updated

8 years ago
No longer blocks: 453146
(Assignee)

Updated

8 years ago
Summary: Enable TUnit leak log in tinderbox (log) → xpcshell-tests: enable leak log in tinderbox (log)
Attachment #396112 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 26

8 years ago
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
Attachment #396112 - Attachment description: (Fv1) Move code to automationutils.py from automation.py.in → (Fv1) Move code to automationutils.py from automation.py.in [Checkin: Comment 26]
(Assignee)

Updated

8 years ago
Attachment #368962 - Attachment description: (Dv2) Enable (bare) leak log after each test [Checkin: See comment 21+22 + 23] → (Dv2) Enable (bare) leak log after each test [Checkin: See comment 21+22 & 23]
(Assignee)

Updated

8 years ago
Whiteboard: [Leave open till fully resolved] [fixed1.9.1b4: Dv2a, Ev1] → [c-n: Fv1 to m-1.9.2] [Leave open till fully resolved] [fixed1.9.1b4: Dv2a, Ev1]
Target Milestone: mozilla1.9.2a1 → mozilla1.9.3a1
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Whiteboard: [c-n: Fv1 to m-1.9.2] [Leave open till fully resolved] [fixed1.9.1b4: Dv2a, Ev1] → [c-n: Fv1 to m-1.9.2 (and m-1.9.1 but depends on bug 483062 landing first)] [Leave open till fully resolved] [fixed1.9.1b4: Dv2a, Ev1]
(Assignee)

Comment 27

8 years ago
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! :-<
Attachment #396112 - Attachment description: (Fv1) Move code to automationutils.py from automation.py.in [Checkin: Comment 26] → (Fv1) Move code to automationutils.py from automation.py.in [Backout: Comment 27]
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
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.
(Assignee)

Comment 29

8 years ago
(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 :-(
(In reply to comment #29)
> Fwiw, my local (clobber) objdir has 8 times this file, all identical.

And in the dep case ?
(Assignee)

Comment 31

8 years ago
(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.
(Assignee)

Comment 32

8 years ago
(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...)
(Assignee)

Comment 33

8 years ago
(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)
(Assignee)

Comment 34

8 years ago
(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!
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.
(Assignee)

Updated

8 years ago
Whiteboard: [c-n: Fv1 to m-1.9.2 (and m-1.9.1 but depends on bug 483062 landing first)] [Leave open till fully resolved] [fixed1.9.1b4: Dv2a, Ev1] → [c-n (but not yet!): Fv1 to m-1.9.2] [fixed1.9.1b4: Dv2a, Ev1]
(Assignee)

Comment 36

8 years ago
Ted, ping.
Sorry, got dragged off to a few other bugs, but this is on my TODO list.
Created attachment 398717 [details] [diff] [review]
(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.
Attachment #398717 - Flags: review?(benjamin)
Attachment #398717 - Flags: review?(benjamin) → review+
Serge: if that patch works for you, feel free to commit it along with your patch when you have time to re-land.
(Assignee)

Comment 40

8 years ago
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
}
(Assignee)

Comment 41

8 years ago
(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 :-(
(Assignee)

Comment 42

8 years ago
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
}
Attachment #398717 - Attachment is obsolete: true
(Assignee)

Comment 43

8 years ago
(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 :-|
+	$(INSTALL) $^ .

Ah, crap, that $^ should be $<.
(Assignee)

Comment 45

8 years ago
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).
Attachment #398717 - Attachment description: fix automationutils.py build dependencies → (Gv1) fix automationutils.py build dependencies [Checkin: See comment 45]
Attachment #398717 - Attachment is obsolete: false
(Assignee)

Comment 46

8 years ago
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
Attachment #396112 - Attachment description: (Fv1) Move code to automationutils.py from automation.py.in [Backout: Comment 27] → (Fv1) Move code to automationutils.py from automation.py.in [Checkin: Comment 46]
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Whiteboard: [c-n (but not yet!): Fv1 to m-1.9.2] [fixed1.9.1b4: Dv2a, Ev1] → [c-n: Gv1a+Fv1 to m-1.9.2] [fixed1.9.1b4: Dv2a, Ev1]
(Assignee)

Comment 47

8 years ago
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
Attachment #398717 - Attachment description: (Gv1) fix automationutils.py build dependencies [Checkin: See comment 45] → (Gv1) fix automationutils.py build dependencies [Checkin: See comment 45 & 47]
(Assignee)

Comment 48

8 years ago
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
Attachment #396112 - Attachment description: (Fv1) Move code to automationutils.py from automation.py.in [Checkin: Comment 46] → (Fv1) Move code to automationutils.py from automation.py.in [Checkin: Comment 46 & 48]
Did you skip the 1.9.2 branch with these?
(Assignee)

Comment 50

8 years ago
(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...
(Assignee)

Updated

8 years ago
Depends on: 484298
(Assignee)

Comment 51

8 years ago
Created attachment 398945 [details] [diff] [review]
(Hv1) Use automationutils.dumpLeakLog(), fix/update automationutils.processLeakLog() log messages
[Wrong patch]

Rewrite part of bug 484298, per discussion there...
Attachment #398945 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 52

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

With 'hg qref' :->
Attachment #398946 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

8 years ago
Attachment #398945 - Attachment description: (Hv1) Use automationutils.dumpLeakLog(), fix/update automationutils.processLeakLog() log messages → (Hv1) Use automationutils.dumpLeakLog(), fix/update automationutils.processLeakLog() log messages [Wrong patch]
Attachment #398945 - Attachment is obsolete: true
Attachment #398945 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 53

8 years ago
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
Attachment #398717 - Attachment description: (Gv1) fix automationutils.py build dependencies [Checkin: See comment 45 & 47] → (Gv1) fix automationutils.py build dependencies [Checkin: See comment 45 & 47 & 53]
(Assignee)

Comment 54

8 years ago
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
Attachment #396112 - Attachment description: (Fv1) Move code to automationutils.py from automation.py.in [Checkin: Comment 46 & 48] → (Fv1) Move code to automationutils.py from automation.py.in [Checkin: Comment 46 & 48 & 54]
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Whiteboard: [c-n: Gv1a+Fv1 to m-1.9.2] [fixed1.9.1b4: Dv2a, Ev1] → [Dv2a+Ev1: fixed1.9.1b4; Gv1a+Fv1: fixed1.9.2b1 fixed1.9.1.4]
Attachment #398946 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 55

8 years ago
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
Attachment #398946 - Attachment description: (Hv1) Use automationutils.dumpLeakLog(), fix/update automationutils.processLeakLog() log messages → (Hv1) Use automationutils.dumpLeakLog(), fix/update automationutils.processLeakLog() log messages [Checkin: Comment 55]
(Assignee)

Comment 56

8 years ago
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
Attachment #398946 - Attachment description: (Hv1) Use automationutils.dumpLeakLog(), fix/update automationutils.processLeakLog() log messages [Checkin: Comment 55] → (Hv1) Use automationutils.dumpLeakLog(), fix/update automationutils.processLeakLog() log messages [Checkin: Comment 55 & 56]
(Assignee)

Updated

8 years ago
Whiteboard: [Dv2a+Ev1: fixed1.9.1b4; Gv1a+Fv1: fixed1.9.2b1 fixed1.9.1.4] → [Dv2a+Ev1: fixed1.9.1b4; Gv1a+Fv1: fixed1.9.2b1 fixed1.9.1.4; Hv1: fixed1.9.2b1]
(Assignee)

Comment 57

8 years ago
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
Attachment #398946 - Attachment description: (Hv1) Use automationutils.dumpLeakLog(), fix/update automationutils.processLeakLog() log messages [Checkin: Comment 55 & 56] → (Hv1) Use automationutils.dumpLeakLog(), fix/update automationutils.processLeakLog() log messages [Checkin: Comment 55 & 56 & 57]
(Assignee)

Updated

8 years ago
Whiteboard: [Dv2a+Ev1: fixed1.9.1b4; Gv1a+Fv1: fixed1.9.2b1 fixed1.9.1.4; Hv1: fixed1.9.2b1] → [Dv2a+Ev1: fixed1.9.1b4; Gv1a+Fv1, Hv1: fixed1.9.2b1 fixed1.9.1.4]
(Assignee)

Comment 58

8 years ago
(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?
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.
(Assignee)

Comment 60

8 years ago
Created attachment 403398 [details] [diff] [review]
(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!
Attachment #403398 - Flags: review?(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.
Attachment #403398 - Flags: review?(ted.mielczarek) → review-
(Assignee)

Updated

7 years ago
Blocks: 560420
Duplicate of this bug: 599716
Duplicate of this bug: 606067
(Assignee)

Updated

7 years ago
Blocks: 605908
(Assignee)

Updated

7 years ago
Blocks: 605910
(Assignee)

Updated

7 years ago
Blocks: 605915
(Assignee)

Updated

7 years ago
Blocks: 605726
(Assignee)

Updated

7 years ago
Blocks: 605732
(Assignee)

Updated

7 years ago
Blocks: 605871
(Assignee)

Updated

7 years ago
Blocks: 605884
(Assignee)

Updated

7 years ago
Blocks: 605900
(Assignee)

Updated

7 years ago
Blocks: 532556
(Assignee)

Updated

7 years ago
Blocks: 605913
(Assignee)

Updated

7 years ago
Blocks: 599713

Updated

7 years ago
No longer blocks: 605913
Depends on: 605913

Updated

7 years ago
No longer blocks: 605910
Depends on: 605910

Updated

7 years ago
Depends on: 605912

Updated

7 years ago
No longer blocks: 605915
Depends on: 605915

Updated

7 years ago
No longer blocks: 605732
Depends on: 605732

Updated

7 years ago
No longer blocks: 605871, 605900
Depends on: 605871, 605900

Updated

7 years ago
No longer blocks: 532556
Depends on: 532556

Updated

7 years ago
No longer blocks: 599713, 605726
Depends on: 599713, 605726

Updated

7 years ago
No longer blocks: 605884
Depends on: 605884
What remains to be done here?
Now that we have manifests for xpcshell tests it should be pretty easy to annotate those that leak and make new leaks fatal.
Created attachment 593400 [details] [diff] [review]
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).
Attachment #593400 - Flags: feedback?(khuey)
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.
Created attachment 593444 [details] [diff] [review]
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.
Attachment #593400 - Attachment is obsolete: true
Attachment #593444 - Flags: feedback?(ted.mielczarek)
Attachment #593400 - Flags: feedback?(khuey)
Depends on: 723064

Updated

6 years ago
No longer depends on: 723064
Depends on: 723064
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?
(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.
(Assignee)

Comment 71

6 years ago
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 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.
Attachment #593444 - Flags: feedback?(ted.mielczarek) → feedback-
(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.
(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.
(Assignee)

Comment 75

5 years ago
(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".
(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.
You need to log in before you can comment on or make changes to this bug.