Closed
Bug 1018300
Opened 10 years ago
Closed 10 years ago
Modify test_pop3MoveFilter.js to use Promises.
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, 5 obsolete files)
8.93 KB,
patch
|
sshagarwal
:
review+
|
Details | Diff | Splinter Review |
As test_pop3MoveFilter.js has some async functions, we intend to modify it to use Promises. As this test uses POP3pump, this patch is based on the patch from bug 1017879.
Attachment #8431663 -
Flags: review?(kent)
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8431663 -
Attachment is obsolete: true
Attachment #8431663 -
Flags: review?(kent)
Attachment #8431666 -
Flags: review?(kent)
Comment 2•10 years ago
|
||
Comment on attachment 8431666 [details] [diff] [review] Patch v1.2 Review of attachment 8431666 [details] [diff] [review]: ----------------------------------------------------------------- Please incorporate some of these comments and redo this. ::: mailnews/local/test/unit/test_pop3MoveFilter.js @@ +46,2 @@ > gPOP3Pump.files = gFiles; > + gPOP3Pump.deferred = Promise.defer(); In the modified version of POP3pump after I incorporate jcranmer's review comments, this line will not be needed, and should be removed. @@ +78,5 @@ > let hdr = enumerator.getNext().QueryInterface(Ci.nsIMsgDBHdr); > keys.push(hdr.messageKey); > hdrs.push(hdr); > } > gMoveFolder.fetchMsgPreviewText(keys, keys.length, false, null, asyncResults); Yes the original was like this, but this looks odd to me. asyncResults is an attribute, so I would expect: do_check_false(gMoveFolder.fetchMsgPreviewText(keys, keys.length, false, null)); and you can delete the "let asyncResults = ..." earlier. Try this and see if it works. @@ +85,5 @@ > + }, > + function endTest() { > + let thread = gThreadManager.currentThread; > + while (thread.hasPendingEvents()) > + thread.processNextEvent(true); Why did you separate the "gPOP3Pump = null;" from this method? Just add it here. If something is not working when you do that, then it is an indicator of some other problem. @@ +120,3 @@ > } > > +add_task(function *doTest() { Please so not add complex loops where you add_task inside of add_task. Just do all of the setup that you need, that is calling add_task for each test, as part of the run_test method. That includes adding the final add_task(exitTest). @@ +132,5 @@ > OnItemAdded: function OnItemAdded(aParentItem, aItem) { > this._showEvent(aParentItem, "OnItemAdded"); > }, > OnItemRemoved: function OnItemRemoved(aParentItem, aItem) { > this._showEvent(aParentItem, "OnItemRemoved"); So by removing the "doTest" here you are essentially converting these tests from sync to async. You really don;t want to do that, as what you will find is that it may work on some platforms and not the others. What I want you do to is to define a generic PromiseFolderListener in PromiseTestUtils, that has an optional parameter for the method that is used to resolve the promise. In you case, it will be "OnItemRemoved". The methods should pick something intelligent to use as the returned item in deferred.resolve(), typically the item that is mentioned in the callback. Then go back and make this whole test async again using yield PromiseTestUtils.PromiseFolderListener("OnItemRemoved"). @@ +144,5 @@ > "\n"); > } > }; > > +function exitTest() Just include this as part of the gTestArray, and you don't need any special processing that do add_task(exitTest)
Attachment #8431666 -
Flags: review?(kent) → review-
Comment 3•10 years ago
|
||
I'm sorry that there is such a moving target in the implementation of Promise in tests, but you need to take the comments from Bug 1018543 and incorporate them in the similar test here. Also this needs to be done, and the changes here separated from the changes in Bug 827048 before I can make any progress with Bug 827048
Assignee | ||
Comment 4•10 years ago
|
||
Setting the dependency on bug 1018542 as both these bugs require editing the same file PromiseTestUtils.jsm.
Depends on: 1018542
Assignee | ||
Comment 5•10 years ago
|
||
Done in the same way as bug 1018542. Please let me know if this is the correct way to convert async tests to Promises.
Attachment #8431666 -
Attachment is obsolete: true
Attachment #8435510 -
Flags: feedback?(kent)
Assignee | ||
Comment 6•10 years ago
|
||
I forgot to remove this._deferred.
Attachment #8435510 -
Attachment is obsolete: true
Attachment #8435510 -
Flags: feedback?(kent)
Attachment #8435519 -
Flags: feedback?(kent)
Assignee | ||
Comment 7•10 years ago
|
||
To be applied over the patch from bug 1017879 and bug 1018542.
Comment 8•10 years ago
|
||
Comment on attachment 8435519 [details] [diff] [review] Patch v2.1 Review of attachment 8435519 [details] [diff] [review]: ----------------------------------------------------------------- Generally I think this is correct with the noted issues that need addressing. ::: mailnews/local/test/unit/test_pop3MoveFilter.js @@ +9,5 @@ > +Components.utils.import("resource:///modules/mailServices.js"); > +Components.utils.import("resource://gre/modules/Promise.jsm"); > +Components.utils.import("resource://gre/modules/Services.jsm"); > +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm"); > +Components.utils.import("resource://testing-common/mailnews/PromiseTestUtils.jsm"); Is there some reason why you are changing the order of the imports here? If not, then please add you new import in a way that does not require deleting and readding the existing import. @@ +114,3 @@ > Ci.nsIFolderListener.event | > Ci.nsIFolderListener.added | > Ci.nsIFolderListener.removed); This listener as far as I can tell does not play any role in the test, so I am not sure why it is here. Can you please just remove it and see what happens? What you have done certainly does not make sense, as you add a promise listener but never actually use the promise. ::: mailnews/test/resources/PromiseTestUtils.jsm @@ +54,4 @@ > }; > > /** > + * Folder listener that can wrap another listener and trigger a callback. If my earlier comment is true, then we will not actually be using this in this test. There are some design issues we need to discuss to optimize this design, but I think it would be better to discuss those in a test that actually needs this. You are welcome to try and find one, but if we turn out not using this here it would be better to delete it rather than to land it untested.
Attachment #8435519 -
Flags: feedback?(kent) → feedback+
Assignee | ||
Comment 9•10 years ago
|
||
(In reply to Kent James (:rkent) from comment #8) > Comment on attachment 8435519 [details] [diff] [review] > Patch v2.1 > ::: mailnews/local/test/unit/test_pop3MoveFilter.js > @@ +9,5 @@ > > +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm"); > > +Components.utils.import("resource://testing-common/mailnews/PromiseTestUtils.jsm"); > > Is there some reason why you are changing the order of the imports here? If > not, then please add you new import in a way that does not require deleting > and readding the existing import. Ya, actually I have noticed that we put imports in lexical order, so I was just trying to do that. If you say, I'll remove it. > @@ +114,3 @@ > > Ci.nsIFolderListener.event | > > Ci.nsIFolderListener.added | > > This listener as far as I can tell does not play any role in the test, so I > am not sure why it is here. Can you please just remove it and see what > happens? The test passes after removing it. So, I have removed it and the added listener from PromiseTestUtils.jsm > What you have done certainly does not make sense, as you add a promise > listener but never actually use the promise. Ya, using |yield promiseFolderListener.promise| in run_test() was making an early return from the test, so I removed it. > ::: mailnews/test/resources/PromiseTestUtils.jsm > @@ +54,4 @@ > > + * Folder listener that can wrap another listener and trigger a callback. > > If my earlier comment is true, then we will not actually be using this in > this test. There are some design issues we need to discuss to optimize this > design, but I think it would be better to discuss those in a test that > actually needs this. Okay, removed it. Thanks.
Attachment #8435519 -
Attachment is obsolete: true
Attachment #8436255 -
Flags: review?(kent)
Comment 10•10 years ago
|
||
Comment on attachment 8436255 [details] [diff] [review] Patch v2.4 Review of attachment 8436255 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine to me except for the nits. Unfortunately we need to wait for the Pop3pump changes to land before we can finish this. So I'll feedback+ for now, and we'll lokk at it one more time after pop3Pump conversion to promises lands. ::: mailnews/local/test/unit/test_pop3MoveFilter.js @@ +10,3 @@ > Components.utils.import("resource:///modules/mailServices.js"); > Components.utils.import("resource://gre/modules/Services.jsm"); > +Components.utils.import("resource://gre/modules/Promise.jsm"); I don't think that you need to add Promise.jsm @@ +70,2 @@ > }, > + function *verifyMessages() { This is not a generator, so no * in *verifyMessages
Attachment #8436255 -
Flags: review?(kent) → feedback+
Comment 11•10 years ago
|
||
Comment on attachment 8436255 [details] [diff] [review] Patch v2.4 So r=me after the two nits are fixed.
Attachment #8436255 -
Flags: review+
Assignee | ||
Comment 12•10 years ago
|
||
Thanks.
Attachment #8436255 -
Attachment is obsolete: true
Attachment #8438141 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Keywords: checkin-needed
Comment 13•10 years ago
|
||
https://hg.mozilla.org/comm-central/rev/1a9fcf6700eb
Status: ASSIGNED → RESOLVED
Closed: 10 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.
Description
•