Closed Bug 783987 Opened 8 years ago Closed 8 years ago

Add tests for the Promise module

Categories

(Toolkit :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: Paolo, Assigned: Yoric)

References

Details

Attachments

(1 file, 3 obsolete files)

The Promise module is currently tested only indirectly, through the Task module.

Promise is an Add-on SDK module and resides in the "addon-sdk" folder. For
testing it, a folder structure under "addon-sdk" might be the right choice.

With regard to the style of xpcshell tests, we could consider a structure
similar to Add-on SDK test, like in attachment 634364 [details] [diff] [review] of bug 708984. An
alternative can be to use plain asynchronous xpcshell tests with "add_test".
I will handle this. I intend to test both importation with Components.utils.import and with importScripts.
Assignee: nobody → dteller
Attached patch Prototype test suite (obsolete) — Splinter Review
Attaching a test suite. Note that this patch does not contain the test of hostile promises, as that test fails with the current implementation of core.js.
Attachment #654746 - Flags: review?(rFobic)
Attachment #654746 - Flags: review?(paolo.mozmail)
Comment on attachment 654746 [details] [diff] [review]
Prototype test suite

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

This is a first-pass review, with some initial considerations and suggestions. Since this test suite will probably be used as a starting point for testing other modules in Toolkit's addon-sdk directory, I recommend getting feedback from a Toolkit peer sooner rather than later, maybe already on the next iteration of the patch.

In general this needs more source code comments, but it's fine to add them when we've figured out the overall structure.

::: testing/xpcshell/xpcshell.ini
@@ +124,5 @@
>  [include:netwerk/test/unit_ipc/xpcshell.ini]
>  [include:netwerk/cookie/test/unit_ipc/xpcshell.ini]
>  [include:toolkit/components/contentprefs/tests/unit_ipc/xpcshell.ini]
> +[include:toolkit/addon-sdk/tests/unit/xpcshell.ini]
> +skip-if = os == "android"

If this exclusion is done because the Addon SDK is not generally compatible with mobile, while migrating to Toolkit I'd exclude from testing only the individual modules that are actually incompatible. Promise looks compatible with both desktop and mobile.

::: toolkit/addon-sdk/Makefile.in
@@ +19,5 @@
>  EXTRA_JS_MODULES := \
>    loader.js \
>    $(NULL)
>  
> +TEST_DIRS += tests

The conventional directory name in mozilla-central is "test" (though you might have seen "tests" still used somewhere, we've not updated existing names).

::: toolkit/addon-sdk/tests/unit/head.js
@@ +5,5 @@
> +Components.utils.import("resource://gre/modules/commonjs/promise/core.js");
> +
> +let make_test_reporter = function(prefix) {
> +  let utils = {
> +    ok: function ok(t, m) {

So, this is the assertion object passed to the test functions. Is this mimicking a specific interface? Doesn't look like an Addon SDK unit test interface to me, seems more like mochitest.

I've no strong preference about which assertion functions to use, but if we plan to migrate more Addon SDK tests, it might be sensible to be compatible with them, or at least default to plain mozilla-central xpcshell assertions.

@@ +9,5 @@
> +    ok: function ok(t, m) {
> +      do_print(prefix + ": " + m);
> +      do_check_true(t);
> +    },
> +    is: function is(l, r, m) {

In any case, please use meaningful parameter names, either is(actualValue, expectedValue, message) or is(aActualValue, aExpectedValue, aMessage).

@@ +23,5 @@
> +    },
> +    fail: function fail(m) {
> +      utils.ok(false, m);
> +    },
> +    okpromise: function okpromise(t, m) {

The idea of incorporating promises in the testing framework for other modules is good, but we should defer implementation until we have such modules to test. For testing the Promise module itself, I'd obviously rely less on promises.

@@ +36,5 @@
> +    }
> +  };
> +  return utils;
> +};
> +let make_async_test = function(prefix, test) {

Looks like this should be called make_promise_test function, see also the comments below.

::: toolkit/addon-sdk/tests/unit/test_promise.js
@@ +6,5 @@
> +{
> +  do_test_pending();
> +  let tests = [test_notification, test_notification_once,
> +               test_exceptions_do_not_stop_notifications,
> +               test_subsequent_resolves_are_ignored,

I'd rather have tests structured in a different way. The only thing needed to add a new test case should be to cut and paste an existing case, without the need to also add the new case to a list.

xpcshell has "add_test" and "run_next_test" that work well to that effect.

@@ +15,5 @@
> +               test_chaining,
> +               test_resolve_to_rejected,
> +               test_resolve];
> +  let current = 0;
> +  let aux = function aux() {

If you had either "add_test(make_promise_test(function test_that_returns_a_promise() { ... }))" or "add_promise_test(function test_that_returns_a_promise() { ... })", this would make this custom runner unnecessary.

make/add_promise_test would contain the final assertions and the call to run_next_test.

@@ +30,5 @@
> +}
> +
> +// Test that all observers are notified
> +let test_notification = make_async_test("notification",
> +  function notification(test) {

Also, use the function name as the test description (add_test does that already).

Passing the assertion object as an argument to the function, rather than a global variable, has the disadvantage that it should be passed in turn to all the helper functions that are called to verify results. But I'm fine with that, if it's how the Addon SDK usually does unit testing.

I didn't go through all the test cases yet, they look good at first glance.
Attachment #654746 - Flags: review?(paolo.mozmail)
Attached patch Prototype test suite, v2 (obsolete) — Splinter Review
Here is a second version.
I have attempted to follow your recommendations, although I have deviated in a few places.
Attachment #654746 - Attachment is obsolete: true
Attachment #654746 - Flags: review?(rFobic)
Attachment #655549 - Flags: feedback?(paolo.mozmail)
Comment on attachment 655549 [details] [diff] [review]
Prototype test suite, v2

I'm still not convinced about the usefulness of a custom assertion object (if
printing the test name beside each assertion in the test logs is useful, it
should be a feature of the xpcshell framework), and the custom runner could also
be simplified, however I'd also rather not block test coverage for this module
on having a final version of the suite. We can always adapt and simplify the
framework later, when we'll have more modules to test, and maybe some real
Add-on SDK unit tests to migrate.

I'd also like to hear the opinion of a Toolkit peer (Dave, feel free to redirect
the feedback request to the appropriate person if it's not you). If that's OK,
we can then continue with code review to get this version in the tree.
Attachment #655549 - Flags: feedback?(dtownsend+bugmail)
(In reply to Paolo Amadini [:paolo] from comment #5)
> Comment on attachment 655549 [details] [diff] [review]
> Prototype test suite, v2
> 
> I'm still not convinced about the usefulness of a custom assertion object (if
> printing the test name beside each assertion in the test logs is useful, it
> should be a feature of the xpcshell framework),

I really think that it should. However, note that the code will work just as well if you do not use the custom assertion object but just the regular |do_ensure_true|, etc.

> and the custom runner could
> also
> be simplified, however I'd also rather not block test coverage for this
> module
> on having a final version of the suite. We can always adapt and simplify the
> framework later, when we'll have more modules to test, and maybe some real
> Add-on SDK unit tests to migrate.

Sounds good to me.


> I'd also like to hear the opinion of a Toolkit peer (Dave, feel free to
> redirect
> the feedback request to the appropriate person if it's not you). If that's
> OK,
> we can then continue with code review to get this version in the tree.

Great.
Comment on attachment 655549 [details] [diff] [review]
Prototype test suite, v2

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

::: toolkit/addon-sdk/test/unit/head.js
@@ +5,5 @@
> +Components.utils.import("resource://gre/modules/commonjs/promise/core.js");
> +
> +let run_promise_tests = function run_promise_tests(tests, cb) {
> +  let timer = Components.classes["@mozilla.org/timer;1"]
> +     .createInstance(Components.interfaces.nsITimer);

This seems to be unused

@@ +50,5 @@
> +      do_throw(msg);
> +    }
> +  };
> +  return utils;
> +};

This seems like an unnecessary complication to me. We already print out the running test earlier, this is just going to fill the logs with churn.

@@ +57,5 @@
> +  let utils = make_test_reporter(test.name);
> +  return function runtest() {
> +    utils.do_print("Test starting");
> +    try {
> +      let result = test.call(this, utils);

Do you actually intend |this| to be anything specific at this point?

@@ +58,5 @@
> +  return function runtest() {
> +    utils.do_print("Test starting");
> +    try {
> +      let result = test.call(this, utils);
> +      utils.do_print("Test complete");

This isn't really accurate, the test is only complete once the promise is resolved right?

@@ +68,5 @@
> +        try {
> +          utils.do_throw("The test did not return a promise: " + result);
> +        } catch (x) {
> +          exn = x;
> +        }

I don't understand why you're doing this.

@@ +87,5 @@
> +      utils.do_throw("Error " + x + " at " + x.stack);
> +      return Promise.reject();
> +    }
> +  };
> +};

I don't really understand the need for this either, as far as I can see all the tests just return a resolved promise, seems like this could all just be vastly simplified by just returning nothing and carrying on. Can you elaborate on what I'm missing?

::: toolkit/addon-sdk/test/unit/test_promise.js
@@ +13,5 @@
> +  });
> +};
> +
> +// Test that all observers are notified
> +let test_notification = tests.push(make_promise_test(

Why are you assigning the new array length to variables? They don't seem to be used ever and it just complicates the code.
Attachment #655549 - Flags: feedback?(dtownsend+bugmail) → feedback-
(In reply to Dave Townsend (:Mossop) from comment #7)
> Comment on attachment 655549 [details] [diff] [review]
> Prototype test suite, v2
> 
> Review of attachment 655549 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/addon-sdk/test/unit/head.js
> @@ +5,5 @@
> > +Components.utils.import("resource://gre/modules/commonjs/promise/core.js");
> > +
> > +let run_promise_tests = function run_promise_tests(tests, cb) {
> > +  let timer = Components.classes["@mozilla.org/timer;1"]
> > +     .createInstance(Components.interfaces.nsITimer);
> 
> This seems to be unused

Right.

> @@ +50,5 @@
> > +      do_throw(msg);
> > +    }
> > +  };
> > +  return utils;
> > +};
> 
> This seems like an unnecessary complication to me. We already print out the
> running test earlier, this is just going to fill the logs with churn.

My experience when debugging long testsuites is that I generally prefer knowing from first glance during which test the failure took place. This saves from having to find that one line that indicates where tests start/stop. Doubly so for promises, since code can be executed long after we have finished executing the body of the test.

Not a big deal, though, I can get rid of this if you want.

> 
> @@ +57,5 @@
> > +  let utils = make_test_reporter(test.name);
> > +  return function runtest() {
> > +    utils.do_print("Test starting");
> > +    try {
> > +      let result = test.call(this, utils);
> 
> Do you actually intend |this| to be anything specific at this point?
No.

> @@ +58,5 @@
> > +  return function runtest() {
> > +    utils.do_print("Test starting");
> > +    try {
> > +      let result = test.call(this, utils);
> > +      utils.do_print("Test complete");
> 
> This isn't really accurate, the test is only complete once the promise is
> resolved right?

Correct. I will just remove that printout.


> 
> @@ +68,5 @@
> > +        try {
> > +          utils.do_throw("The test did not return a promise: " + result);
> > +        } catch (x) {
> > +          exn = x;
> > +        }
> 
> I don't understand why you're doing this.

Well, I want to 1/ report an error 2/ still return a promise.
Since the only way I know of to report an error in xpcshell is |do_throw| and since |do_throw| throws an exception, I have found no better way.

> 
> @@ +87,5 @@
> > +      utils.do_throw("Error " + x + " at " + x.stack);
> > +      return Promise.reject();
> > +    }
> > +  };
> > +};
> 
> I don't really understand the need for this either, as far as I can see all
> the tests just return a resolved promise, seems like this could all just be
> vastly simplified by just returning nothing and carrying on. Can you
> elaborate on what I'm missing?

Well, this catches (and displays) errors that take place outside a promise. I have had quite a few of these while writing the code, so I figure this safety net is useful.

> 
> ::: toolkit/addon-sdk/test/unit/test_promise.js
> @@ +13,5 @@
> > +  });
> > +};
> > +
> > +// Test that all observers are notified
> > +let test_notification = tests.push(make_promise_test(
> 
> Why are you assigning the new array length to variables? They don't seem to
> be used ever and it just complicates the code.

You are right, this is a leftover if a refactoring, I will fix that.
Attached patch Prototype test suite, v3 (obsolete) — Splinter Review
Same one, with fixes. I have not removed |util| yet, but I can do so if you prefer.
Attachment #655549 - Attachment is obsolete: true
Attachment #655549 - Flags: feedback?(paolo.mozmail)
Attachment #656460 - Flags: review?(dtownsend+bugmail)
Comment on attachment 656460 [details] [diff] [review]
Prototype test suite, v3

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

I'd still like to see the utils stuff gone here
Attachment #656460 - Flags: review?(dtownsend+bugmail) → review-
Ok, I will do that. However, I really think that removing context information from asynchronous tests is a good way to make debugging nightmarish.
Attachment #656460 - Attachment is obsolete: true
Attachment #663387 - Flags: review?(dtownsend+bugmail)
Attachment #663387 - Flags: review?(dtownsend+bugmail) → review+
I don't see any Try results here, so I've gone ahead and done so. I'll land it on inbound if it's green.
https://tbpl.mozilla.org/?tree=Try&rev=d434f0c44563
https://hg.mozilla.org/mozilla-central/rev/0b1ff56c95ae
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.