Closed Bug 1018542 Opened 6 years ago Closed 6 years ago

Modify test_movemailDownload.js to use Promises.

Categories

(MailNews Core :: Testing Infrastructure, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 33.0

People

(Reporter: sshagarwal, Assigned: sshagarwal)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch movemailDownload.patch (obsolete) — Splinter Review
Modify test_movemailDownload.js to use add_task()
and promises instead of do_test_pending() and do_test_finished().
Attachment #8432045 - Flags: review?(kent)
Comment on attachment 8432045 [details] [diff] [review]
movemailDownload.patch

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

Here's what I would like for you to do with this.

First, we need a standard PromiseStreamListener in PromiseTestUtils that follows the pattern of the others.

The PromiseStreamListener should include the same basic logic as the test's listener, including have a local _stream and _data variable. When resolved, it should return the data using deferred.resolve(this._data).

The "do_check_true(this._data.startsWith("From "));" should be in the test, not in the listener, after the "let data = yield promiseStreamListener.promise"

Avoid doing the dynamic add_task. Although it may work, it is not typical and not necessary. You can use normal control loops in your tasks, just do that instead.

Can you try that?
Attachment #8432045 - Flags: review?(kent) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
This patch tries to address comment 1.

Thanks.
Attachment #8432045 - Attachment is obsolete: true
Attachment #8432634 - Flags: review?(kent)
Comment on attachment 8432634 [details] [diff] [review]
Patch v2

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

As we discussed on IRC, the management of yield with the streams needs to be fixed.
Attachment #8432634 - Flags: review?(kent) → review-
Attached patch Patch v3 (obsolete) — Splinter Review
Made the suggested and discussed fixes.

Thanks.
Attachment #8432634 - Attachment is obsolete: true
Attachment #8432723 - Flags: review?(kent)
Comment on attachment 8432723 [details] [diff] [review]
Patch v3

Generally this looks OK. Unfortunately when I went to test it, it is disabled for Windows. With the tree broken I don't have any easy way to get a Linux or OSX build working to test it on. So I think I will have to wait until the tree gets fixed to finish looking at this.
Comment on attachment 8432723 [details] [diff] [review]
Patch v3

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

I tested this on the try server and it works fine. Unfortunately we need to incorporate the changes that Irving suggested.

You might look at the patch I did on Bug 1017879. Essentially instead of storing a deferred, you store the resolve function (and the promise and reject if needed). Perhaps you could implement that approach into the stream listener here.
Comment on attachment 8432723 [details] [diff] [review]
Patch v3

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

I tested this on the try server and it works fine. Unfortunately we need to incorporate the changes that Irving suggested.

You might look at the patch I did on Bug 1017879. Essentially instead of storing a deferred, you store the resolve function (and the promise and reject if needed). Perhaps you could implement that approach into the stream listener here.
Attachment #8432723 - Flags: review?(kent) → review-
Attached patch Patch v4Splinter Review
Okay so I have tried the approach you mentioned in comment 7.

Thanks.
Attachment #8432723 - Attachment is obsolete: true
Attachment #8435480 - Flags: review?(kent)
Blocks: 1018300
Blocks: 1018543
Comment on attachment 8435480 [details] [diff] [review]
Patch v4

Looks good to me, so let's try to land this without waiting for additional feedback. We can always revise the approach later if needed, but we need to get some of these things landed so that we can make progress.

One nit: I don't believe that you need to include Task.jsm, so please try witho that and remove if possible.
Attachment #8435480 - Flags: review?(kent) → review+
Thanks.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/dfe18de037d7
Status: ASSIGNED → RESOLVED
Closed: 6 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.