1451333 - Why aren't our generator (function*) tasks running in xpcshell?

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Description

[Chris H-C :chutten]

In bug 1450690 I noticed that entire segments of test_TelemetryEvent.js weren't being run with `./mach test toolkit/components/telemetry/tests/unit/test_TelemetryEvent.js`

When I switched it from generator notation (function*) to async (async function) it started working again.

Why? According to :Mossop on #developers, this has been the case for about five years (since Bug 819033), but Telemetry Events (and its tests) are only two years old.

Something strange is happening here and we should look into it.

Comment 1

[Alessio Placitelli [:Dexter]]

Looks like some other components have xpcshell tests with generators: https://searchfox.org/mozilla-central/search?q=add_task(function*&case=false&regexp=false&path=unit

Comment 2

[Chris H-C :chutten]

Related, an automated script in bug 1374282 converted -most- of our tests from `function*` to `async function` ... but missed quite a few. Not sure what this might mean.

Oh, and there's probably a typo in my ./mach test line above. Sorry about that.

Comment 3

[Chris H-C :chutten]

devtools is notable for having them, but with extensive use of "yield" ... maybe that's our problem? We don't have any `yield` statements in our generators.

Comment 4

[Alessio Placitelli [:Dexter]]

(In reply to Chris H-C :chutten from comment #3)
> devtools is notable for having them, but with extensive use of "yield" ...
> maybe that's our problem? We don't have any `yield` statements in our
> generators.

Mh, weird. Adding

> yield new Promise(r => r());
> dump("\n\n**** WOHOOOOOOOOOOOO****\n\n\n");

Doesn't seem to make the test run.

Comment 5

[Chris H-C :chutten]

Confirmed that devtools' xpcshell tests run just fine with generators. ./mach test devtools/client/memory/test/unit/test_action-toggle-inverted.js prints out my dump strings and test outputs.

It's using things like `dispatch` and `equal`, though, so maybe there's special sauce in there that we don't get with Assert.equal? *wild speculation*

Comment 6

[Chris H-C :chutten]

test_dynamicEvents runs on the build on my Windows laptop. The build's about a month old (latest commit's from 2018-03-10).

I wonder if I've found a regression range or if this is a platform-specific weirdness.

Comment 7

[Alessio Placitelli [:Dexter]]

(In reply to Chris H-C :chutten from comment #6)
> test_dynamicEvents runs on the build on my Windows laptop. The build's about
> a month old (latest commit's from 2018-03-10).
> 
> I wonder if I've found a regression range or if this is a platform-specific
> weirdness.

I just tested latest m-c on my UbuntuVM: the test doesn't get run there as well :-\

Comment 8

[Georg Fritzsche [:gfritzsche]]

At a minimum we need to enable our tests again, the rest can be broken out.

Comment 9

[Chris H-C :chutten]

This is a regression in xpcshell-test caused by bug 1446833 part 2 affecting all xpcshell tests that are generators (function*). Reverting that patch makes generator xpcshell tests work again and, luckily, the following tests still all pass:

./mach test devtools/client/memory/test/unit/
./mach test toolkit/components/telemetry/tests/unit/
./mach test devtools/client/shared/test/unit/
./mach test browser/modules/test/unit/test_SitePermissions.js
./mach test toolkit/components/normandy/test/unit/

ni?kmag ni?florian for how they'd like to proceed.

Comment 10

[Kris Maglione [:kmag]]

They need to be migrated to use async functions. Task.jsm is going away. Backing out that patch doesn't get us anywhere.

Comment 11

[Chris H-C :chutten]

Was migrating all xpcshell tests to async functions a project or a program we could hang some action items off of? For instance, the public documentation[1] will need to be updated, and these tests[2] (and possibly others) migrated. I'm not deeply steeped in the mysteries of our test infrastructure, so I'm not sure what else might need to be done.

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests
[2]: https://searchfox.org/mozilla-central/search?q=add_task(function*&case=false&regexp=false&path=unit

Comment 12

[Kris Maglione [:kmag]]

Florian did a scripted migration months ago. I'm not sure why so many slipped through. A can add an assertion that task functions in xpcshell don't return a generator, I guess.

As for the docs on MDN, I could update them, but I thought they were deprecated at this point.

Comment 13

[Florian Quèze [:florian]]

This was all cleaned up using scripts in bug 1374282, including test_TelemetryEvent.js (https://hg.mozilla.org/mozilla-central/rev/05d686d3c53d#l56.1).

devtools/ was excluded from the scripted rewrite because it uses it's own fork of Task.jsm and I left that cleanup project for devtools engineers to handle (and it's currently ongoing in bug 1365607).

Some occurrences were re-introduced in test_TelemetryEvent.js by https://hg.mozilla.org/mozilla-central/rev/dc324afba772 and https://hg.mozilla.org/mozilla-central/rev/618bbe87c822

Seeing https://searchfox.org/mozilla-central/search?q=add_task(function*&case=false&regexp=false&path=unit makes me wonder if we should make add_task fail loudly when it's given a generator function.

Comment 14

[Chris H-C :chutten]

devtools may be interested to know that their tests aren't being run as of bug 1446833. My comment #5 was a little hasty, as the number of subtests that it was passing was 1, instead of the anticipated 5.

Comment 15

[Chris H-C :chutten]

Ugh, that wasn't helpefully worded. Let's try again.

I will comment on bug 1365607 to let devtools know that their unconverted tests are no longer running.

I will task this bug with converting the telemetry tests to async so that they run.

Comment 16

[Chris H-C :chutten]

Attachment: bug 1451333 - Convert last few Telemetry tests from generators to async

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

Comment 17

[Florian Quèze [:florian]]

If these generators had no 'yield' and these async functions have no 'await', can't this just be changed to normal functions?

Comment 18

[Chris H-C :chutten]

The cargo cult is strong when it comes to xpcshell. In this case there may be practical considerations as well to minimize the noisiness of test changes as we change the behaviour of the operations we're testing.

Comment 19

[Alessio Placitelli [:Dexter]]

Comment on attachment 8967125 [details]
bug 1451333 - Convert last few Telemetry tests from generators to async

https://reviewboard.mozilla.org/r/235768/#review241704

Let's ship this! Also, I agree on the reasoning from comment 18 :)

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b1704c46dd79
Convert last few Telemetry tests from generators to async r=Dexter

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b1704c46dd79