1453881 - Fail tests when tasks return generators

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, enhancement)

Description

[Kris Maglione [:kmag]]

Previous versions of add_task() allowed tasks to return generator functions, which were automatically wrapped in Task.jsm tasks. That is no longer supported, but people who are used to the old style still occasionally write new tests using generator functions rather than async functions, which don't run, but pass silently.

To help people avoid that footgun, we should fail any tasks that return genreators.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1453881: Part 1a - Automatically write add_task(function*) to add_task(async function)

This was done using the script at:

https://bitbucket.org/kmaglione/m-c-rewrites/src/bc5629735d8c7e4150f376d4a906e7c7c23132ba/processors/add-task-async.jsm?at=default&fileviewer=file-view-default

Review commit: https://reviewboard.mozilla.org/r/236376/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/236376/

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1453881: Part 1b - Manually fix tests that await generators in async functions.

Review commit: https://reviewboard.mozilla.org/r/236378/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/236378/

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1453881: Part 2 - Assert that task functions do not return generators.

In older builds, task functions were allowed to return generators, which were
automatically wrapped into tasks using Task.jsm. This model is no longer
supported, so to prevent it from passing silently, we should check for and
reject uses of the old pattern.

Review commit: https://reviewboard.mozilla.org/r/236380/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/236380/

Comment 4

[Code Review Bot [:reviewbot]]

Comment on attachment 8967643 [details]
Bug 1453881: Part 1a - Automatically write add_task(function*) to add_task(async function)

https://reviewboard.mozilla.org/r/236376/#review242116


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: mobile/android/components/extensions/test/mochitest/test_ext_tabs_update_url.html:117
(Diff revision 1)
>  
>    let testCases = checkList
>          .map((check) => Object.assign({}, check, {existentTabURL: "about:blank"}));
>  
>    for (let {existentTabURL, tabsUpdateURL, isErrorExpected} of testCases) {
> -    yield* testTabsUpdateURL(existentTabURL, tabsUpdateURL, isErrorExpected);
> +    await* testTabsUpdateURL(existentTabURL, tabsUpdateURL, isErrorExpected);

Error: Parsing error: unexpected token * [eslint]

Comment 5

[Florian Quèze [:florian]]

Comment on attachment 8967645 [details]
Bug 1453881: Part 2 - Assert that task functions do not return generators.

https://reviewboard.mozilla.org/r/236380/#review242208

::: testing/xpcshell/head.js:204
(Diff revision 1)
>      do_timeout(newDelay, this._func);
>    }
>  };
>  
> +function _isGenerator(val) {
> +  return typeof val === "object" && val && typeof val.next === "function";

Could we check the function before calling it with the usual https://searchfox.org/mozilla-central/rev/9f3da81290054c5b8955bb67ff98cae66676f745/toolkit/modules/Task.jsm#130 instaed of checking the result?

Comment 6

[Florian Quèze [:florian]]

Comment on attachment 8967643 [details]
Bug 1453881: Part 1a - Automatically write add_task(function*) to add_task(async function)

https://reviewboard.mozilla.org/r/236376/#review242214

The devtools changes would be better reviewed by a devtools peer, I would suggest :ochameau as he's working on bug 1365607. He said in bug 1365607 comment 6 that he ignored the old webconsole tests because they are planned for removal.

The change we made to xpcshell doesn't affect mochitests and I think it doesn't affect robocop tests either. I'm not sure it's safe to do a mass replace in these tests for just the add_task lines. Or have you reviewed all the other generators left in these test folders to ensure they are real generators and not tasks?

Comment 7

[Florian Quèze [:florian]]

Comment on attachment 8967644 [details]
Bug 1453881: Part 1b - Manually fix tests that await generators in async functions.

https://reviewboard.mozilla.org/r/236378/#review242216

This is mostly devtools too.

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8967645 [details]
Bug 1453881: Part 2 - Assert that task functions do not return generators.

https://reviewboard.mozilla.org/r/236380/#review242208

> Could we check the function before calling it with the usual https://searchfox.org/mozilla-central/rev/9f3da81290054c5b8955bb67ff98cae66676f745/toolkit/modules/Task.jsm#130 instaed of checking the result?

I'm not sure I'd call that the "usual" way. Generators are defined by the spec using duck typing. Any object with a "next" method can be used as a generator, not just generator functions. We could check them that way, I guess, but I'd really rather we not.

Comment 9

[Kris Maglione [:kmag]]

(In reply to Florian Quèze [:florian] from comment #6)
> The devtools changes would be better reviewed by a devtools peer, I would
> suggest :ochameau as he's working on bug 1365607. He said in bug 1365607
> comment 6 that he ignored the old webconsole tests because they are planned
> for removal.

Believe me, I'd have loved to ignore the old webconsole tests, but they started failing after part 2.

> The change we made to xpcshell doesn't affect mochitests and I think it
> doesn't affect robocop tests either. I'm not sure it's safe to do a mass
> replace in these tests for just the add_task lines. Or have you reviewed all
> the other generators left in these test folders to ensure they are real
> generators and not tasks?

Limiting the replacement to xpcshell tests is easier said than done, and since we ideally want to stop using Task.jsm in other test suites too, it didn't seem worth the effort.

I did do a quick scan for generator functions that were being used as promises, but I didn't do a thorough review. The tests that I did find that used generators all failed loudly before I converted them, so I'm not too worried about others being missed.

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

It looks like bug 1381834 did in fact recently remove the old webconsole directory, so you may want to rebase this.

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8967643 [details]
Bug 1453881: Part 1a - Automatically write add_task(function*) to add_task(async function)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/236376/diff/1-2/

Comment 12

[Kris Maglione [:kmag]]

Comment on attachment 8967644 [details]
Bug 1453881: Part 1b - Manually fix tests that await generators in async functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/236378/diff/1-2/

Comment 13

[Kris Maglione [:kmag]]

Comment on attachment 8967645 [details]
Bug 1453881: Part 2 - Assert that task functions do not return generators.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/236380/diff/1-2/

Comment 14

[Florian Quèze [:florian]]

(In reply to Kris Maglione [:kmag] from comment #9)

> Limiting the replacement to xpcshell tests is easier said than done, and
> since we ideally want to stop using Task.jsm in other test suites too, it
> didn't seem worth the effort.

I'm actually looking forward to us removing Task.jsm support from mochitests, and then removing Task.jsm completely from the tree. I was waiting on the devtools conversion to finish before considering that.

> I did do a quick scan for generator functions that were being used as
> promises, but I didn't do a thorough review. The tests that I did find that
> used generators all failed loudly before I converted them, so I'm not too
> worried about others being missed.

Fair enough. For test-only code I think it's fine. If it was actual shipping code I would want us to do a more thorough review.

Comment 15

[Florian Quèze [:florian]]

Comment on attachment 8967645 [details]
Bug 1453881: Part 2 - Assert that task functions do not return generators.

https://reviewboard.mozilla.org/r/236380/#review242544

Thanks for doing this! :-)

Comment 16

[Alexandre Poirot [:ochameau]]

Comment on attachment 8967643 [details]
Bug 1453881: Part 1a - Automatically write add_task(function*) to add_task(async function)

https://reviewboard.mozilla.org/r/236376/#review242666

Thanks a lot. I ignored these folders as there is intends to removes them, but I didn't realized it would block such work...

Comment 17

[Alexandre Poirot [:ochameau]]

Comment on attachment 8967644 [details]
Bug 1453881: Part 1b - Manually fix tests that await generators in async functions.

https://reviewboard.mozilla.org/r/236378/#review242676

::: devtools/client/netmonitor/test/browser_net_resend.js:42
(Diff revision 2)
>    store.dispatch(Actions.cloneSelectedRequest());
> +  // FIXME: This used to be a generator function that nothing awaited on
> +  // and therefore didn't run. It has been broken for some time.
> +  if (false) {
> -  testCustomForm(origItem);
> +    testCustomForm(origItem);
> +  }

This is being worked on in bug 1441792.
You may have to rebase depending on which bug lands first.

::: testing/mochitest/chrome/test_sanityAddTask.xul:29
(Diff revision 2)
> -        var x = await [Promise.resolve(1), Promise.resolve(2), Promise.resolve(3)];
> +        var x = yield [Promise.resolve(1), Promise.resolve(2), Promise.resolve(3)];
>          is(x.join(""), "123", "task yields Promise value as expected");
>        });
>  
> -      add_task(async function() {
> -        var x = await (function* () {
> +      add_task(function* () {
> +        var x = yield (function* () {

I don't quite follow why you undo the convertion of mochitest folder?

You seem to fix xpcshell _and_ mochitests test scripts, while only throwing from xpcshell harness. Do you plan to also throw from mochitest one?
In any case, isn't it better to remove all occurences of `add_task(function *() {}`?

Comment 18

[Kris Maglione [:kmag]]

Comment on attachment 8967644 [details]
Bug 1453881: Part 1b - Manually fix tests that await generators in async functions.

https://reviewboard.mozilla.org/r/236378/#review242676

> I don't quite follow why you undo the convertion of mochitest folder?
> 
> You seem to fix xpcshell _and_ mochitests test scripts, while only throwing from xpcshell harness. Do you plan to also throw from mochitest one?
> In any case, isn't it better to remove all occurences of `add_task(function *() {}`?

These are tests for the functionality of generator-based tasks that don't apply to async function versions. I'm planning to remove them when we remove support for Task.jsm in mochitests, but that's a separate bug, so I figured it made more sense to leave it as-is for now.

Comment 19

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/56c287941f2a60a7ea18baf45ffc526c4cd137e2
Bug 1453881: Part 1a - Automatically write add_task(function*) to add_task(async function) r=ochameau

https://hg.mozilla.org/integration/mozilla-inbound/rev/b05440a8fe3fbb8406354eaadfb213760c2723b3
Bug 1453881: Part 1b - Manually fix tests that await generators in async functions. r=ochameau

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f31e4da13c67dc171105e9909b6d81203e9f14a
Bug 1453881: Part 2 - Assert that task functions do not return generators. r=florian

Comment 20

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a98ad37316f62fa1a5231c59ae71677db24f689a
Bug 1453881: Fix xpcshell self-test failure. r=bustage CLOSED TREE

Comment 21

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9beb8f20e0a1708093034e8740fa9506ec1e2d7
Bug 1453881: Fix Android tests that still used task generators. r=bustage

Comment 22

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/56c287941f2a
https://hg.mozilla.org/mozilla-central/rev/b05440a8fe3f
https://hg.mozilla.org/mozilla-central/rev/7f31e4da13c6
https://hg.mozilla.org/mozilla-central/rev/a98ad37316f6
https://hg.mozilla.org/mozilla-central/rev/e9beb8f20e0a