Closed Bug 1024053 Opened 5 years ago Closed 5 years ago

Modify test_quarantineFilterMove.js to use Promises

Categories

(MailNews Core :: Testing Infrastructure, defect)

defect
Not set

Tracking

(thunderbird34 fixed)

RESOLVED FIXED
Thunderbird 34.0
Tracking Status
thunderbird34 --- fixed

People

(Reporter: sshagarwal, Assigned: sshagarwal)

References

Details

Attachments

(1 file, 7 obsolete files)

We intend to modify base/test/unit/test_quarantineFilterMove.js to use
Promises.
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #8438643 - Flags: review?(kent)
Attached file promiseFolderEvent.js (obsolete) —
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.
Attachment #8438643 - Flags: review?(kent) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #8438643 - Attachment is obsolete: true
Attachment #8439979 - Flags: review?(kent)
Attached file promiseFolderEvent.js v2 (obsolete) —
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.
Attachment #8439348 - Attachment is obsolete: true
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.
Attachment #8439979 - Flags: review?(kent) → review-
Attached patch Patch v2.4 (obsolete) — Splinter Review
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.
Attachment #8439979 - Attachment is obsolete: true
Attachment #8446984 - Flags: feedback?(kent)
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.
Attachment #8446984 - Flags: feedback?(kent) → feedback-
Attached patch Patch v3 (obsolete) — Splinter Review
Made the suggested changes.
Please let me know if this is fine.

Thanks.
Attachment #8446984 - Attachment is obsolete: true
Attachment #8449677 - Flags: feedback?(kent)
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]
Attachment #8449677 - Flags: feedback?(kent) → feedback+
Attached patch Patch v3.3 (obsolete) — Splinter Review
Made the suggested changes.

Thanks.
Attachment #8449677 - Attachment is obsolete: true
Attachment #8458426 - Flags: feedback?(kent)
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
Attachment #8458426 - Flags: review+
Attachment #8458426 - Flags: feedback?(kent)
Attachment #8458426 - Flags: feedback+
Attached patch Patch v4Splinter Review
Thanks.
Carrying over review from comment 12.
Attachment #8440132 - Attachment is obsolete: true
Attachment #8458426 - Attachment is obsolete: true
Attachment #8460739 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/8c83b0799edf
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 34.0
Blocks: 1046998
You need to log in before you can comment on or make changes to this bug.