Closed Bug 1017879 Opened 10 years ago Closed 10 years ago

Allow promise processing of POP3pump callback

Categories

(MailNews Core :: Testing Infrastructure, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 33.0

People

(Reporter: rkent, Assigned: rkent)

References

Details

Attachments

(1 file, 4 obsolete files)

Adapt test_pop3Pump.js and POP3pump to use a promise callback as an alternative to the original onDone callback.
Attached patch 1017879.patch (obsolete) — Splinter Review
I had to resist the urge to do a lot of cleanup of POP3pump, since this was some of the original xpcshell code that I wrote. This is just a minimal way to use promises as well as onDone callback.
Assignee: nobody → kent
Status: NEW → ASSIGNED
Attachment #8431178 - Flags: review?(Pidgeot18)
Attached patch 1017879.patch (obsolete) — Splinter Review
Sorry, just a couple of minor tweaks.
Attachment #8431178 - Attachment is obsolete: true
Attachment #8431178 - Flags: review?(Pidgeot18)
Attachment #8431182 - Flags: review?(Pidgeot18)
Comment on attachment 8431182 [details] [diff] [review]
1017879.patch

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

::: mailnews/test/resources/POP3pump.js
@@ +9,3 @@
>   *  gPOP3Pump.files:  (in) an array of message files to load
>   *  gPOP3Pump.onDone: function to execute after completion
> + *  gPOP3Pump.deferred: set to a Promise.defer()to enable promise processing

I wonder how much it makes sense to need to set gPOP3Pump.deferred. {See below}

@@ +68,4 @@
>  
> +    var thread = Services.tm.currentThread;
> +    while (thread.hasPendingEvents())
> +      thread.processNextEvent(true);

I'm starting to wonder how necessary the pump-the-event-loop is post-server-stop. I think this is all a massive cargo-cult from my original NNTP fake server patch, and the original motivation there was probably a mixture of needing to assert that the connection had the CLOSE event get matched for transaction scanning purposes, and possibly to avoid shutdown leaks or crashes due to late spins of the loop. The former case ought to be handled by nsMailServer.stop() already spinning the event loop until the socket is closed, and the latter could easily be handled by asynchronous shutdown registration.

@@ +124,5 @@
>      {
>        // exit this module
>        do_test_finished();
> +      if (this.onDone)
> +        do_timeout(0, this.onDone);

So the goal of the do_timeout(0, this.onDone) is to execute this.onDone in a separate iteration. You can get a similar effect with saying this.deferred.then(this.onDone, this.onDone), if it existed. With this in mind, it makes more sense to me to have the Pop3Pump set the deferred internally, and have a setter for onDone that adds it to the .then method, and unconditionally return the promise in the run method.
Attached patch Cleanup with jcranmer comments (obsolete) — Splinter Review
Attachment #8431182 - Attachment is obsolete: true
Attachment #8431182 - Flags: review?(Pidgeot18)
Attachment #8431914 - Flags: review?(Pidgeot18)
Comment on attachment 8431914 [details] [diff] [review]
Cleanup with jcranmer comments

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

::: mailnews/local/test/unit/test_pop3Pump.js
@@ +21,2 @@
>  
> +add_task(function continueTest() {

You don't need this to be a separate function. Just have it run after the yield from runPump.

@@ +23,4 @@
>    // get message headers for the inbox folder
>    let enumerator = localAccountUtils.inboxFolder.msgDatabase.EnumerateMessages();
>    var msgCount = 0;
> +  while(enumerator.hasMoreElements()) {

Nit: space after while

::: mailnews/test/resources/POP3pump.js
@@ +9,2 @@
>   *  gPOP3Pump.files:  (in) an array of message files to load
>   *  gPOP3Pump.onDone: function to execute after completion

Add a note that onDone isn't necessary and is deprecated.

@@ +43,5 @@
>    this._finalCleanup = false;
> +  this._expectedResult = Components.results.NS_OK;
> +
> +  // This probably does not work with multiple tests, but nobody is using that.
> +  this._deferred = Promise.defer();

This needs the Cu.import for Promise.jsm in this file.

@@ +117,5 @@
>      {
>        // exit this module
>        do_test_finished();
> +      if (this.onDone)
> +        this._deferred.promise.then(this.onDone);

then(this.onDone, this.onDone)--you need onDone to be called even if the URL listener failed for backwards compatibility.
Attachment #8431914 - Flags: review?(Pidgeot18) → review+
Attached patch 1017879_v2.patch (obsolete) — Splinter Review
This is the patch I will land when the tree reopens.
Attachment #8431914 - Attachment is obsolete: true
Attachment #8432777 - Flags: review+
Attached patch 1017879_v3.patchSplinter Review
Irving, I tried to incorporate here your suggestions from bug 1017939. Can you please confirm that this approach is correct? The previous patch had r=jcranmer but that was using deferred()

I've seen several other examples in the Mozilla codebase where the resolve method was saved, and then passed to a different part of the code.
Attachment #8432777 - Attachment is obsolete: true
Attachment #8435186 - Flags: review?(irving)
Blocks: 1018543
Comment on attachment 8435186 [details] [diff] [review]
1017879_v3.patch

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

This is a very tentative r+; the particular change around using Promise.new(...) and saving the resolve & reject function references for later use is fine, but I'm nervous about the way the shutdown is managed.

::: mailnews/test/resources/POP3pump.js
@@ +116,5 @@
>      {
>        // exit this module
>        do_test_finished();
> +      if (this.onDone)
> +        this._promise.then(this.onDone, this.onDone);

I disagree with Joshua on this; I'm not comfortable using the promise.then() call in this case to yield to the event loop.

I spent a while trying to sort out the logical relationship between the pop3pump promise and onDone before I re-read the bug comments and saw the suggestion of using .then() as a replacement for do_execute_soon() / setTimeout(0).

I'm also pretty sure we shouldn't be calling do_test_finished() directly in the add_task() test driver; the driver handles finishing the test suite when all the tasks have completed.

And if we haven't yet resolved this._promise by the time we're shutting down the pump, we should probably just reject rather than resolve, so the test case doesn't get the idea that the pump finished doing what was it was asked to. Note that if you just always call reject() here, it will be ignored if the promise had previously been resolved so you don't need fancy logic.

@@ +212,5 @@
> +
> +  // This probably does not work with multiple tests, but nobody is using that.
> +  this._promise = new Promise( (resolve, reject) => {
> +    this._resolve = resolve;
> +    this._reject = reject;

This is fine; the 'resolve' and 'reject' function handles are designed so that they can be saved away and used later.
Attachment #8435186 - Flags: review?(irving) → review+
"This is a very tentative r+"

We are still learning the proper way to setup the promise-based test environment for xpcshell. What I think makes sense is to get some of these landed, which makes it easier to land other tests, and then cleanup issues as we learn them.

So I'll add bug 1023042 that asks us to consider this issue as part of cleanup of management of promises in xpcshell tests as we learn our way.
Checked in https://hg.mozilla.org/comm-central/rev/88b552f75896
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
OS: Windows 7 → All
Hardware: x86_64 → All
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 33.0
You need to log in before you can comment on or make changes to this bug.