819033 - Native support for Task-based tests in xpcshell runner

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

[Gregory Szorc [:gps]]

Attachment: Add add_task_test() to xpcshell runner, v1

Task.jsm and it's main function Task.spawn() are friggin awesome, especially for unit tests where you are often chaining lots of async, promise-based APIs together.

I found myself writing a lot of boilerplate in my xpcshell tests to deal with Task.spawn so failures would be handled properly. So, I thought "why not add this to the xpcshell runner itself." So, I did.

The attached patch adds a new function, add_task_test(). Conceivably, I could probably hack around things and overload add_test() to accept generator functions. But, I believe that would involve some generator light magic. I think the approach I took is simplest.

While I was here, I also gave add_test() some basic test coverage. But only a little.

The only nit I have with the implementation is that a do_check_* failure inside the generator function yields an exceptionally long stack stack. There are a lot of frames from Task.jsm and promise's core.js in there. But, the main failure is still on top of the stack, so it's easy to see what went wrong. I would be happy with a follow-up bug to sanitize the stack trace before printing.

Ted gets primary review because this is all xpcshell runner code.

I'd like Yoric and rnewman to offer feedback on the implementation and utility of this.

I'd like to write the unit tests for bug 813833 using this. So, a review within the next week or two would be appreciated.

Comment 1

[Richard Newman [:rnewman]]

Comment on attachment 689334 [details] [diff] [review]
Add add_task_test() to xpcshell runner, v1

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

::: testing/xpcshell/head.js
@@ +884,5 @@
> + *
> + * Unlike add_test(), there is no need to call run_next_test(). The next test
> + * will run automatically as soon the task function is exhausted. To trigger
> + * premature (but successful) termination of the function, throw a Task.Result
> + * instance.

Add an example of this.

@@ +918,5 @@
>      if (gTestIndex < gTests.length) {
>        do_test_pending();
> +      let isTask;
> +      [isTask, gRunningTest] = gTests[gTestIndex++];
> +      print("is task? " + isTask);

Kill this. Fold it into the next line if you want.

@@ +923,5 @@
> +      print("TEST-INFO | " + _TEST_FILE + " | Starting " + gRunningTest.name);
> +
> +      if (isTask) {
> +        Task.spawn(gRunningTest).then(run_next_test, function onError (error) {
> +          do_throw(error);

Just 

  Task.spawn(gRunningTest).then(run_next_test, do_throw);

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 689334 [details] [diff] [review]
Add add_task_test() to xpcshell runner, v1

Paolo was already integrating something similar in Places, I think he may be interested in feedbacking this.

fwiw we were using just add_task() without a _test suffix

Comment 3

[Gregory Szorc [:gps]]

Attachment: Add add_task_test() to xpcshell runner, v2

Incorporated feedback. (I can't believe I left that print statement in there.)

Comment 4

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

I will take a look at the patch once I have popped a few patches from my review queue. Just one bit on the intention: I have somewhere in my code a function |TaskUtils.spawn| that behaves exactly as |Task.spawn| but also reports errors using |Components.utils.reportError|.

Wouldn't it be better to split this bug in two? One for error reporting and one for native support?

Comment 5

[Gregory Szorc [:gps]]

This patch merely allows task functions to be registered as tests. There is nothing to it besides making it easier to run tasks as tests. I don't think there is anything to split.

If you create a utility function that does more intelligent error handling, the code in this patch could conceivably switch to use that.

Comment 6

[Marco Bonardo [:mak]]

if you check the head_common changes here https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=763295&attachment=683479
Paolo used a different way to distinguish tasks from functions, basically add_task() creates a wrapper function, so that gTests doesn't require modifications

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 689349 [details] [diff] [review]
Add add_task_test() to xpcshell runner, v2

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

I don't really know anything about Task.jsm, but the harness changes look good. Thanks for adding tests to selftest.py!

::: testing/xpcshell/head.js
@@ +901,5 @@
> + * add_task_test(function test_early_return() {
> + *   let result = yield somethingThatReturnsAPromise();
> + *
> + *   if (!result) {
> + *     throw Task.Result();

Can you explain with a meta-comment what this actually does?

Comment 8

[Gregory Szorc [:gps]]

(In reply to Marco Bonardo [:mak] (Away 10-11 Dec) from comment #6)
> if you check the head_common changes here
> https://bugzilla.mozilla.org/page.cgi?id=splinter.
> html&bug=763295&attachment=683479
> Paolo used a different way to distinguish tasks from functions, basically
> add_task() creates a wrapper function, so that gTests doesn't require
> modifications

I could do that, sure. I don't think it makes much difference. I agree having one path for test execution is simpler. Although, it does introduce an extra stack frame. I think the meh principle applies.

Ted, et al: do you like add_task_test or add_task? We could even alias them if we wanted!

Comment 9

[(not currently active) Ted Mielczarek]

I don't have a preference. Use your judgement, but pick just one please.

Comment 10

[:Paolo Amadini]

Comment on attachment 689349 [details] [diff] [review]
Add add_task_test() to xpcshell runner, v2

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

We went for add_task in Places (after trying calling it add_test_task) because:
* It's shorter, so it's easier to give clearer names to test functions and still stay under 80 characters.
* Usually, you already write "test" in the function, like add_task(function test_something() { ... }).
* You don't have to remember whether it's add_test_task or add_task_test.

::: testing/xpcshell/head.js
@@ +864,5 @@
>   * @return the test function that was passed in.
>   */
>  let gTests = [];
>  function add_test(func) {
> +  gTests.push([false, func]);

I like defining whether the function is a task explicitly, like this, more than using a wrapper function like I had to do in Places to avoid modifying the harness.

Unfortunately, some tests populate gTests directly. I think we should either go with the wrapper function approach, or rename gTests to something else, so that it becomes private to the harness, then search the tree for direct uses of gTests, and fix them to use add_test instead.

A tryserver build with this patch applied will show those uses.

@@ +869,5 @@
>    return func;
>  }
>  
> +// We lazy import Task.jsm so we don't incur a run-time penalty for all tests.
> +let Task;

Variables defined here become available to tests, so this should be renamed to something that cannot accidentally conflict with variables defined in test files.

@@ +886,5 @@
> + * will run automatically as soon the task function is exhausted. To trigger
> + * premature (but successful) termination of the function, throw a Task.Result
> + * instance.
> + *
> + * Example usage:

You wrote better examples than mine :-)

Something I would mention in the comment, taken from my patch:

Test files should call run_next_test() inside run_test() to execute all the
asynchronous tests, as usual.  Test files may include both function added
with add_test() as well as function added with add_task().

@@ +901,5 @@
> + * add_task_test(function test_early_return() {
> + *   let result = yield somethingThatReturnsAPromise();
> + *
> + *   if (!result) {
> + *     throw Task.Result();

You can just call "return" in a generator, if you don't return a value. Since we ignore any result of the task, just "return" here would be clearer.

@@ +909,5 @@
> + * });
> + */
> +function add_task_test(func) {
> +  if (!Task) {
> +    Task = Components.utils.import("resource://gre/modules/Task.jsm").Task;

This will import the module in the global scope; you probably want to import to a private scope specified as the second argument of Cu.import.

Tests that want to call Task.spawn would have to define their own module lazy getter.

@@ +931,5 @@
> +      [isTask, gRunningTest] = gTests[gTestIndex++];
> +      print("TEST-INFO | " + _TEST_FILE + " | Starting " + gRunningTest.name);
> +
> +      if (isTask) {
> +        Task.spawn(gRunningTest).then(run_next_test, do_throw);

You probably want to call do_report_unexpected_exception rather than do_throw so that you get the stack trace of the place where the exception was thrown.

Comment 11

[Gregory Szorc [:gps]]

(In reply to Paolo Amadini [:paolo] from comment #10)

Great feedback!

> Unfortunately, some tests populate gTests directly. I think we should either
> go with the wrapper function approach, or rename gTests to something else,
> so that it becomes private to the harness, then search the tree for direct
> uses of gTests, and fix them to use add_test instead.

Wat?! I will search for and eradicate all such abuses!
 
> @@ +869,5 @@
> >    return func;
> >  }
> >  
> > +// We lazy import Task.jsm so we don't incur a run-time penalty for all tests.
> > +let Task;
> 
> Variables defined here become available to tests, so this should be renamed
> to something that cannot accidentally conflict with variables defined in
> test files.

Another reason why I want to significantly refactor the xpcshell runner logic so most things are defined as modules, not #included files. But, that's for another bug.

Comment 12

[Gregory Szorc [:gps]]

https://tbpl.mozilla.org/?tree=Try&rev=715f351fbf38

Comment 13

[Gregory Szorc [:gps]]

Attachment: Add add_task() to xpcshell runner, v3

Incorporated feedback.

Re-requesting review because I _-prefixed gTests and friends.

Comment 14

[Gregory Szorc [:gps]]

Attachment: Part 0: Remove references to private test runner variables from tests, v1

This is what I found in a local test run. Try may find some more.

Comment 15

[Gregory Szorc [:gps]]

Attachment: Part 0: Remove references to private test runner variables from tests, v2

Sorry - forgot to qref.

Comment 16

[:Paolo Amadini]

I've just landed on inbound the patch that defines the local add_task in Places.
Sorry for the race, but it was at the top of a queue of 9 bugs that has already
waited for too long, and was getting bitrotten already!

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=94d11f7d9114

The upside is that these changes might have already fixed some instances of
direct use of the gTests variable.

Comment 17

[Gregory Szorc [:gps]]

(In reply to Paolo Amadini [:paolo] from comment #16)
> I've just landed on inbound the patch that defines the local add_task in
> Places.

Well, if you define that function in a test file, it will just override what's in head.js. So, no harm done. I think.

Comment 18

[Marco Bonardo [:mak]]

I'm sorry but I'm away for a couple days, so I won't be able to review these at least untile Wed. feel free to ask paolo, or land whatever is needed on top of the current tests. I just care they keep passing, rs=me if that is true.

Comment 19

[(not currently active) Ted Mielczarek]

Comment on attachment 690113 [details] [diff] [review]
Add add_task() to xpcshell runner, v3

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

::: testing/xpcshell/head.js
@@ +862,5 @@
>   * async tests.
>   *
>   * @return the test function that was passed in.
>   */
> +let _gTests = [];

I approve. I didn't know we had tests touching this directly. :-/

Comment 20

[(not currently active) Ted Mielczarek]

(In reply to Paolo Amadini [:paolo] from comment #10)
> We went for add_task in Places (after trying calling it add_test_task)
> because:
[...]
> * You don't have to remember whether it's add_test_task or add_task_test.

This is a great reason, I am totally in favor.

Comment 21

[Gregory Szorc [:gps]]

Comment on attachment 690117 [details] [diff] [review]
Part 0: Remove references to private test runner variables from tests, v2

test_486978_sort_by_date_queries.js was bit rotted. But, the one-liner from test_sql_guid_functions.js should be good for review.

Comment 22

[:Paolo Amadini]

Comment on attachment 690117 [details] [diff] [review]
Part 0: Remove references to private test runner variables from tests, v2

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

I missed this easy review yesterday. Thanks for fixing these tests!

::: toolkit/components/places/tests/unit/test_sql_guid_functions.js
@@ +13,5 @@
>   *        The guid to check.
>   */
>  function check_invariants(aGuid)
>  {
> +  print("Checking guid '" + aGuid + "'");

You can use "do_print" here.

Comment 23

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/907a846d368f
https://hg.mozilla.org/integration/mozilla-inbound/rev/fac650c31656

Comment 24

[Ed Morley [:emorley]]

Sorry had to backout for:
{
TEST-PASS | /builds/slave/talos-slave/test/build/xpcshell/tests/browser/components/places/tests/unit/head_bookmarks.js -> file:///builds/slave/talos-slave/test/build/xpcshell/tests/toolkit/components/places/tests/head_common.js | [remove_bookmarks_html : 487] false == false

TEST-PASS | /builds/slave/talos-slave/test/build/xpcshell/tests/browser/components/places/tests/unit/head_bookmarks.js -> file:///builds/slave/talos-slave/test/build/xpcshell/tests/toolkit/components/places/tests/head_common.js | [remove_all_JSON_backups : 543] false == false
###!!! ASSERTION: The statement 'SELECT h.id, h.url, IFNULL(b.title, h.title), h.rev_host, h.visit_count, h.last_visit_date, f.url, null, b.id, b.dateAdded, b.lastModified, b.parent, null, h.frecency, b.position, b.type, b.fk, b.guid FROM moz_bookmarks b LEFT JOIN moz_places h ON b.fk = h.id LEFT JOIN moz_favicons f ON h.favicon_id = f.id WHERE b.parent = :parent ORDER BY b.position ASC' failed to compile with the error message ''.: 'Error', file ../../../dist/include/mozilla/storage/StatementCache.h, line 125
imgLoader::SupportImageWithMimeType(char const*)+0x00989282 [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x00C3F802]
JSD_GetValueForObject+0x0007D794 [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x014D8D84]
JSD_GetValueForObject+0x000799CE [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x014D4FBE]
NS_InvokeByIndex_P+0x0000021D [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x019A796D]
xpc_LocalizeContext(JSContext*)+0x00019D06 [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x01162976]
xpc_LocalizeContext(JSContext*)+0x00017B3A [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x011607AA]
xpc_LocalizeContext(JSContext*)+0x00021BB1 [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x0116A821]
JS_EnumerateDiagnosticMemoryRegions(int (*)(void*, unsigned long))+0x0003022C [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x0209FD6C]
JS_EnumerateDiagnosticMemoryRegions(int (*)(void*, unsigned long))+0x0002B48C [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x0209AFCC]
JS_EnumerateDiagnosticMemoryRegions(int (*)(void*, unsigned long))+0x00027CD3 [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x02097813]
JS_EnumerateDiagnosticMemoryRegions(int (*)(void*, unsigned long))+0x00020475 [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x0208FFB5]
JS_EnumerateDiagnosticMemoryRegions(int (*)(void*, unsigned long))+0x0002B460 [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x0209AFA0]
JS_EnumerateDiagnosticMemoryRegions(int (*)(void*, unsigned long))+0x0002BBCE [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x0209B70E]
JS_CallFunctionValue(JSContext*, JSObject*, JS::Value, unsigned int, JS::Value*, JS::Value*)+0x0000014B [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x01FF7ACB]
xpc_LocalizeContext(JSContext*)+0x0000F3FA [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x0115806A]
xpc_LocalizeContext(JSContext*)+0x00007402 [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x01150072]
nsXPTCStubBase::Stub249()+0x00000315 [/builds/slave/talos-slave/test/build/FirefoxNightlyDebug.app/Contents/MacOS/XUL +0x019A8C95]
###!!! ASSERTION: The statement 'SELECT h.id, h.url, IFNULL(b.title, h.title), h.rev_host, h.visit_count, h.last_visit_date, f.url, null, b.id, b.dateAdded, b.lastModified, b.parent, null, h.frecency, b.position, b.type, b.fk, b.guid FROM moz_bookmarks b LEFT JOIN moz_places h ON b.fk = h.id LEFT JOIN moz_favicons f ON h.favicon_id = f.id WHERE b.parent = :parent ORDER BY b.position ASC' failed to compile with the error message ''.: 'Error', file ../../../dist/include/mozilla/storage/StatementCache.h, line 125
<<<<<<<
}
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=fac650c31656

https://hg.mozilla.org/integration/mozilla-inbound/rev/469076f776d6

Comment 25

[Gregory Szorc [:gps]]

Looks like I missed a test that uses gTests. (I saw this in the try run but figured it was an intermittent crash.)

This Try push instruments the test runner to abort tests if gTests is being defined directly by a test.

https://tbpl.mozilla.org/?tree=Try&rev=1f5cabaa0a52

Comment 26

[Gregory Szorc [:gps]]

Try was green. Only change from last review was a trivial change to /browser/components/places/tests/unit/test_browserGlue_prefs.js to not use gTests. I consider this not worthy of time for review.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7592c22f19ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/fef8ecbe43f5

Comment 27

[Ed Morley [:emorley]]

(In reply to Gregory Szorc [:gps] from comment #25)
> Looks like I missed a test that uses gTests. (I saw this in the try run but
> figured it was an intermittent crash.)

I'm happy to take a look at cases like this in the future if that helps, just ping me on IRC. (Also there are not many cases where TBPL won't make bug suggestions now, so if there isn't a suggestion, it is a good sign it's something new)

Comment 28

[Marco Bonardo [:mak]]

yes, some tests were using gTests, but not by will of replacing the global one, just cause it's common to prefix g to globals and Tests was a convenient name... that is unfortunate indeed.

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7592c22f19ec
https://hg.mozilla.org/mozilla-central/rev/fef8ecbe43f5