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)
MailNews Core
Testing Infrastructure
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 33.0
People
(Reporter: sshagarwal, Assigned: sshagarwal)
References
Details
Attachments
(1 file, 2 obsolete files)
4.39 KB,
patch
|
sshagarwal
:
review+
|
Details | Diff | Splinter Review |
Fix test_msgIDParsing.js to work with both mbox and maildir mailbox formats.
Assignee | ||
Comment 1•10 years ago
|
||
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 2•10 years ago
|
||
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+
Assignee | ||
Comment 3•10 years ago
|
||
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 4•10 years ago
|
||
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+
Assignee | ||
Comment 5•10 years ago
|
||
Ya :) Thanks.
Assignee: nobody → syshagarwal
Attachment #8449960 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8451090 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 6•10 years ago
|
||
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.
Description
•