384339 - The check-interactive code executes head, tail scripts before the user runs

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

[Alex Vincent [:WeirdAl]]

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?

Comment 1

[Alex Vincent [:WeirdAl]]

Attachment: infrastructure patch

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.

Comment 2

[Dave Townsend [:mossop]]

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.

Comment 3

[Alex Vincent [:WeirdAl]]

I'm open to suggestions.

Comment 4

[Jeff Walden [:Waldo]]

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.

Comment 5

[Alex Vincent [:WeirdAl]]

Are you saying this bug is a WONTFIX?

Comment 6

[Jeff Walden [:Waldo]]

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.

Comment 7

[Alex Vincent [:WeirdAl]]

Attachment: patch, v2

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.

Comment 8

[Alex Vincent [:WeirdAl]]

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

Comment 9

[Rob Campbell [:rc] (:robcee)]

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.

Comment 10

[Mark Banner (:standard8)]

(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 11

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 270842 [details] [diff] [review]
patch, v2

passing review request over to waldo.

Comment 12

[Jeff Walden [: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.

Comment 13

[Håkan Waara]

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).

Comment 14

[Alex Vincent [:WeirdAl]]

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.

Comment 15

[Alex Vincent [:WeirdAl]]

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

Comment 16

[Mark Banner (:standard8)]

(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.

Comment 17

[Alex Vincent [:WeirdAl]]

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

Comment 18

[Rob Campbell [:rc] (:robcee)]

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.

Comment 19

[Alex Vincent [:WeirdAl]]

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.

Comment 20

[(not currently active) Ted Mielczarek]

(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.
 

Comment 21

[Rob Campbell [:rc] (:robcee)]

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!

Comment 22

[(not currently active) Ted Mielczarek]

(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.

Comment 23

[Håkan Waara]

(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?

Comment 24

[Alex Vincent [:WeirdAl]]

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

Comment 25

[Alex Vincent [:WeirdAl]]

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.

Comment 26

[Serge Gautherie (:sgautherie)]

Alex, ping ?

Comment 27

[Dan Mosedale (:dmosedale, :dmose)]

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...

Comment 28

[Mark Banner (:standard8)]

(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.

Comment 29

[Serge Gautherie (:sgautherie)]

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

Comment 30

[Alex Vincent [:WeirdAl]]

(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?

Comment 31

[(not currently active) Ted Mielczarek]

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.

Comment 32

[Mark Banner (:standard8)]

(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).

Comment 33

[(not currently active) Ted Mielczarek]

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."

Comment 34

[Serge Gautherie (:sgautherie)]

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

I'll do.

Comment 35

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) Move _execute_test() definition to head.js from tail.js [Checkin: Comment 36 & 37]

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

Comment 36

[Serge Gautherie (:sgautherie)]

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 37

[Serge Gautherie (:sgautherie)]

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

Comment 38

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1) Load tail files after test execution, as expected

[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 ;-)

Comment 39

[Mark Banner (:standard8)]

(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?

Comment 40

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1a) Load head, test and tail files from _execute_test()

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.

Comment 41

[Serge Gautherie (:sgautherie)]

Jeff, ping for review?

Comment 42

[Serge Gautherie (:sgautherie)]

Jeff, ping for review?

Comment 43

[Serge Gautherie (:sgautherie)]

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?

Comment 44

[Mark Banner (:standard8)]

(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.

Comment 45

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1b) Load head, test and tail files from _execute_test()

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 :-/

Comment 46

[Rob Campbell [:rc] (:robcee)]

Jeff, do you have time to review this?

Comment 47

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1c) Load head, test and tail files from _execute_test() [Checkin: Comment 48 & 49]

Dv1b, unbitrotted after bug 362433.

Comment 48

[Serge Gautherie (:sgautherie)]

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 49

[Serge Gautherie (:sgautherie)]

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

Comment 50

[Serge Gautherie (:sgautherie)]

(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
}