The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Thunderbird 17.0

Status

MailNews Core
Testing Infrastructure
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

Trunk
Thunderbird 17.0
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.
Assignee: nobody → hiikezoe
Status: NEW → ASSIGNED
Attachment #634232 - Flags: review?(mconley)
(Assignee)

Comment 2

5 years ago
xpcshell tests with the patch in imap gets 24s faster.
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.
Attachment #634232 - Flags: review?(mconley) → review-
(Assignee)

Comment 4

5 years ago
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.
Attachment #634232 - Attachment is obsolete: true
Attachment #640802 - Flags: review?(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
Attachment #640802 - Flags: review?(mconley) → review+
(Assignee)

Comment 6

5 years ago
Created attachment 643693 [details] [diff] [review]
Removed dump.

mconley, thank you for your review!
Attachment #640802 - Attachment is obsolete: true
Attachment #643693 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/4936d2537b23
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 17.0
(Assignee)

Comment 8

5 years ago
(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.

Updated

4 years ago
Duplicate of this bug: 544396
You need to log in before you can comment on or make changes to this bug.