Closed Bug 1453881 Opened 6 years ago Closed 6 years ago

Fail tests when tasks return generators

Categories

(Testing :: XPCShell Harness, enhancement)

enhancement
Not set
normal

Tracking

(firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

Details

Attachments

(3 files)

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 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 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 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?
Attachment #8967643 - Flags: review?(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.
Attachment #8967644 - Flags: review?(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

> 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.
(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.
Attachment #8967644 - Flags: review?(poirot.alex)
Attachment #8967643 - Flags: review?(poirot.alex)
It looks like bug 1381834 did in fact recently remove the old webconsole directory, so you may want to rebase this.
(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 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! :-)
Attachment #8967645 - Flags: review?(florian) → review+
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...
Attachment #8967643 - Flags: review?(poirot.alex) → review+
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 *() {}`?
Attachment #8967644 - Flags: review?(poirot.alex) → review+
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.
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
Blocks: 1454813
You need to log in before you can comment on or make changes to this bug.