Closed Bug 1011597 Opened 7 years ago Closed 7 years ago

Use promise-based listener in test_ImapPump as a demo of Promise in testing

Categories

(MailNews Core :: Testing Infrastructure, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 32.0

People

(Reporter: rkent, Assigned: rkent)

Details

Attachments

(1 file, 1 obsolete file)

I propose that we start to move mailnews XPCSHELL tests away from asyncTestUtils and toward promise and Task.jsm - based async management. As an initial step in this, convert test_impaPump.js to Promises.
Attached patch 1011579_1.patch (obsolete) — Splinter Review
Work in progress
Attached patch rev 2Splinter Review
Just some tweaks, including using function*
Assignee: nobody → kent
Attachment #8423998 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8424348 - Flags: review?(Pidgeot18)
The main point of this is to make sure that we are happy with the overall pattern here, including the naming conventions. I used more standard module naming for example (PromiseTestUtils.jsm)instead of the local pattern (which would be promiseTestUtils.js) OK?

Also as we discussed on IRC, one big difference here is that you need to do a new PromiseTestUtils.PromiseUrlListener(); each time the listener is used, while the old listener could be reused. I still think that is the best way to do it, as promises once resolved cannot be reused.
Comment on attachment 8424348 [details] [diff] [review]
rev 2

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

I think the promise/nsIUrlListener interconversion approach may be more desirable in front-end code in the future, but hg moves are cheap and we should probably solidify our future Promises story before making it more widely available outside of tests.

::: mailnews/test/resources/PromiseTestUtils.jsm
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> + * This file provides utilities useful in using Promises and Task.jsm
> + * with mailnews tests

Nit: missing .

@@ +29,5 @@
> +
> +PromiseTestUtils.PromiseUrlListener = function(aWrapped) {
> +  this.wrapped = aWrapped ? aWrapped.QueryInterface(Ci.nsIUrlListener) : null;
> +  this._deferred = Promise.defer();
> +}

Missing a semicolon here.

@@ +44,5 @@
> +      this.wrapped.OnStopRunningUrl(aUrl, aExitCode);
> +    if (aExitCode == Cr.NS_OK)
> +      this._deferred.resolve();
> +    else
> +      this._deferred.reject();

this._deferred.reject(aExitCode) perhaps?
Attachment #8424348 - Flags: review?(Pidgeot18) → review+
Checked in https://hg.mozilla.org/comm-central/rev/8200f186be46
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
OS: Windows 7 → All
Hardware: x86_64 → All
Target Milestone: --- → Thunderbird 32.0
You need to log in before you can comment on or make changes to this bug.