872229 - Implement add_task() for mochitest browser-chrome

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Mike de Boer [:mikedeboer]]

To enable developers to create unit tests for async code and APIs more conveniently, we will - as a first step - mirror the add_task() method that is also present and actively used in XPCShell.
The implementation will be based on Task.jsm and Promises.

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: Add add_task() API to Mochitest

Initial implementation, feature-complete.

I ported the `browser_colorAnalyzer.js` mochitest as 1) proof of concept, 2) the start of deprecating the `generatorTest()` method of facilitating async mochitests.

I will file a follow-up bug to remove generatorTest() entirely in favor of add_task() - there is one unit test left in m-c that uses this - which will save us plenty of lines in the test framework and will simplify the code flow a lot there.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 757921 [details] [diff] [review]
Add add_task() API to Mochitest

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

::: testing/mochitest/browser-test.js
@@ +80,5 @@
>  Tester.prototype = {
>    EventUtils: {},
>    SimpleTest: {},
> +  Task: null,
> +  Promise: null,

I was about to criticize harshly this misuse of prototype until I realized that the misuse predates your patch. Oh, well. I guess fixing this deserves its own bug anyway.

@@ +431,5 @@
> +      } else {
> +        this.currentTest.addResult(new testMessage("Exception thrown: " + ex));
> +      }
> +      this.currentTest.scope.finish();
> +    }

Since you are introducing |let| in that file, you can take the opportunity to replace |var| by |let| in this function.

@@ +449,5 @@
> +            while (task = this.__tasks.shift()) {
> +              yield task();
> +            }
> +          }.bind(testScope)
> +        ).then(this.nextTest.bind(this), onException.bind(this));

Since you are using Task, I believe that you can do something much more readable than a call to |then|.

@@ +684,5 @@
> +   * If an exception is thrown, an assertion fails, or if a rejected
> +   * promise is yielded, the test function aborts immediately and the test is
> +   * reported as a failure.
> +   *
> +   * To trigger premature (but successful) termination of the function, simply 

Nit: whitespace.

::: testing/mochitest/tests/browser/browser_add_task.js
@@ +16,5 @@
> +}
> +
> +add_task(function asyncTest_no1() {
> +  yield executeWithTimeout();
> +});

Could you also add a test that raises an exception?

::: toolkit/components/places/tests/browser/browser_colorAnalyzer.js
@@ -272,5 @@
> -      } else {
> -        is(allCorrect, true, "perfInSeries colors are all correct");
> -        info("perfInSeries: " + ((new Date()) - t1) + "ms");
> -        nextStep();
> -      }

I don't quite understand the logics of the code you have removed. Can you talk me through it and explain to me why removing it is correct?

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Mike, are you still working on this?

Comment 4

[Mike de Boer [:mikedeboer]]

David, unfortunately I've had to downgrade the prio for me to work on this bug, due to Australis.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Attachment: Add add_task() API to Mochitest, v2

Unbitrotten, applied my own feedback.
If I have time, I'll try and complete the work.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Attachment: Add add_task() API to Mochitest, v3

I, hadn't realized that v1 wasn't working.
Here's a version that should work as intended.
Also, I have taken the opportunity to refine error reporting and add a test that actually tests something.

Comment 7

[(not currently active) Ted Mielczarek]

The bug title says "Mochitest", but this patch only adds this functionality to browser-chrome tests. Is that all you're planning to do here? (Just want to clarify.)

Comment 8

[David Teller [:Yoric] - still alive but not very active]

No, that's an oversight by me. How do you suggest we proceed to have this on all mochitests?

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 787458 [details] [diff] [review]
Add add_task() API to Mochitest, v3

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

Just needs a little cleanup. Not r+ing because I want an answer to the nextTest/finish question.

::: testing/mochitest/browser-test.js
@@ +74,5 @@
>    this._scriptLoader.loadSubScript("chrome://mochikit/content/chrome-harness.js", simpleTestScope);
>    this.SimpleTest = simpleTestScope.SimpleTest;
> +  let taskScope = {};
> +  this.Task = Components.utils.import("resource://gre/modules/Task.jsm", taskScope).Task;
> +  this.Promise = Components.utils.import("resource://gre/modules/commonjs/sdk/core/promise.js", taskScope).Promise;

This is a little awkward, using both the scope argument (which you don't appear to actually need) and the return value of import. You can just pass null as the scope value if you only want to grab properties off of the return value.

@@ +460,5 @@
> +          if (this.nextTest) {
> +            this.nextTest();
> +          } else {
> +            currentTest.scope.finish();
> +          }

I don't really understand this block. Seems like you could just call scope.finish unconditionally. (Although really, currentTest.scope == this inside this task, right?)

@@ +467,2 @@
>          if ("test" in this.currentTest.scope)
>            throw "Cannot run both a generator test and a normal test at the same time.";

Do you want to have this same check in the task branch? Surely you don't want to have a mixed task/regular test.

::: testing/mochitest/tests/browser/Makefile.in
@@ +25,5 @@
>  
>  # Disabled, these are only good for testing the harness' failure reporting
>  #  browser_zz_fail_openwindow.js \
>  #  browser_fail.js \
> +#  browser_fail_add_task.js \

We need to get these in manifests so we can mark them as expected-fail. :-/

::: testing/mochitest/tests/browser/browser_add_task.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Do you actually prefer MPL over a public domain header for tests, or are you just copying boilerplate? Note: http://www.mozilla.org/MPL/headers/

@@ +8,5 @@
> +let test2Complete = false;
> +
> +function executeWithTimeout(aTimeout = 10) {
> +  let deferred = Promise.defer();
> +  setTimeout(function() {

Is there any reason to prefer setTimeout here over just executeSoon?

@@ +28,5 @@
> +
> +add_task(function() {
> +  ok(test1Complete, "We have been through test 1");
> +  ok(test2Complete, "We have been through test 2");
> +  finish();

The harness changes look like they'll call finish for you when all tasks complete, right? Doesn't seem like this call to finish() is necessary (or even desirable).

::: testing/mochitest/tests/browser/browser_fail_add_task.js
@@ +5,5 @@
> +"use strict";
> +
> +// This test is designed to fail.
> +// It ensures that throwing an asynchronous error from add_task will
> +// fail the test.

Nice test, wish we could actually run it as part of the test suite! (Soon...)

Comment 10

[(not currently active) Ted Mielczarek]

(In reply to David Rajchenbach Teller [:Yoric] from comment #8)
> No, that's an oversight by me. How do you suggest we proceed to have this on
> all mochitests?

Let's just make this bug about the browser-chrome part, and file a followup on getting this into regular Mochitests. We should be able to expose Task/Promise via SpecialPowers, and use <script type="application/javascript;version=1.7"> in tests that want to use tasks.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Attachment: Add add_task() API to Mochitest, v4

Applied feedback.
Note that this patch seems to conflict with tests that already import Task or Promise, so I may have missed something.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Try: https://tbpl.mozilla.org/?tree=Try&rev=99395b59afbc

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Attachment: Add add_task() API to Mochitest, v4.1

Removed name collision in a test.

Comment 14

[David Teller [:Yoric] - still alive but not very active]

Try: https://tbpl.mozilla.org/?tree=Try&rev=99395b59afbc

Comment 15

[Mike de Boer [:mikedeboer]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #14)
> Try: https://tbpl.mozilla.org/?tree=Try&rev=99395b59afbc

That looks like the exact same try run as the previous patch?

Comment 16

[David Teller [:Yoric] - still alive but not very active]

My bad, I posted it twice.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b055e87bc4de

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b055e87bc4de