Closed Bug 1031703 Opened 10 years ago Closed 10 years ago

Fix test_msgIDParsing.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, 2 obsolete files)

Fix test_msgIDParsing.js to work with both mbox and maildir mailbox formats.
Attached patch Patch v1 (obsolete) — Splinter Review
This test was hard-coded to use 0 as the msgKey, using
msgDatabase.FirstNew makes the test to pass with both
mbox and maildir.

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

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

Overall the approach is correct, but all of the extra code that you have to add to support both mbox and maildir points to the need for a refactoring of the standard setup code. This very simple test would be a good place to do that. You should have to add very little code to this test to support the swap from only mbox to both mbox and maildir if the common code was refactored correctly.

Really the meat of this patch is:
-  var msgHdr = localAccountUtils.inboxFolder.GetMessageHeader(0);
+  let msgHdr = localAccountUtils.inboxFolder.firstNewMessage;

Everything else should be minimal if we had proper common support methods. So I will feedback+ on this, asking for a rewrite.

::: mailnews/local/test/unit/test_msgIDParsing.js
@@ +11,5 @@
> +// Currently we have two mailbox storage formats.
> +var gPluggableStores = [
> +  "@mozilla.org/msgstore/berkeleystore;1",
> +  "@mozilla.org/msgstore/maildirstore;1"
> +];

This is being added to almost every test now, so it should be centralized. Could you add this to localAccountUtils.pluggableStores and use that value instead?

@@ +36,5 @@
> +  let identity = MailServices.accounts.createIdentity();
> +  identity.email = "bob@t2.example.net";
> +  localAccount.addIdentity(identity);
> +  localAccount.defaultIdentity = identity;
> +}

The fact that you had to rewrite this method for this test makes me think that the original design was probably incorrect. Could you add some version of this to localAccountUtils, that would be able to replace the one in POP3Pump? You should not have to add a custom version of this method to every test that does not use POP3Pump.

@@ +43,5 @@
> +{
> +  let localFolder = localAccountUtils.inboxFolder.QueryInterface(Ci.nsIMsgLocalMailFolder);
> +  localAccountUtils.inboxFolder.addMessage("From \r\n"+ headers + "\r\nhello\r\n");
> +  let db = localAccountUtils.inboxFolder.msgDatabase;
> +  let msgHdr = localAccountUtils.inboxFolder.GetMessageHeader(db.FirstNew);

Can you just use this:

let msgHdr = localAccountUtils.inboxFolder.firstNewMessage;
Attachment #8447665 - Flags: review?(kent) → feedback+
Attached patch Patch v1.6 (obsolete) — Splinter Review
I have tried to make the suggested modifications to allow for least change required for supporting both mbox and maildir in a test.

Thanks.
Attachment #8447665 - Attachment is obsolete: true
Attachment #8449960 - Flags: review?(kent)
Comment on attachment 8449960 [details] [diff] [review]
Patch v1.6

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

With our joint effort, we've got the code to add maildir to this test really small :)  r=me with the suggested changes.

::: mailnews/local/test/unit/test_msgIDParsing.js
@@ +23,5 @@
> +  let identity = MailServices.accounts.createIdentity();
> +  identity.email = "bob@t2.example.net";
> +  localAccount.addIdentity(identity);
> +  localAccount.defaultIdentity = identity;
> +}

This entire method exists to set the identity, yet other local tests don't seem to set identity. If it is needed, then it should be in the common routines.

But I don't believe it is needed. I did a try run deleting this method, and replacing the call below to setup_store with localAccountUtils.loadLocalMailAccount, and it worked just fine. So let's do that. See https://hg.mozilla.org/try-comm-central/rev/8c3261de965d

@@ +36,5 @@
>  
>  function run_test()
>  {
> +  for (let store of localAccountUtils.pluggableStores) {
> +    setup_store(store);

This will be replaced with localAccountUtils.loadLocalMailAccount
Attachment #8449960 - Flags: review?(kent) → review+
Attached patch Patch v2Splinter Review
Ya :)
Thanks.
Assignee: nobody → syshagarwal
Attachment #8449960 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8451090 - Flags: review+
Keywords: checkin-needed
Checked in https://hg.mozilla.org/comm-central/rev/bdf3156f6c1c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
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.