503662 - Stop using tail_ files in xpcshell: use do_register_cleanup() instead, in MailNews

Status: RESOLVED FIXED

(MailNews Core :: Testing Infrastructure, defect)

Comment 1

[Serge Gautherie (:sgautherie)]

[
/mailnews/addrbook/test/unit/tail_addrbook.js
/mailnews/base/test/unit/tail_base.js
/mailnews/compose/test/unit/tail_compose.js
/mailnews/db/gloda/test/unit/tail_gloda.js
/mailnews/db/msgdb/test/unit/tail_msgdb.js
/mailnews/extensions/bayesian-spam-filter/test/unit/tail_bayesian.js
/mailnews/imap/test/unit/tail_imap.js
/mailnews/import/test/unit/tail_import.js
/mailnews/local/test/unit/tail_local.js
/mailnews/news/test/unit/tail_news.js
/mailnews/mime/test/unit/tail_mime.js
]

Comment 2

[Serge Gautherie (:sgautherie)]

They all just call |load("../../mailnews/resources/mailShutdown.js");|.
So it should be trivial to move this call to the cooresponding head files.

Comment 3

[Serge Gautherie (:sgautherie)]

Just fix "same case"
[
/mail/base/test/unit/tail_base.js
]
too.

Comment 4

[Mark Banner (:standard8)]

This isn't a bug for 3.1, this isn't needed on what is very soon going to be a stable branch as it doesn't actually buy us anything there. Its would be a good improvement going forward, but totally unnecessary to rush in before 3.1 or try and port across afterwards.

Comment 5

[:aceman]

Attachment: patch

Is this what is wanted here?

Comment 6

[Serge Gautherie (:sgautherie)]

Comment on attachment 766842 [details] [diff] [review]
patch

Review of attachment 766842 [details] [diff] [review]:
-----------------------------------------------------------------

Iirc, this patch seems to look like what I expected : f+ for that (only).

::: mail/base/test/unit/head_mailbase.js
@@ +2,5 @@
>  
>  // Import the main scripts that mail tests need to set up and tear down
>  load("../../../../mailnews/resources/mailDirService.js");
>  load("../../../../mailnews/resources/mailTestUtils.js");
> +do_register_cleanup(function() {

(My) Nits, fwiw:
*Ensure there is a blank line before do_register_cleanup() everywhere.
*Name the inner function.

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 766842 [details] [diff] [review]
patch

Review of attachment 766842 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/imap/test/unit/head_server.js
@@ +79,5 @@
>    do_check_eq(realTransaction.join(", "), expected.join(", "));
>  }
> +
> +do_register_cleanup(function() {
> +  load(gDEPTH + "mailnews/resources/mailShutdown.js");

It seems a bit strange to suddenly switch to using gDEPTH but not use it anywhere else.

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 766842 [details] [diff] [review]
patch

Looks fine, but I'd like to see what you think we should do for consistency before giving this the r+

Comment 9

[Mark Banner (:standard8)]

Attachment: Updated patch to remove tail files from comm-central

Lets get this landed (as I'm also getting bug 527444 landed), then we can just drop handling of tail files from xpcshell completely.

Comment 10

[:aceman]

Jorg, please run this through the try server for me. xpcshell is enough.

Comment 11

[Jorg K (CEST = GMT+2)]

Next time, please let me know which platforms and opt/debug you want. Here's the cheapest version:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a97a6d35db9562042ebe788542cdfdc2072051ca

Comment 12

[:aceman]

Comment on attachment 8826239 [details] [diff] [review]
Updated patch to remove tail files from comm-central

Review of attachment 8826239 [details] [diff] [review]:
-----------------------------------------------------------------

Seems to work, thanks.

Comment 13

[Mark Banner (:standard8)]

https://hg.mozilla.org/comm-central/rev/0b4789cf7743bd1e3319eca637b649f9f9491fc9
Bug 503662 - Stop using tail_ files in xpcshell: use do_register_cleanup() instead. r=aceman