Spin off of bug 382752. In that bug, I realized there's a subtle flaw where the tail_extensionmanager.js code executes before the user can run the test. From the other bug: "One solution to this would be to wrap this code in a function that run_test or one of its subsidiaries calls. A better one would be to define in head.js a couple arrays to store functions that _execute_test() must call on." Right now, there's about a dozen head_*.js files that this bug would affect, and three tail_*.js files. I can also write a testcase which demonstrates this problem. Opinions on which solution to take?

Fixes to individual head, tail files I'd like to write up as we go. This patch simply adds the ability to handle the changes in head.js, tail.js.

FWIW if I'm reading your suggestions right and you plan to change every head and tail js test file then I don't think it's the right way to go about this.

I'm open to suggestions.

Comment on attachment 268276 [details] [diff] [review] infrastructure patch This flat-out isn't happening; I'll save robcee the trouble of looking at it.

Are you saying this bug is a WONTFIX?

No. I'm saying the suggested solution to it is the wrong one, and I *very* much doubt this particular problem can't be solved without modifying all the head/tail-using tests.

I realized a much better way to fix the bug. Here, the scripts are loaded via the subscript loader (instead of at the command-line), when the user calls _execute_test(). You can only run _execute_test() once per xpcshell instance, but given the intent of check-interactive (to allow attaching a debugger), that's fine imho. I have changed the sequencing slightly so that test-specific tail scripts load before the tail.js script.

rob: any plans to review this patch? It's been in your queue for over six months now.

whups. I kind of dropped this earlier and meant to get back to it. Alex: Do you know if this patch is still relevant since the changes to check-interactive were landed? see bug 416388. If so, I'll get back to taking a look at this.

(In reply to comment #9) > Alex: Do you know if this patch is still relevant since the changes to > check-interactive were landed? I can vouch for this still being a problem on today's cvs trunk. We could really do with a fix for this because its going to make debugging mailnews tests really hard (we need to clean up after the majority of tests).

Comment on attachment 270842 [details] [diff] [review] patch, v2 passing review request over to waldo.

Comment on attachment 270842 [details] [diff] [review] patch, v2 I think there's a load() builtin shell function which you can/should use instead of the subscript loader. Also, please wrap each load in a try/catch and ensure test failure if a throw happens; I don't recall whether a throw would properly terminate it here or not.

Alex, any plans to update this patch soon? We're gonna have to work around the bug in all tests in mailnews (which, luckily aren't *that* many - yet).

hwaara - I haven't been nearly as active this past year in Moz development as I had hoped. Even though it probably wouldn't take me that long to do it, I would probably need some nagging by e-mail to do it. Just too many things on my plate - in and out of Mozilla. Your best bet is on a weekend. :) Don't bother nagging on the bug itself - just send me a direct e-mail.

Also, what branches do you want this on? That may trivially affect my work (1.9.0.x branch - CVS, 1.9.1 - hg).

(In reply to comment #15) > Also, what branches do you want this on? That may trivially affect my work > (1.9.0.x branch - CVS, 1.9.1 - hg). > definitely hg. Possibly cvs pending the current discussions.

Waldo: a quick test shows that if the script file doesn't exist, load() doesn't throw... is that a problem?

It's definitely not good, though probably not considered show-stopping. If you can tighten up that code, feel free to drop a patch. Also, Waldo's going to be hard to reach for the next month or so.

robcee: I'll probably post a patch after bug 443220 gets fixed. Right now, any patch I'd post would conflict with that bug and would need redoing.

(In reply to comment #18) > Also, Waldo's going to be hard to reach for the next month or so. Next 5 months or so, AFAIK.

Awhile yeah. Last update he still had a couple thousand miles to go(!). (In reply to comment #19) > robcee: I'll probably post a patch after bug 443220 gets fixed. Right now, any > patch I'd post would conflict with that bug and would need redoing. Understood. Thanks for staying with this stuff!

(In reply to comment #17) > Waldo: a quick test shows that if the script file doesn't exist, load() doesn't > throw... is that a problem? hwaara was poking at the xpcshell load() code recently, iirc.

(In reply to comment #17) > Waldo: a quick test shows that if the script file doesn't exist, load() doesn't > throw... is that a problem? That's bug 439110 which was checked in, but then backed out due to test failures. I think that bug is not a showstopper for this bug though. Am I wrong?

Whoops, I dropped the ball on this. (There was a vacation in between.) I may need weekly reminders.

Requesting wanted1.9.1 for the mailnews testing requirement, and also in case someone wants to pick up this bug before I post another patch.

Alex, ping ?

Standard8: it doesn't look to me like this is specifically tied to shipping Thunderbird 3. Is it? If not, I'd suggest removing the [tb3needs] flag...

(In reply to comment #27) > Standard8: it doesn't look to me like this is specifically tied to shipping > Thunderbird 3. Is it? If not, I'd suggest removing the [tb3needs] flag... True, it doesn't block us for the release. It does however mean we can't implement tidy up on shutdown (without upsetting developers) which in turn means we can't use unit tests as an additional tool to check for potential memory leaks.

Note 'Bug 482084 - rewrite xpcshell test harness' landed.

(In reply to comment #29) > Note 'Bug 482084 - rewrite xpcshell test harness' landed. I don't know Python. Is there someone here who can take this bug over?

Could we solve this simply by not loading the tail files at all in interactive mode? I mean, if you're debugging there's a decent chance you might not let it finish anyway. The tail scripts are likely to just be cleanup anyway.

(In reply to comment #31) > Could we solve this simply by not loading the tail files at all in interactive > mode? I mean, if you're debugging there's a decent chance you might not let it > finish anyway. The tail scripts are likely to just be cleanup anyway. True, but is there a chance folks will want them for attaching the debugger and looking at leaks on exit? I'm thinking this may be true in mailnews where we need to do clean up to avoid some leaks on exit (which we'll need for TUnit leak checking).

Ok, my other thought then is that we could make interactive mode not load the scripts with -f, but instead pass something like: -e "function _load_tail_files() { load('/full/path/to/tail_bar.js'); load('/full/path/to/tail_foo.js'); }", and the info message for interactive mode say something like: "Type _execute_test(); to run the test. When finished, type _load_tail_files(); to run cleanup code."

(In reply to comment #30) > Is there someone here who can take this bug over? I'll do.

Bug 382682 changed this inline code into a function, so there is no need for it to live in tail.js anymore.

Comment on attachment 367520 [details] [diff] [review] (Cv1) Move _execute_test() definition to head.js from tail.js [Checkin: Comment 36 & 37] http://hg.mozilla.org/mozilla-central/rev/1752f796d0aa

Comment on attachment 367520 [details] [diff] [review] (Cv1) Move _execute_test() definition to head.js from tail.js [Checkin: Comment 36 & 37] http://hg.mozilla.org/releases/mozilla-1.9.1/rev/eaf873bfd86b

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090402 Minefield/3.6a1pre] (home, optim default) (W2Ksp4) (http://hg.mozilla.org/mozilla-central/rev/c4f6bb1db611) [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090402 SeaMonkey/2.0b1pre] (experimental/_m-c_, home, optim default) (W2Ksp4) (http://hg.mozilla.org/mozilla-central/rev/c4f6bb1db611 +http://hg.mozilla.org/comm-central/rev/cdfcaddd6832) I did not do it for the head files, as I considered the main need is to not execute the tail files before the test, not to debug heads and tails this way. (This could easily be added if need be.) This is "patch, v2" rewritten in python and more ;-)

(In reply to comment #38) > Created an attachment (id=371184) [details] > (Dv1) Load tail files after test execution, as expected This patch looks really good and will finally enable a good solution for getting mailnews xpcshell test shutting down cleanly and keeping developers happy... Jeff, any chance of a review soon?

Dv1, with your suggestions from your recent irc review. "... load is variadic or not, but fileList.forEach(load) ..." It seems it is not: I tried it and got |testing\xpcshell\head.js:124: Error: cannot open file '0' for reading| As the test head files moved, the test file had to too.

Jeff, ping for review?

Jeff, ping for review?

Comment on attachment 374604 [details] [diff] [review] (Dv1a) Load head, test and tail files from _execute_test() Jeff reviewed the first patch over irc. Rob, would you take over the new patch review?

(In reply to comment #40) > Created an attachment (id=374604) [details] > (Dv1a) Load head, test and tail files from _execute_test() This seems to have bitrotted, I can't apply to trunk or branch at the moment.

Dv1a, unbitrotted. Jeff, Rob, this is still very much wanted asap, as, for example, it blocks bug 469523 which prevents catching leak regressions on a timely manner :-/

Jeff, do you have time to review this?

Dv1b, unbitrotted after bug 362433.

Comment on attachment 381678 [details] [diff] [review] (Dv1c) Load head, test and tail files from _execute_test() [Checkin: Comment 48 & 49] http://hg.mozilla.org/mozilla-central/rev/eaec174cb9c2

Comment on attachment 381678 [details] [diff] [review] (Dv1c) Load head, test and tail files from _execute_test() [Checkin: Comment 48 & 49] http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a08e2e4571d6

(In reply to comment #49) > (From update of attachment 381678 [details] [diff] [review]) > > http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a08e2e4571d6 After fixing context for { patching file testing/xpcshell/head.js Hunk #1 FAILED at 64 1 out of 3 hunks FAILED patching file testing/xpcshell/runxpcshelltests.py Hunk #1 FAILED at 71 1 out of 3 hunks FAILED }