Closed
Bug 765958
Opened 12 years ago
Closed 12 years ago
xpcshell tests for IMAP should clean up with asyncTestUtils.js and IMAPpump.js
Categories
(MailNews Core :: Testing Infrastructure, defect)
MailNews Core
Testing Infrastructure
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 17.0
People
(Reporter: hiro, Assigned: hiro)
References
Details
Attachments
(1 file, 2 obsolete files)
178.76 KB,
patch
|
hiro
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
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 | ||
Comment 2•12 years ago
|
||
xpcshell tests with the patch in imap gets 24s faster.
Comment 3•12 years ago
|
||
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•12 years ago
|
||
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 5•12 years ago
|
||
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•12 years ago
|
||
mconley, thank you for your review!
Attachment #640802 -
Attachment is obsolete: true
Attachment #643693 -
Flags: review+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 7•12 years ago
|
||
https://hg.mozilla.org/comm-central/rev/4936d2537b23
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 17.0
Assignee | ||
Comment 8•12 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.
You need to log in
before you can comment on or make changes to this bug.
Description
•