Closed Bug 1037947 Opened 7 years ago Closed 7 years ago

Fix test_movemailDownload.js to work with maildir

Categories

(MailNews Core :: Testing Infrastructure, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 33.0

People

(Reporter: sshagarwal, Assigned: sshagarwal)

References

Details

Attachments

(1 file, 1 obsolete file)

We intend to make test_movemailDownload.js to pass with both
mbox and maildir mailbox formats.
Attached patch Patch v1 (obsolete) — Splinter Review
nsIInputStream wasn't needed in nsMovemailService::GetNewMail()
so I removed it.
Also maybe because movemailspool is just an mbox format file.
So, I thought to firstly create another movemailspool directory in maildir
format. But then this works as is.
Please let me know the changes/modifications I am supposed to make in this.

Thanks.
Attachment #8454994 - Flags: review?(kent)
Blocks: 1011399
Comment on attachment 8454994 [details] [diff] [review]
Patch v1

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

This looks good except for the two issues (using the store list from localAccountsUtils, and clearing the hdr array). r=me with these two issues fixed.

::: mailnews/local/src/nsMovemailService.cpp
@@ +512,1 @@
>        rv = newMailParser->Init(serverFolder, inbox,

Just a note for future reference:

The inputStream was added in bug #748726 without any comment. I cannot think of any valid reason why this QI to an unused variable makes sense. I'm going to assume this was just some leftover stray code, and that the removal here makes sense.

::: mailnews/local/test/unit/test_movemailDownload.js
@@ +6,5 @@
>  Components.utils.import("resource://gre/modules/Services.jsm");
>  Components.utils.import("resource://gre/modules/Task.jsm");
>  Components.utils.import("resource://testing-common/mailnews/PromiseTestUtils.jsm");
>  
> +var gPluggableStores = [

Isn't this in localAccountUtils now? Just use the version from there.

@@ +55,5 @@
> +    var msgCount = 0;
> +    let hdr;
> +    while (enumerator.hasMoreElements()) {
> +      let hdr = enumerator.getNext().QueryInterface(Ci.nsIMsgDBHdr);
> +      gMsgHdrs.push(hdr);

You run this code twice, once for each store. But the gMsgHdrs array only gets cleared in the overall initialization. So the second time that you run streamMessages, you restream the headers from the mbox test.

So you need to clear gMsgHdrs at the start of continueTest().
Attachment #8454994 - Flags: review?(kent) → review+
Thanks.
Assignee: nobody → syshagarwal
Attachment #8454994 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8457769 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/74c23393fbbc
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 33.0
You need to log in before you can comment on or make changes to this bug.