1078657 - Add a Task library for mochitest chrome and plain

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

>>Problem:
add_task was added for mochitest-browser in bug 872229, and it introduces a really easy way to write asynchronous, promise-y tests in a very readable manner.

Unfortunately, this sweet sweet nectar is only available for mochitest-browser tests, and none of the other mochitest flavours. :(

We should make it available.

>>Solution:
Port the change from bug 872229 to the other mochitest runners, I guess?

>>Mozilla Top Level Goal:
mochitest-chrome and mochitest-plain are usually used by people testing XUL widgetry or DOM stuff, so this would be to make life easier for them.

>>Existing Bug:
No bug

>>Per-Commit:
No

>>Data other than Pass/Fail:
No

>>Prototype Date:
Not provided

>>Production Date:
Not provided

>>Most Valuable Piece:
Not provided

>>Responsible Engineer:
Not provided

>>Manager:
Not provided

>>Other Teams/External Dependencies:
Not provided

>>Additional Info:
Not provided

Comment 1

[Jonathan Griffin (:jgriffin)]

Added to unprioritized backlog: https://trello.com/b/3BjXQCEp/projects

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

Attachment: Potential add_task.js implementation

Here's a local add_task implementation I wrote for bug 1087145 after getting tricked by the MDN docs and already having gone down the generator road.  This might be a useful starting point if we don't just grab an existing implementation.  It only ever wants to live in a mochitest context.

Note that this implementation was written assuming ES6 generators and the existence of yield*.  Since yield* can be used to transfer control to a sub-generator, there's no need for the driver mechanism to handle a generator being returned instead of a Promise.  I also wrote the implementation with the goal of not requiring a JS1.7 version to be specified (which is how we seem to currently get "let"), so it probably looks a bit old-school even though it assumes the existence of fancy things.

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

We are working around this in layout/style/test/animation_utils.js too. Being able to use add_task there would be great.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

Attachment: add_task implementation that went in dom/network/tests

This is the actual version landed in dom/network/tests.  The previous version made dangerous assumptions about script loading having completed.  This version adds some use of readystateChange in order to make sure the JS files have loaded/executed before potentially closing out the test.  For tests that involve the document not reaching the 'complete' state this implementation won't work as-is.  In those cases it might make sense for any users to just take over the responsibility for indicating all the calls to add_task that are going to happen have happened.

Comment 5

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-1078657-Add-SpawnTask.js-for-async-tasks-in-moch.patch

Here's a patch that introduces a new script, SpawnTask.js, for use in mochitests (plain and browser). SpawnTask.js exposes a single function, spawnTask(generator), which has pretty much identical behavior to Task.spawn(...) in Mozilla's Task.jsm.

Embed SpawnTask.js in a mochitest with the line
<script type="application/javascript;version=1.7" src="/tests/SimpleTest/SpawnTask.js"></script>

Unit tests for SpawnTask.js itself are also included in this patch.

Comment 6

[Arthur Edelstein [:arthur]]

(I should have said mochitests, "plain and chrome", not "plain and browser"...)

Comment 7

[Arthur Edelstein [:arthur]]

Try server results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5ce5c217a15

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 8626970 [details] [diff] [review]
0001-Bug-1078657-Add-SpawnTask.js-for-async-tasks-in-moch.patch

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

This is cool, thanks for the patch! I do wonder if we wouldn't be better served just importing a well-liked library like co:
https://github.com/tj/co

I asked dherman (who wrote task.js) about its status and he said it hadn't been worked on in a while and that co covered the same ground.

::: testing/mochitest/tests/Harness_sanity/test_spawn_task.html
@@ +2,5 @@
> +<html>
> +<head>
> +  <title>Test for mochitest SpawnTask.js sanity</title>
> +  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <script type="application/javascript;version=1.7" src="/tests/SimpleTest/SpawnTask.js"></script>

The version=1.7 bit isn't fantastic, is that just to get "let"?

::: testing/mochitest/tests/SimpleTest/SpawnTask.js
@@ +12,5 @@
> +let promiseFromGenerator;
> +
> +// Returns true if aValue is a generator object.
> +let isGenerator = aValue => {
> +  return Object.prototype.toString.call(aValue) === "[object Generator]";

I don't love this. Could you just duck type the object instead?

Comment 9

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-1078657-Add-SpawnTask.js-for-async-tasks-in-moch.patch

(In reply to (on vacation July 18- 25) Ted Mielczarek [:ted.mielczarek] from comment #8)
> Comment on attachment 8626970 [details] [diff] [review]
> 0001-Bug-1078657-Add-SpawnTask.js-for-async-tasks-in-moch.patch
> 
> Review of attachment 8626970 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is cool, thanks for the patch! I do wonder if we wouldn't be better
> served just importing a well-liked library like co:
> https://github.com/tj/co

Aha, I didn't know about this library. Thanks for pointing it out. Here's a patch that uses it instead. Does this seem like the right approach?

Try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f9b8f5cc89e

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8634918 [details] [diff] [review]
0001-Bug-1078657-Add-SpawnTask.js-for-async-tasks-in-moch.patch

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

nit: if this is intended for chrome tests, then we should have a test in chrome.ini as well.

Comment 11

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-1078657-Add-SpawnTask.js-for-async-tasks-in-moch.patch

Here's a new version with the chrome tests as requested.

Comment 12

[Arthur Edelstein [:arthur]]

Try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa9166849052

Comment 13

[Pulsebot]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Backed out for test_async_setTimeout_stack.html and test_async_setTimeout_stack_across_globals.html failures on at least Mulet.
https://treeherder.mozilla.org/logviewer.html#?job_id=11831549&repo=mozilla-inbound

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

Comment 15

[Arthur Edelstein [:arthur]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #14)
> Backed out for test_async_setTimeout_stack.html and
> test_async_setTimeout_stack_across_globals.html failures on at least Mulet.
> https://treeherder.mozilla.org/logviewer.html#?job_id=11831549&repo=mozilla-
> inbound
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/c5d8689db9b2

Looks like this error was not caused by my patch, but by
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa9fe193778f
as it created test_async_setTimeout_stack.html and test_async_setTimeout_stack_across_globals.html.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Gah! Thanks.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/498153aa50a7

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/498153aa50a7

Comment 19

[Matthew N. [:MattN]]

IIUC, I don't think what landed here was sufficient to address what the bug was asking for and what add_task does. It seems like we landed a Task library which has no integration with the mochitest chrome or plain harnesses. Notice how the example in attachment 8509260 [details] integrates with SimpleTest.finish and keeps track of pending tasks.

Arthur, am I missing something or is this something you can work on in another bug (we can re-summarize this one)?

Comment 20

[Arthur Edelstein [:arthur]]

(In reply to Matthew N. [:MattN] from comment #19)
> IIUC, I don't think what landed here was sufficient to address what the bug
> was asking for and what add_task does. It seems like we landed a Task
> library which has no integration with the mochitest chrome or plain
> harnesses. Notice how the example in attachment 8509260 [details] integrates
> with SimpleTest.finish and keeps track of pending tasks.
> 
> Arthur, am I missing something or is this something you can work on in
> another bug (we can re-summarize this one)?

Yes, I will have a look at this: Bug 1187701.