Last Comment Bug 765958 - xpcshell tests for IMAP should clean up with asyncTestUtils.js and IMAPpump.js
: xpcshell tests for IMAP should clean up with asyncTestUtils.js and IMAPpump.js
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Testing Infrastructure (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 17.0
Assigned To: Hiroyuki Ikezoe (:hiro)
:
:
Mentors:
: 544396 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-18 16:31 PDT by Hiroyuki Ikezoe (:hiro)
Modified: 2013-05-06 10:01 PDT (History)
4 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed fix (178.51 KB, patch)
2012-06-18 16:43 PDT, Hiroyuki Ikezoe (:hiro)
mconley: review-
Details | Diff | Splinter Review
Revised patch (178.83 KB, patch)
2012-07-10 14:52 PDT, Hiroyuki Ikezoe (:hiro)
mconley: review+
Details | Diff | Splinter Review
Removed dump. (178.76 KB, patch)
2012-07-18 17:42 PDT, Hiroyuki Ikezoe (:hiro)
hiikezoe: review+
Details | Diff | Splinter Review

Description Hiroyuki Ikezoe (:hiro) 2012-06-18 16:31:25 PDT
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) 2012-06-18 16:43:28 PDT
Created attachment 634232 [details] [diff] [review]
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) 2012-06-18 16:44:28 PDT
xpcshell tests with the patch in imap gets 24s faster.
Comment 3 Mike Conley (:mconley) 2012-07-03 11:11:47 PDT
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) 2012-07-10 14:52:27 PDT
Created attachment 640802 [details] [diff] [review]
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) 2012-07-18 12:17:08 PDT
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) 2012-07-18 17:42:11 PDT
Created attachment 643693 [details] [diff] [review]
Removed dump.

mconley, thank you for your review!
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-07-18 18:25:12 PDT
https://hg.mozilla.org/comm-central/rev/4936d2537b23
Comment 8 Hiroyuki Ikezoe (:hiro) 2012-07-18 19:23:20 PDT
(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.
Comment 9 :aceman 2013-05-06 10:01:28 PDT
*** Bug 544396 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.