765958 - xpcshell tests for IMAP should clean up with asyncTestUtils.js and IMAPpump.js

Status: RESOLVED FIXED

(MailNews Core :: Testing Infrastructure, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

There are a lot of do_timeout(XX. Those should be removed as possible. And many tests have the same code related to IMAP server, it should be replaced by IMAPpump functions.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Proposed fix

Most of the tests are simple replacement but test_copyThenMove.js and test_imapFlagChange.js are not.

These two tests need:

  Services.prefs.setBoolPref("mail.server.default.autosync_offline_stores", false);

because the tests crashed on macosx debug build on try server without the line.

In case of test_copyThenMove.js fetch request is fired after OnStopCopy of CopyMessages, so the request persists after teardown(), then crased. See bug 436366#c10 for the crash log.
In case of test_imapFlagChange.js previewBody request persists after teardown().

Try server result is:
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=569b32a65c22

I've also confirmed all tests work fine on local Windows XP and Linux.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

xpcshell tests with the patch in imap gets 24s faster.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 634232 [details] [diff] [review]
Proposed fix

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

hiro:

This looks really good - I like how you're DRYing up the code, and the faster tests are *always* welcome. :) So thank you!

I've found a few small things - nothing major. If you could fix those, and then push up a try build to run them across each platform, that'll be enough for me to give my r+.

Thanks,

-Mike

::: mailnews/imap/test/unit/test_bccProperty.js
@@ +16,5 @@
>  const gMsgFile = do_get_file("../../../data/" + gFileName);
>  
> +var tests = [
> +  setup,
> +  donwloadAllForOffline,

Typo "donwload" (and typo in the corresponding function, which is why it wasn't picked up)

::: mailnews/imap/test/unit/test_downloadOffline.js
@@ +56,5 @@
>  
> +function verifyDownloaded() {
> +  // verify that the message headers have the offline flag set.
> +  let msgEnumerator = gIMAPInbox.msgDatabase.EnumerateMessages();
> +  let offset = new Object;

"new Object" isn't something we see too often. Maybe just use:

let offset = {};
let size = {};

::: mailnews/imap/test/unit/test_imapContentLength.js
@@ +35,4 @@
>    let ioService = Cc["@mozilla.org/network/io-service;1"]
>                      .getService(Ci.nsIIOService);
>  
> +  let URI = ioService.newFileURI(gFile).QueryInterface(Ci.nsIFileURL);

So there are a ton of places where we're manually getting the IO service...we might just want to use Services.jsm, and use Services.io.

You don't have to do that here, but can you file a bug for it for future cleanup?

::: mailnews/imap/test/unit/test_imapFolderCopy.js
@@ +90,5 @@
>      CopyListener._expectedStatus = 0x80550021;
>      gCopyService.CopyFolders(array, gIMAPInbox, false, CopyListener, null);
> +    yield false;
> +
> +    yield false;

Can I assume that the double-yield is on purpose? If so, can you please put in a comment explaining it?

::: mailnews/imap/test/unit/test_imapStoreMsgOffline.js
@@ +215,5 @@
>    {
> +    // nsIMsgFolder.DownloadMessagesForOffline does not take a listener, so
> +    // we invoke nsIImapService.downloadMessagesForOffline directly with a 
> +    // listener.
> +    let imapService = Cc["@mozilla.org/messenger/imapservice;1"]

As before with Services.io, we could be using MailServices.imap for this instead. Please replace, or file a bug.

If you file a bug, a nit here - the .getService line should be indented one more space.

::: mailnews/imap/test/unit/test_imapUndo.js
@@ +139,5 @@
>    addMessagesToServer([{file: gMsgFile1, messageId: gMsgId1},
>                          {file: gMsgFile4, messageId: gMsgId4},
>                           {file: gMsgFile5, messageId: gMsgId5},
>                          {file: gMsgFile2, messageId: gMsgId2}],
> +                        gIMAPMailbox, gIMAPInbox);

Nit - since this is outside of the array braces, this should be lined up vertically with the opening array brace on line 139.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Revised patch

Try server result is:

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=4ea49ff98d42

Unfortunately there are lots of failure of netwerk/test/httpserver/test/test_load_module.js, but it is not related to this issue.

(In reply to Mike Conley (:mconley) from comment #3)
> ::: mailnews/imap/test/unit/test_bccProperty.js
> @@ +16,5 @@
> >  const gMsgFile = do_get_file("../../../data/" + gFileName);
> >  
> > +var tests = [
> > +  setup,
> > +  donwloadAllForOffline,
> 
> Typo "donwload" (and typo in the corresponding function, which is why it
> wasn't picked up)

Done.

> ::: mailnews/imap/test/unit/test_downloadOffline.js
> @@ +56,5 @@
> >  
> > +function verifyDownloaded() {
> > +  // verify that the message headers have the offline flag set.
> > +  let msgEnumerator = gIMAPInbox.msgDatabase.EnumerateMessages();
> > +  let offset = new Object;
> 
> "new Object" isn't something we see too often. Maybe just use:
> 
> let offset = {};
> let size = {};

Done.

> ::: mailnews/imap/test/unit/test_imapContentLength.js
> @@ +35,4 @@
> >    let ioService = Cc["@mozilla.org/network/io-service;1"]
> >                      .getService(Ci.nsIIOService);
> >  
> > +  let URI = ioService.newFileURI(gFile).QueryInterface(Ci.nsIFileURL);
> 
> So there are a ton of places where we're manually getting the IO
> service...we might just want to use Services.jsm, and use Services.io.
> 
> You don't have to do that here, but can you file a bug for it for future
> cleanup?

I will open a new bug for this.

> ::: mailnews/imap/test/unit/test_imapFolderCopy.js
> @@ +90,5 @@
> >      CopyListener._expectedStatus = 0x80550021;
> >      gCopyService.CopyFolders(array, gIMAPInbox, false, CopyListener, null);
> > +    yield false;
> > +
> > +    yield false;
> 
> Can I assume that the double-yield is on purpose? If so, can you please put
> in a comment explaining it?

Added a comment.

The first yiled for a OnStopCopy comes from nsMsgCopyService, the second one comes from nsImapFolderCopyState.

I am not sure these two notifications are intentional or not...


> ::: mailnews/imap/test/unit/test_imapStoreMsgOffline.js
> @@ +215,5 @@
> >    {
> > +    // nsIMsgFolder.DownloadMessagesForOffline does not take a listener, so
> > +    // we invoke nsIImapService.downloadMessagesForOffline directly with a 
> > +    // listener.
> > +    let imapService = Cc["@mozilla.org/messenger/imapservice;1"]
> 
> As before with Services.io, we could be using MailServices.imap for this
> instead. Please replace, or file a bug.
> 
> If you file a bug, a nit here - the .getService line should be indented one
> more space.

Done.

> ::: mailnews/imap/test/unit/test_imapUndo.js
> @@ +139,5 @@
> >    addMessagesToServer([{file: gMsgFile1, messageId: gMsgId1},
> >                          {file: gMsgFile4, messageId: gMsgId4},
> >                           {file: gMsgFile5, messageId: gMsgId5},
> >                          {file: gMsgFile2, messageId: gMsgId2}],
> > +                        gIMAPMailbox, gIMAPInbox);
> 
> Nit - since this is outside of the array braces, this should be lined up
> vertically with the opening array brace on line 139.

Done.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 640802 [details] [diff] [review]
Revised patch

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

Sorry it took so long to get to this (again!). Finally had a chance to look it over.

It looks fantastic, and the tests pass. Great work!

::: mailnews/imap/test/unit/test_offlineStoreLocking.js
@@ +104,5 @@
> +    yield false;
> +
> +    // Because we're streaming the message while compaction is going on,
> +    // we should not have stored it for offline use.
> +    dump("finished streaming " + gStreamedHdr.messageKey + "\n");

We can probably take this opportunity to strip this dump statement

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Removed dump.

mconley, thank you for your review!

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/4936d2537b23

Comment 8

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> > ::: mailnews/imap/test/unit/test_imapContentLength.js
> > @@ +35,4 @@
> > >    let ioService = Cc["@mozilla.org/network/io-service;1"]
> > >                      .getService(Ci.nsIIOService);
> > >  
> > > +  let URI = ioService.newFileURI(gFile).QueryInterface(Ci.nsIFileURL);
> > 
> > So there are a ton of places where we're manually getting the IO
> > service...we might just want to use Services.jsm, and use Services.io.
> > 
> > You don't have to do that here, but can you file a bug for it for future
> > cleanup?
> 
> I will open a new bug for this.

Filed Bug 775394.