1047883 - Modify test_offlinePlayback.js to use promises.

Status: RESOLVED FIXED

(MailNews Core :: Testing Infrastructure, defect)

Description

[Suyash Agarwal (:sshagarwal)]

We intend to convert imap/test/unit/test_offlinePlayback.js to use Promises.

Comment 1

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v1

So, I tried converting this test to use Promises, but it started failing.
I am not sure what the cause is. Is it that I am not giving sufficient time
to the previous tests to finish before proceeding to the next one?

Comment 2

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch 1.1

There was a typo in my patch. Thanks to rkent for pointing that out.
But, the test still doesn't pass for me.

Comment 3

[Kent James (:rkent)]

Comment on attachment 8466707 [details] [diff] [review]
Patch v1

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

I think this is a good start, but there are a number of issues that I mentioned.

::: mailnews/imap/test/unit/test_offlinePlayback.js
@@ +9,3 @@
>  
>  load("../../../resources/logHelper.js");
>  load("../../../resources/asyncTestUtils.js");

You should remove this.

@@ +24,5 @@
>      gSecondFolder = rootFolder.getChildNamed("secondFolder")
>                        .QueryInterface(Ci.nsIMsgImapMailFolder);
>      gThirdFolder =  rootFolder.getChildNamed("thirdFolder")
>                        .QueryInterface(Ci.nsIMsgImapMailFolder);
>      IMAPPump.incomingServer.closeCachedConnections();

If the test continues to fail after fixing the typo, perhaps we should try to add a promise-based delay at the points where you removed them. I don't have a good feel for why such a long delay was needed in the first place.

@@ +45,5 @@
>      msgHdr2.folder.markMessagesFlagged(headers2, true);
>      MailServices.copy.CopyMessages(IMAPPump.inbox, headers1, gSecondFolder, true, null,
>                                     null, true);
>      MailServices.copy.CopyMessages(IMAPPump.inbox, headers2, gThirdFolder, true, null,
>                                     null, true);

I really think that we should not assume this is sync. Instead, create three promiseCopyListeners, and yield Promise.all() with the three of them.

@@ +52,3 @@
>      MailServices.copy.CopyFileMessage(file, IMAPPump.inbox, null, false, 0,
> +                                      "", promiseCopyListener, null);
> +    yield promiseCopyListener.proimse;

As I pointed out in irc, this is a typo.

@@ +66,2 @@
>      if (gOfflineManager.inProgress)
>        do_timeout(2000, updateSecondFolder);

This does not make any sense anymore. At the very least, add a do_print so that you can see when gOfflineManager.inProgress is true at this point. If sometime true, then perhaps we need to add a method that polls gOfflineManager.inProgress using a promise, resolving when false.

Comment 4

[Kent James (:rkent)]

Attachment: 1047883_v2.patch

The issues went away when I reinserted the original delays, as in this patch. Much as I hate delays, they were there in the original, and we have bigger fish to fry than getting rid of them. I would like them implemented though through a standard promise as shown.

This is basically your patch with some suggested changes. If you are OK with this, I will review it (which I have not done yet) and we can land it.

Comment 5

[Suyash Agarwal (:sshagarwal)]

Comment on attachment 8467288 [details] [diff] [review]
1047883_v2.patch

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

Ya, this looks great to me! :)

::: mailnews/imap/test/unit/test_offlinePlayback.js
@@ +46,5 @@
>      msgHdr2.folder.markMessagesFlagged(headers2, true);
>      MailServices.copy.CopyMessages(IMAPPump.inbox, headers1, gSecondFolder, true, null,
>                                     null, true);
>      MailServices.copy.CopyMessages(IMAPPump.inbox, headers2, gThirdFolder, true, null,
>                                     null, true);

Shouldn't we add promiseCopyListeners for the above two actions instead of relying on the fact that these are sync operations now?

I tried to add them and the test still passes.
Please let me know if we need to add them, else the rest seems okay.

Comment 6

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2

I have tried to incorporate the changes mentioned in comment 5.
Please let me know if this is okay.

Thanks.

Comment 7

[Kent James (:rkent)]

Comment on attachment 8471008 [details] [diff] [review]
Patch v2

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

r=me with the changes implemented.

::: mailnews/imap/test/unit/test_offlinePlayback.js
@@ +9,3 @@
>  
>  load("../../../resources/logHelper.js");
>  load("../../../resources/asyncTestUtils.js");

nit: You neglected to remove the references to logHelper and asyncTestUtils. Please remove these.

@@ +72,2 @@
>      if (gOfflineManager.inProgress)
>        do_timeout(2000, updateSecondFolder);

As stated earlier, these two lines starting "if (gOfflineManager.inProgress)" make no sense, you cannot simply call a generator as a function. Just remove them, I really think they were overkill in the original test. If we see intermittent failures we may have to try to understand why this was needed.

Comment 8

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch for check-in

Made the suggested changes.
The test still passes.

Thanks.

Comment 9

[Patrick Cloke [:clokep]]

https://hg.mozilla.org/comm-central/rev/14f6b667b0e4