Closed Bug 1078657 Opened 5 years ago Closed 4 years ago

Add a Task library for mochitest chrome and plain

Categories

(Testing :: General, defect)

All
Other
defect
Not set

Tracking

(firefox42 fixed)

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: mconley, Assigned: arthur)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [tor])

Attachments

(1 file, 4 obsolete files)

>>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
Attached file Potential add_task.js implementation (obsolete) —
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.
Attachment #8509260 - Attachment is patch: true
Attachment #8509260 - Attachment mime type: application/javascript → text/plain
We are working around this in layout/style/test/animation_utils.js too. Being able to use add_task there would be great.
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.
Attachment #8509260 - Attachment is obsolete: true
Assignee: jgriffin → nobody
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.
Attachment #8626970 - Flags: review?(ted)
(I should have said mochitests, "plain and chrome", not "plain and browser"...)
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?
Attachment #8626970 - Flags: review?(ted)
(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
Attachment #8626970 - Attachment is obsolete: true
Attachment #8634918 - Flags: review?
Attachment #8634918 - Flags: review? → review?(jmaher)
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.
Attachment #8634918 - Flags: review?(jmaher) → review+
Here's a new version with the chrome tests as requested.
Attachment #8530417 - Attachment is obsolete: true
Attachment #8634918 - Attachment is obsolete: true
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
Assignee: nobody → arthuredelstein
(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.
Flags: needinfo?(ryanvm)
Gah! Thanks.
Flags: needinfo?(ryanvm)
https://hg.mozilla.org/mozilla-central/rev/498153aa50a7
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
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)?
Flags: needinfo?(arthuredelstein)
Blocks: 1186277
(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.
Status: RESOLVED → REOPENED
Flags: needinfo?(arthuredelstein)
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Summary: Implement add_task() for mochitest chrome and plain → Add a Task library for mochitest chrome and plain
Whiteboard: [tor]
Blocks: meta_tor
You need to log in before you can comment on or make changes to this bug.