Closed Bug 1018543 Opened 5 years ago Closed 5 years ago

Modify test_pop3FilterActions.js to use Promises

Categories

(MailNews Core :: Testing Infrastructure, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 33.0

People

(Reporter: sshagarwal, Assigned: sshagarwal)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch pop3FilterActions.patch (obsolete) — Splinter Review
We intend to use promises for async functions in
test_pop3FilterActions.js

This is an attempt for the same.
Attachment #8432046 - Flags: review?(kent)
Comment on attachment 8432046 [details] [diff] [review]
pop3FilterActions.patch

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

::: mailnews/local/test/unit/test_pop3FilterActions.js
@@ +26,3 @@
>  const gTestArray =
>  [
> +  function *createFilters() {

This is not a generator, so no "*"

@@ +65,2 @@
>      } catch (ex) {
>        do_check_true(ex.result == Cr.NS_ERROR_NOT_INITIALIZED);

I checked the code, and contrary to normal practice, this error means that an async folder parse has begun. So we need a yield promiseUrlListener.promise at this point (and this function becomes a generator and needs function*)

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

I doubt that we need this whole function. Please try without it.

Actually what I tested and seems to work is to get rid of all of these thread spinning functions, and just add the gPOP3Pump = null; here (eliminating your exitTest().)

@@ +121,5 @@
>  
>    MailServices.mailSession.AddFolderListener(FolderListener,
>                                               Ci.nsIFolderListener.event |
>                                                 Ci.nsIFolderListener.added |
>                                                 Ci.nsIFolderListener.removed);

All of this folderListener stuff makes no sense in this test. Just remove this, as well as the whole FolderListener method. It looks like it was copied from another test but actually play no role here.

@@ +127,5 @@
>  }
>  
> +add_task(function *doTest() {
> +  for (let foo of gTestArray)
> +    add_task(foo);

Don't do add_task within add_task. Just do this operation right before run_next_test() above.

@@ +132,2 @@
>  
> +  add_task(exitTest);

and you will be eliminating this as well.

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

Just add this to the list of tasks in gTestArray, there is no need to handle it as a special case. Or add the gPPO3Pump = null; earlier as I suggest.
Attachment #8432046 - Flags: review?(kent) → review-
Attached patch Patch v1.6 (obsolete) — Splinter Review
Tried to incorporate the suggested changes.
Attachment #8432046 - Attachment is obsolete: true
Attachment #8433090 - Flags: review?(kent)
Depends on: 1017879
Comment on attachment 8433090 [details] [diff] [review]
Patch v1.6

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

This is OK with the one important change.

This patch cannot be landed until the dependent bug 1017879 also lands.

::: mailnews/local/test/unit/test_pop3FilterActions.js
@@ +62,4 @@
>      try {
> +      localAccountUtils.inboxFolder
> +                       .getDatabaseWithReparse(promiseUrlListener, null);
> +      yield promiseUrlListener.promise;

This is not where I meant for you to put this. Here it never gets called, because the previous statement throws. It should be in the catch before the return.
Attachment #8433090 - Flags: review?(kent) → review+
Attached patch Patch v2 (obsolete) — Splinter Review
Same approach as the other Promise based tests.

Thanks.
Attachment #8433090 - Attachment is obsolete: true
Attachment #8435522 - Flags: review?(kent)
Depends on: 1018542, 1018300
To be applied over the patch from bug 1017879, bug 1018542 and bug 1018300.
Comment on attachment 8435522 [details] [diff] [review]
Patch v2

Looks good with one nit:

This line in run_test:

  gMoveFolder = localAccountUtils.rootFolder.createLocalSubfolder("MoveFolder");

does nothing, and generates a warning. It must have been copied originally from another test.  Please delete it.

r=me with nit fixed.
Attachment #8435522 - Flags: review?(kent) → review+
Thanks.
Attachment #8435522 - Attachment is obsolete: true
Attachment #8438623 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/791d5276e5cb
Status: ASSIGNED → RESOLVED
Closed: 5 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.