1018542 - Modify test_movemailDownload.js to use Promises.

Status: RESOLVED FIXED

(MailNews Core :: Testing Infrastructure, defect)

Description

[Suyash Agarwal (:sshagarwal)]

Attachment: movemailDownload.patch

Modify test_movemailDownload.js to use add_task()
and promises instead of do_test_pending() and do_test_finished().

Comment 1

[Kent James (:rkent)]

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?

Comment 2

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2

This patch tries to address comment 1.

Thanks.

Comment 3

[Kent James (:rkent)]

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.

Comment 4

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3

Made the suggested and discussed fixes.

Thanks.

Comment 5

[Kent James (:rkent)]

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 6

[Kent James (:rkent)]

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 7

[Kent James (:rkent)]

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 8

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v4

Okay so I have tried the approach you mentioned in comment 7.

Thanks.

Comment 9

[Kent James (:rkent)]

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.

Comment 10

[Suyash Agarwal (:sshagarwal)]

Thanks.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/dfe18de037d7