Closed
Bug 1451333
Opened 7 years ago
Closed 7 years ago
Why aren't our generator (function*) tasks running in xpcshell?
Categories
(Toolkit :: Telemetry, enhancement, P1)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: chutten, Assigned: chutten)
Details
Attachments
(1 file)
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•7 years ago
|
||
Looks like some other components have xpcshell tests with generators: https://searchfox.org/mozilla-central/search?q=add_task(function*&case=false®exp=false&path=unit
Assignee | ||
Comment 2•7 years ago
|
||
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.
Assignee | ||
Comment 3•7 years ago
|
||
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•7 years ago
|
||
(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.
Assignee | ||
Comment 5•7 years ago
|
||
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*
Assignee | ||
Comment 6•7 years ago
|
||
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•7 years ago
|
||
(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•7 years ago
|
||
At a minimum we need to enable our tests again, the rest can be broken out.
Assignee: nobody → chutten
Priority: -- → P1
Assignee | ||
Comment 9•7 years ago
|
||
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.
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(florian)
Comment 10•7 years ago
|
||
They need to be migrated to use async functions. Task.jsm is going away. Backing out that patch doesn't get us anywhere.
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Comment 11•7 years ago
|
||
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®exp=false&path=unit
Comment 12•7 years ago
|
||
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•7 years ago
|
||
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®exp=false&path=unit makes me wonder if we should make add_task fail loudly when it's given a generator function.
Flags: needinfo?(florian)
Assignee | ||
Comment 14•7 years ago
|
||
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.
Assignee | ||
Comment 15•7 years ago
|
||
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 hidden (mozreview-request) |
Comment 17•7 years ago
|
||
If these generators had no 'yield' and these async functions have no 'await', can't this just be changed to normal functions?
Assignee | ||
Comment 18•7 years ago
|
||
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•7 years ago
|
||
mozreview-review |
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 :)
Attachment #8967125 -
Flags: review?(alessio.placitelli) → review+
Comment 20•7 years ago
|
||
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b1704c46dd79
Convert last few Telemetry tests from generators to async r=Dexter
Comment 21•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•