Closed Bug 1018300 Opened 10 years ago Closed 10 years ago

Modify test_pop3MoveFilter.js to use Promises.

Categories

(MailNews Core :: Testing Infrastructure, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 33.0

People

(Reporter: sshagarwal, Assigned: sshagarwal)

References

Details

Attachments

(1 file, 5 obsolete files)

Attached patch Patch v1 (obsolete) — Splinter Review
As test_pop3MoveFilter.js has some async functions, we intend to
modify it to use Promises.
As this test uses POP3pump, this patch is based on the patch from
bug 1017879.
Attachment #8431663 - Flags: review?(kent)
Attached patch Patch v1.2 (obsolete) — Splinter Review
Attachment #8431663 - Attachment is obsolete: true
Attachment #8431663 - Flags: review?(kent)
Attachment #8431666 - Flags: review?(kent)
Comment on attachment 8431666 [details] [diff] [review]
Patch v1.2

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

Please incorporate some of these comments and redo this.

::: mailnews/local/test/unit/test_pop3MoveFilter.js
@@ +46,2 @@
>      gPOP3Pump.files = gFiles;
> +    gPOP3Pump.deferred = Promise.defer();

In the modified version of POP3pump after I incorporate jcranmer's review comments,  this line will not be needed,  and should be removed.

@@ +78,5 @@
>        let hdr = enumerator.getNext().QueryInterface(Ci.nsIMsgDBHdr);
>        keys.push(hdr.messageKey);
>        hdrs.push(hdr);
>      }
>      gMoveFolder.fetchMsgPreviewText(keys, keys.length, false, null, asyncResults);

Yes the original was like this, but this looks odd to me. asyncResults is an attribute, so I would expect:

do_check_false(gMoveFolder.fetchMsgPreviewText(keys, keys.length, false, null));

and you can delete the "let asyncResults = ..." earlier. Try this and see if it works.

@@ +85,5 @@
> +  },
> +  function endTest() {
> +    let thread = gThreadManager.currentThread;
> +    while (thread.hasPendingEvents())
> +      thread.processNextEvent(true);

Why did you separate the "gPOP3Pump = null;" from this method? Just add it here. If something is not working when you do that, then it is an indicator of some other problem.

@@ +120,3 @@
>  }
>  
> +add_task(function *doTest() {

Please so not add complex loops where you add_task inside of add_task. Just do all of the setup that you need, that is calling add_task for each test, as part of the run_test method. That includes adding the final add_task(exitTest).

@@ +132,5 @@
>    OnItemAdded: function OnItemAdded(aParentItem, aItem) {
>      this._showEvent(aParentItem, "OnItemAdded");
>    },
>    OnItemRemoved: function OnItemRemoved(aParentItem, aItem) {
>      this._showEvent(aParentItem, "OnItemRemoved");

So by removing the "doTest" here you are essentially converting these tests from sync to async. You really don;t want to do that, as what you will find is that it may work on some platforms and not the others.

What I want you do to is to define a generic PromiseFolderListener in PromiseTestUtils, that has an optional parameter for the method that is used to resolve the promise. In you case, it will be "OnItemRemoved". The methods should pick something intelligent to use as the returned item in deferred.resolve(), typically the item that is mentioned in the callback.

Then go back and make this whole test async again using yield PromiseTestUtils.PromiseFolderListener("OnItemRemoved").

@@ +144,5 @@
>           "\n");
>    }
>  };
>  
> +function exitTest()

Just include this as part of the gTestArray, and you don't need any special processing that do add_task(exitTest)
Attachment #8431666 - Flags: review?(kent) → review-
I'm sorry that there is such a moving target in the implementation of Promise in tests, but you need to take the comments from Bug 1018543 and incorporate them in the similar test here. Also this needs to be done, and the changes here separated from the changes in Bug 827048 before I can make any progress with Bug 827048
Setting the dependency on bug 1018542 as both these bugs require editing the same file
PromiseTestUtils.jsm.
Depends on: 1018542
Attached patch Patch v2 (obsolete) — Splinter Review
Done in the same way as bug 1018542. Please let me know if this is the
correct way to convert async tests to Promises.
Attachment #8431666 - Attachment is obsolete: true
Attachment #8435510 - Flags: feedback?(kent)
Attached patch Patch v2.1 (obsolete) — Splinter Review
I forgot to remove this._deferred.
Attachment #8435510 - Attachment is obsolete: true
Attachment #8435510 - Flags: feedback?(kent)
Attachment #8435519 - Flags: feedback?(kent)
Blocks: 1018543
To be applied over the patch from bug 1017879 and bug 1018542.
Comment on attachment 8435519 [details] [diff] [review]
Patch v2.1

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

Generally I think this is correct with the noted issues that need addressing.

::: mailnews/local/test/unit/test_pop3MoveFilter.js
@@ +9,5 @@
> +Components.utils.import("resource:///modules/mailServices.js");
> +Components.utils.import("resource://gre/modules/Promise.jsm");
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +Components.utils.import("resource://testing-common/mailnews/PromiseTestUtils.jsm");

Is there some reason why you are changing the order of the imports here? If not, then please add you new import in a way that does not require deleting and readding the existing import.

@@ +114,3 @@
>                                               Ci.nsIFolderListener.event |
>                                                 Ci.nsIFolderListener.added |
>                                                 Ci.nsIFolderListener.removed);

This listener as far as I can tell does not play any role in the test, so I am not sure why it is here. Can you please just remove it and see what happens?

What you have done certainly does not make sense, as you add a promise listener but never actually use the promise.

::: mailnews/test/resources/PromiseTestUtils.jsm
@@ +54,4 @@
>  };
>  
>  /**
> + * Folder listener that can wrap another listener and trigger a callback.

If my earlier comment is true, then we will not actually be using this in this test. There are some design issues we need to discuss to optimize this design, but I think it would be better to discuss those in a test that actually needs this. You are welcome to try and find one, but if we turn out not using this here it would be better to delete it rather than to land it untested.
Attachment #8435519 - Flags: feedback?(kent) → feedback+
Attached patch Patch v2.4 (obsolete) — Splinter Review
(In reply to Kent James (:rkent) from comment #8)
> Comment on attachment 8435519 [details] [diff] [review]
> Patch v2.1
> ::: mailnews/local/test/unit/test_pop3MoveFilter.js
> @@ +9,5 @@
> > +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> > +Components.utils.import("resource://testing-common/mailnews/PromiseTestUtils.jsm");
> 
> Is there some reason why you are changing the order of the imports here? If
> not, then please add you new import in a way that does not require deleting
> and readding the existing import.
Ya, actually I have noticed that we put imports in lexical order, so I was
just trying to do that. If you say, I'll remove it.

> @@ +114,3 @@
> >                                               Ci.nsIFolderListener.event |
> >                                                 Ci.nsIFolderListener.added |
> 
> This listener as far as I can tell does not play any role in the test, so I
> am not sure why it is here. Can you please just remove it and see what
> happens?
The test passes after removing it. So, I have removed it
and the added listener from PromiseTestUtils.jsm

> What you have done certainly does not make sense, as you add a promise
> listener but never actually use the promise.
Ya, using |yield promiseFolderListener.promise| in run_test() was making
an early return from the test, so I removed it.

> ::: mailnews/test/resources/PromiseTestUtils.jsm
> @@ +54,4 @@
> > + * Folder listener that can wrap another listener and trigger a callback.
> 
> If my earlier comment is true, then we will not actually be using this in
> this test. There are some design issues we need to discuss to optimize this
> design, but I think it would be better to discuss those in a test that
> actually needs this.
Okay, removed it.

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

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

This looks fine to me except for the nits. Unfortunately we need to wait for the Pop3pump changes to land before we can finish this. So I'll feedback+ for now, and we'll lokk at it one more time after pop3Pump conversion to promises lands.

::: mailnews/local/test/unit/test_pop3MoveFilter.js
@@ +10,3 @@
>  Components.utils.import("resource:///modules/mailServices.js");
>  Components.utils.import("resource://gre/modules/Services.jsm");
> +Components.utils.import("resource://gre/modules/Promise.jsm");

I don't think that you need to add Promise.jsm

@@ +70,2 @@
>    },
> +  function *verifyMessages() {

This is not a generator, so no * in *verifyMessages
Attachment #8436255 - Flags: review?(kent) → feedback+
Comment on attachment 8436255 [details] [diff] [review]
Patch v2.4

So r=me after the two nits are fixed.
Attachment #8436255 - Flags: review+
Attached patch Patch v3Splinter Review
Thanks.
Attachment #8436255 - Attachment is obsolete: true
Attachment #8438141 - Flags: review+
Status: NEW → ASSIGNED
Keywords: checkin-needed
Blocks: 1023700
Blocks: 827048
https://hg.mozilla.org/comm-central/rev/1a9fcf6700eb
Status: ASSIGNED → RESOLVED
Closed: 10 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.