-d passed for xpcshell tests that do not need -d

RESOLVED FIXED in mozilla15

Status

RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: luke, Assigned: sfink)

Tracking

unspecified
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
Running all xpcshell tests (make xpcshell-tests) passes '-d' to xpcshell while running single tests or subsets of tests does not pass -d.  Whenever a developer gets a failure from a test suite, the first thing they always want to do is repro the failure by running the single test.  In my case, the failure required '-d' so I was pretty frustrated when I couldn't reproduce (esp. considering that each xpcshell test is run by itself).  This bug is to pass '-d' always.
You mean "-d" for "enable JSDebugger"? AFAICT we only pass that when a test has "debug = True":
http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/runxpcshelltests.py#651
(Reporter)

Comment 2

7 years ago
(Oops, sorry I missed your response until now.)

I don't know what '-d' really means, but what I do know is that I printed out argv in xpcshell's main() and '-d' is the only difference between the arguments passed to single-test and 'make xpcshell-tests' runs.

The reason I filed this is that I had a failure that only reproduced when I ran the whole test-suite and it took me a while to figure out that running a single test wasn't passing '-d'.  In the end I just hacked runxpcshelltests.py to always pass -d.  Being able to reproduce a failure by running a single test is really important to developers and speeds up debugging immensely.  Reproducibility with xpcshell tests is generally quite good because each tests runs in its own process, so I really hope we can smooth out this last hole.
I agree with all your assertions, I'm just trying to figure out the root cause here. AFAICT we only want to pass that flag for tests that request it (since it enables JSD). If we're passing it otherwise that seems like a bug.
(Reporter)

Comment 4

7 years ago
Ah.  I wonder if it goes something like this: the test harness sets the flag if it intends to run any test that needs jsd.  When you run a single test, it doesn't find any, so doesn't set the flag.  When you 'make xpcshell-tests', it finds the jsd xpcshell tests and, instead of setting -d *just for the jsd tests*, it sets them for all tests.
(Reporter)

Comment 5

7 years ago
Steve may know something about this!
(Reporter)

Comment 6

7 years ago
For posterity, from irc with sfink, running the command: 

  python -u ../config/pythonpath.py  -I../build  ../testing/xpcshell/runxpcshelltests.py  --build-info=./mozinfo.json  dist/bin/xpcshell  _tests/xpcshell/js/xpconnect/tests/unit --test-path=test_bug604362.js

does not pass -d (the test doesn't want it), but 'make xpcshell-tests' passes -d for this test (which is the bug, I think).
(Reporter)

Comment 7

7 years ago
And I see the confusion, my bug title was backwards (as was, probably, my understanding when filing :).
Summary: can we pass -d when running a single test or a single directory of tests? → -d passed for xpcshell tests that do not need -d
Oh! I know what goes wrong here. We use "debug" to mean "this is a debug build", so that other debug key is totally wrong. We need to rename that to "jsd" or something.
Actually, now that I think about it I'm totally not sure about that.
(Assignee)

Comment 10

7 years ago
I added the debug stuff in bug 692722. Note that it is really about SM's debugMode, not JSD in particular, and right now I expect gps will be using it for tests using the new debugger API.
(Assignee)

Comment 11

7 years ago
Actually, bug 692987 would be a better pointer, since that's about adding it as a manifest option.
(Assignee)

Comment 12

7 years ago
Created attachment 620428 [details] [diff] [review]
Only use -d for tests that want it

I'm still trying to reproduce (I have a run going that probably will, but it hasn't made it to js/jsd yet), but if I were to take a guess, I'd say this patch should fix it. I'll mark it for r? when I can verify that.
Oh, that seems much more likely!
(Assignee)

Comment 14

7 years ago
Comment on attachment 620428 [details] [diff] [review]
Only use -d for tests that want it

Yep, it appeared to work. With the patch, I saw no -d, then -d for the jsd tests, then back to no -d.
Attachment #620428 - Flags: review?(ted.mielczarek)
Attachment #620428 - Flags: review?(ted.mielczarek) → review+
Assignee: nobody → sphink
(Assignee)

Comment 15

7 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ab010583f6f
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/3ab010583f6f
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.