502909 - Stop using tail_ files in xpcshell: use do_register_cleanup() instead, in Toolkit Places

Status: VERIFIED INVALID

(Toolkit :: Places, defect)

Description

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Because tail_ files are ran right after run_test is called, the contents of those files may run before the test is actually done.  This is very bad.

Comment 1

[Marco Bonardo [:mak]]

do you mean tails do not take in cound do_test_pending? sounds like a bug that should be fixed in tests harness, unless something prevents that.

Comment 2

[Shawn Wilsher :sdwilsh]

can't be fixed there because tests rely on it.  tails run right after run_test is called.

Comment 3

[Marco Bonardo [:mak]]

it is still a bug if it does not take in count do_pending_test... any test relying on such a behavior would be crazy. The patch looks correct, but i'll wait for Ted to comment on the dependency.

Comment 4

[Shawn Wilsher :sdwilsh]

(In reply to comment #3)
> it is still a bug if it does not take in count do_pending_test... any test
> relying on such a behavior would be crazy. The patch looks correct, but i'll
> wait for Ted to comment on the dependency.
No, the intent was that it was supposed to run in order - head_*, test_*, tail_*.  That's why I filed bug 502907 to add the cleanup method registration that allows us to run after all do_test_pending calls.

Comment 5

[Marco Bonardo [:mak]]

(In reply to comment #4)
> No, the intent was that it was supposed to run in order - head_*, test_*,
> tail_*. 

indeed, the harness should wait for EVERYTHING one could put in test_* (included pending things) before executing tail_*, that's the bug i'm talking about.

Comment 6

[Shawn Wilsher :sdwilsh]

but it cannot because tests rely on that behavior.  That's why ted and I agreed we need some function like this (and why I filed and patched bug 502907 which adds this API)

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 387259 [details] [diff] [review]
v1.0

at this point, looking at discussion in bug 502907, the do_register_cleanup should go in head_, and all tail_ files should be discarded. That will even simplify management since all common code for all tests will live in a single file.

Comment 8

[Serge Gautherie (:sgautherie)]

(In reply to comment #4)
> No, the intent was that it was supposed to run in order - head_*, test_*,
> tail_*.

I'm very confused by your comments here:
this issue was (fixed by) bug 384339 (a month ago), wasn't it?


(In reply to comment #7)
> the do_register_cleanup should go in head_,
> and all tail_ files should be discarded.

Indeed!
I'm "morphing" this bug per bug 503613.

Nit: you may want to add names to anonymous functions.

Comment 9

[Shawn Wilsher :sdwilsh]

(In reply to comment #8)
> I'm very confused by your comments here:
> this issue was (fixed by) bug 384339 (a month ago), wasn't it?
Apparently, but it was landed with little fanfare, so I wasn't aware that it had changed.

Comment 10

[Serge Gautherie (:sgautherie)]

(In reply to comment #9)
> Apparently, but it was landed with little fanfare,

Well, the "fanfare" was on MailNews bug 438922 (only),
as it seemed Core+Firefox tests were "fine" with how it (not-)worked.

> so I wasn't aware that it had changed.

I would have thought you would have noticed when you did the patch / filed the bug.

(Anyway.)

Comment 11

[Marco Bonardo [:mak]]

after bug 478718 we don't have anymore tail files. be happy.

Comment 12

[Serge Gautherie (:sgautherie)]

V.Invalid, per comment 11.