1024053 - Modify test_quarantineFilterMove.js to use Promises

Status: RESOLVED FIXED

(MailNews Core :: Testing Infrastructure, defect)

Description

[Suyash Agarwal (:sshagarwal)]

We intend to modify base/test/unit/test_quarantineFilterMove.js to use
Promises.

Comment 1

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v1

Comment 2

[Kent James (:rkent)]

Attachment: promiseFolderEvent.js

Comment 3

[Kent James (:rkent)]

See the snippet that I added, promiseFolderEvent.js.  Please merge this into PromiseTestUtils and use it.

In your patch, you eliminated this async check, but IIRC sometimes it can give us trouble without it, as you don't actually know that the copy will finish before the item is added.

I've been testing this on another bug. Because you do not know the order that the promises will resolve, create both promises, and then use Promise.all([promise1, promise2]) as the promise to return to the task.

The event to give is created like this:

+const kDeleteOrMoveMsgCompleted = Cc["@mozilla.org/atom-service;1"]
+                                    .getService(Ci.nsIAtomService)
+                                    .getAtom("DeleteOrMoveMsgCompleted");

I don't really know if all of this is needed or not, but the original test used both events, so let's do the same.

Comment 4

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2

Comment 5

[Kent James (:rkent)]

Attachment: promiseFolderEvent.js v2

Sorry, I did not intend for you to add a new EXPORT in the module. Here is an updated version. I also think it would be best to pass in a string and let the test generate the atom. So please use this revised version.

Comment 6

[Kent James (:rkent)]

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

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

::: mailnews/base/test/unit/test_quarantineFilterMove.js
@@ +16,4 @@
>  const gFiles = ["../../../data/bugmail1", "../../../data/bugmail10"];
> +const kDeleteOrMoveMsgCompleted = Cc["@mozilla.org/atom-service;1"]
> +                                    .getService(Ci.nsIAtomService)
> +                                    .getAtom("DeleteOrMoveMsgCompleted");

This will go away with the new promiseFolderEvent version

@@ +42,2 @@
>      gPOP3Pump.files = gFiles;
> +    yield gPOP3Pump.run();

This is where I wanted you to put the new folderEventListener, basically replacing the 1 second delay that was there previously.

@@ +68,4 @@
>      MailServices.copy.CopyMessages(gMoveFolder, messages, gMoveFolder2, false,
> +                                   promiseCopyListener, null, false);
> +    let folderEventPromise = promiseFolderEvent(gMoveFolder, kDeleteOrMoveMsgCompleted);
> +    yield Promise.all([promiseCopyListener.promise, folderEventPromise]);

This Promise.all is basically correct, but should be placed on the filter move, not here.

You were probably trying to avoid a naming conflict by flipping the order of that name, but I would prefer if you keep the same standard of starting these promises with "promise". So call it "promiseMoveMsg" when you use it in the earlier test.

::: mailnews/test/resources/PromiseTestUtils.jsm
@@ +6,5 @@
>   * This file provides utilities useful in using Promises and Task.jsm
>   * with mailnews tests.
>   */
>  
> +const EXPORTED_SYMBOLS = ['PromiseTestUtils', 'promiseFolderEvent'];

As I said, with the new version you will not add a second export here.

@@ +82,5 @@
> +  },
> +  SetMessageId: function(aMessageId) {
> +    if (this.wrapped)
> +      this.wrapped.SetMessageId(aMessageId);
> +  },

It would be worth trying to accumulate two arrays, one with the message keys, the other with the messageIds, and pass those on resolve. So you would have an object:

let result = { messageKeys: [], messageIds: []}

that you would resolve.

@@ +87,5 @@
> +  OnStopCopy: function(aStatus) {
> +    if (this.wrapped)
> +      this.wrapped.OnStopCopy(aStatus);
> +    if (aStatus == Cr.NS_OK)
> +      this._resolve(aStatus);

pass here the result object, not aStatus which is worthless (always NS_OK)

@@ +144,5 @@
>  
>    get promise() { return this._promise; }
>  };
>  
> +function promiseFolderEvent(folder, event) {

To be replaced with new version.

Comment 7

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2.4

I added mfn listener and now the notifications are being caught.
Also, I think now we can remove the folder listener as they are also being caught by the mfn listener itself. So, please let me know if we can remove it as well.

Thanks.

Comment 8

[Kent James (:rkent)]

Comment on attachment 8446984 [details] [diff] [review]
Patch v2.4

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

I don't like the generic promise listener that resolves on multiple callbacks. Please replace with a specific promise listener as mentioned in the other comments.

::: mailnews/base/test/unit/test_quarantineFilterMove.js
@@ +39,2 @@
>      gPOP3Pump.files = gFiles;
> +    let promise1 = PromiseTestUtils.promiseFolderEvent(gMoveFolder);

With your changed design, I cannot tell what callback you expect to resolve this. After you do the changes suggested below, hopefully that will be clear.

::: mailnews/test/resources/PromiseTestUtils.jsm
@@ +180,5 @@
> +      MailServices.mailSession.AddFolderListener(folderListener, Ci.nsIFolderListener.event);
> +    }
> +
> +    let mfnListener = {
> +      msgsMoveCopyCompleted: function(aMove, aSrcMsgs, aDestFolder, aDestMsgs) {

The plan for promiseFolderEvent, which I think is correct, is that the object resolves a promise for one and only one type of response, originally a particular folder event.

By adding all of these extra possible resolutions, you have gone back to the old (bad) design of having possibly unexpected events leading to a resolution. I don't like that.

What I would prefer is that you leave the original promiseFolderEvent to just listen for "OnItemEvent" and add a new method, promiseFolderNotification, that would have a parameter similar to the "event" parameter, but would instead create a listener for a particular callback such as "notifyMsgsDeleted".

If you were clever, you could just pass the name of a method to it such as:

let listener = PromiseTestUtils.promiseFolderNotification(gInboxFolder, "notifyFolderMoveCopyCompleted");

and have the resolve() method pass an array of the call objects to make it very general.

Comment 9

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3

Made the suggested changes.
Please let me know if this is fine.

Thanks.

Comment 10

[Kent James (:rkent)]

Comment on attachment 8449677 [details] [diff] [review]
Patch v3

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

This is looking really good, maybe one more iteration and we'll have it!

::: mailnews/test/resources/PromiseTestUtils.jsm
@@ +55,5 @@
>  };
>  
>  /**
> + * Copy listener that can wrap another listener and trigger a callback.
> + * @param [aWrapped] The copyListener to pass all notifications through to.

You need to specify the type of this parameter, as well as the type of the overall method.

@@ +68,5 @@
> +  this._result = { messageKeys: [], messageIds: [] };
> +};
> +
> +PromiseTestUtils.PromiseCopyListener.prototype = {
> +  OnStartCopy: function() {

While rarely really necessary, I'd like to see a QI here to help make clear that the returned item is an nsIMsgCopyServiceListener.

@@ +178,5 @@
> +    MailServices.mailSession.AddFolderListener(folderListener, Ci.nsIFolderListener.event);
> +  });
> +};
> +
> +PromiseTestUtils.promiseFolderNotification = function(folder, callback) {

This needs documenting of course.

"callback" is not a good name for that, it implies that you are passing a function, when in fact you are passing a string. Maybe "listenerMethod"? Also document that you are expecting a string, and give an example value.

@@ +181,5 @@
> +
> +PromiseTestUtils.promiseFolderNotification = function(folder, callback) {
> +  return new Promise( (resolve, reject) => {
> +    let mfnListener = {};
> +    mfnListener[callback] = function(aFolder) {

You are not using aFolder, it's just function () {

@@ +186,5 @@
> +      let args = Array.prototype.slice.call(arguments);
> +      let flag = true;
> +      for (arg of args) {
> +        if (arg instanceof Ci.nsIMsgFolder) {
> +          if (arg == folder) {

Let's allow no folder specified by doing if (folder && arg instanceof ...

@@ +202,5 @@
> +      }
> +    }
> +    MailServices.mfn.addListener(mfnListener, nsIMFNService.folderAdded |
> +                                              nsIMFNService.msgsMoveCopyCompleted |
> +                                              nsIMFNService.msgAdded | nsIMFNService.msgsClassified);

You should not be adding flags for methods that you are not defining. Actually what you should do is to add the correct flag by just using nsIMFNService[callback]

Comment 11

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3.3

Made the suggested changes.

Thanks.

Comment 12

[Kent James (:rkent)]

Comment on attachment 8458426 [details] [diff] [review]
Patch v3.3

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

Looks good to me. You only asked for feedback+, but you could have review+ if you want (one little nit)

::: mailnews/test/resources/PromiseTestUtils.jsm
@@ +184,5 @@
> +};
> +
> +/**
> + * Folder listener to resolve a promise when a certain folder event occurs.
> + * 

Nit: trailing space

Comment 13

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v4

Thanks.
Carrying over review from comment 12.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/8c83b0799edf