Closed
Bug 1018543
Opened 10 years ago
Closed 10 years ago
Modify test_pop3FilterActions.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, 3 obsolete files)
7.65 KB,
patch
|
sshagarwal
:
review+
|
Details | Diff | Splinter Review |
We intend to use promises for async functions in test_pop3FilterActions.js This is an attempt for the same.
Attachment #8432046 -
Flags: review?(kent)
Comment 1•10 years ago
|
||
Comment on attachment 8432046 [details] [diff] [review] pop3FilterActions.patch Review of attachment 8432046 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/local/test/unit/test_pop3FilterActions.js @@ +26,3 @@ > const gTestArray = > [ > + function *createFilters() { This is not a generator, so no "*" @@ +65,2 @@ > } catch (ex) { > do_check_true(ex.result == Cr.NS_ERROR_NOT_INITIALIZED); I checked the code, and contrary to normal practice, this error means that an async folder parse has begun. So we need a yield promiseUrlListener.promise at this point (and this function becomes a generator and needs function*) @@ +93,5 @@ > }, > + function endTest() { > + let thread = gThreadManager.currentThread; > + while (thread.hasPendingEvents()) > + thread.processNextEvent(true); I doubt that we need this whole function. Please try without it. Actually what I tested and seems to work is to get rid of all of these thread spinning functions, and just add the gPOP3Pump = null; here (eliminating your exitTest().) @@ +121,5 @@ > > MailServices.mailSession.AddFolderListener(FolderListener, > Ci.nsIFolderListener.event | > Ci.nsIFolderListener.added | > Ci.nsIFolderListener.removed); All of this folderListener stuff makes no sense in this test. Just remove this, as well as the whole FolderListener method. It looks like it was copied from another test but actually play no role here. @@ +127,5 @@ > } > > +add_task(function *doTest() { > + for (let foo of gTestArray) > + add_task(foo); Don't do add_task within add_task. Just do this operation right before run_next_test() above. @@ +132,2 @@ > > + add_task(exitTest); and you will be eliminating this as well. @@ +152,5 @@ > "\n"); > } > }; > > +function exitTest() Just add this to the list of tasks in gTestArray, there is no need to handle it as a special case. Or add the gPPO3Pump = null; earlier as I suggest.
Attachment #8432046 -
Flags: review?(kent) → review-
Assignee | ||
Comment 2•10 years ago
|
||
Tried to incorporate the suggested changes.
Attachment #8432046 -
Attachment is obsolete: true
Attachment #8433090 -
Flags: review?(kent)
Comment 3•10 years ago
|
||
Comment on attachment 8433090 [details] [diff] [review] Patch v1.6 Review of attachment 8433090 [details] [diff] [review]: ----------------------------------------------------------------- This is OK with the one important change. This patch cannot be landed until the dependent bug 1017879 also lands. ::: mailnews/local/test/unit/test_pop3FilterActions.js @@ +62,4 @@ > try { > + localAccountUtils.inboxFolder > + .getDatabaseWithReparse(promiseUrlListener, null); > + yield promiseUrlListener.promise; This is not where I meant for you to put this. Here it never gets called, because the previous statement throws. It should be in the catch before the return.
Attachment #8433090 -
Flags: review?(kent) → review+
Assignee | ||
Comment 4•10 years ago
|
||
Same approach as the other Promise based tests. Thanks.
Attachment #8433090 -
Attachment is obsolete: true
Attachment #8435522 -
Flags: review?(kent)
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 5•10 years ago
|
||
To be applied over the patch from bug 1017879, bug 1018542 and bug 1018300.
Comment 6•10 years ago
|
||
Comment on attachment 8435522 [details] [diff] [review] Patch v2 Looks good with one nit: This line in run_test: gMoveFolder = localAccountUtils.rootFolder.createLocalSubfolder("MoveFolder"); does nothing, and generates a warning. It must have been copied originally from another test. Please delete it. r=me with nit fixed.
Attachment #8435522 -
Flags: review?(kent) → review+
Assignee | ||
Comment 7•10 years ago
|
||
Thanks.
Attachment #8435522 -
Attachment is obsolete: true
Attachment #8438623 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 8•10 years ago
|
||
https://hg.mozilla.org/comm-central/rev/791d5276e5cb
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
•