1454813 - Remove Task.jsm/spawn_task support from mochitest harnesses

Status: RESOLVED FIXED

(Testing :: Mochitest, enhancement)

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1454813: Part 1a - Remove Tasks.jsm support in browser tests.

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

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1454813: Part 2a - Remove spawn_task support in plain/chrome mochitests.

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

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8968717 [details]
Bug 1454813: Part 1a - Remove Tasks.jsm support in browser tests.

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

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8968718 [details]
Bug 1454813: Part 2a - Remove spawn_task support in plain/chrome mochitests.

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

Comment 5

[Code Review Bot [:reviewbot]]

Comment on attachment 8968717 [details]
Bug 1454813: Part 1a - Remove Tasks.jsm support in browser tests.

https://reviewboard.mozilla.org/r/237442/#review243202


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


::: devtools/client/inspector/markup/test/browser_markup_toggle_03.js:12
(Diff revision 2)
>  // Test toggling (expand/collapse) elements by alt-clicking on twisties, which
>  // should expand/collapse all the descendants
>  
>  const TEST_URL = URL_ROOT + "doc_markup_toggle.html";
>  
> -add_task(function* () {
> +add_task(async function () {

Error: Unexpected space before function parentheses. [eslint: space-before-function-paren]

Comment 6

[Florian Quèze [:florian]]

Comment on attachment 8968718 [details]
Bug 1454813: Part 2a - Remove spawn_task support in plain/chrome mochitests.

https://reviewboard.mozilla.org/r/237444/#review243278

::: testing/mochitest/tests/SimpleTest/SpawnTask.js
(Diff revision 2)
> -// # SpawnTask.js

Should we rename this file now that it doesn't contain the spawn_task function anymore?

If the reason you didn't do it is because there are more than 300 references to it and that would make the patch unreadable, that's fine. Maybe just file a follow-up in that case if you agree it would be better to rename.

::: toolkit/components/places/tests/chrome/test_browser_disableglobalhistory.xul
(Diff revision 2)
>  
>    let w = window.open('browser_disableglobalhistory.xul', '_blank', 'chrome,resizable=yes,width=400,height=600');
>  
>    function done() {
>      w.close();
> -    SimpleTest.finish();

What is this change for? Or did you mean to also remove the waitForExplicitFinish call?

Comment 7

[Florian Quèze [:florian]]

Comment on attachment 8968717 [details]
Bug 1454813: Part 1a - Remove Tasks.jsm support in browser tests.

https://reviewboard.mozilla.org/r/237442/#review243288

Exciting! Thanks for working on this :-).

::: testing/mochitest/browser-test.js:8
(Diff revision 2)
>  var gTimeoutSeconds = 45;
>  var gConfig;
>  var gSaveInstrumentationData = null;
>  
>  ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
>  ChromeUtils.import("resource://gre/modules/Task.jsm");

Can we now remove this import? The only other use left seems to be
  this.Task.Debugging.maintainStack = true;
and I doubt it's still useful if we no longer start the test tasks using Task.jsm.

::: testing/mochitest/browser-test.js:1427
(Diff revision 2)
>    __expectedMinAsserts: 0,
>    __expectedMaxAsserts: 0,
>  
>    EventUtils: {},
>    SimpleTest: {},
>    Task: null,

Do we still need this line?

::: testing/mochitest/browser-test.js:1458
(Diff revision 2)
>  
>      return sandbox;
>    },
>  
>    /**
>     * Add a test function which is a Task function.

Could you please update this comment to remove the reference to Task.jsm and replace the 'yield' keywords?

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8968718 [details]
Bug 1454813: Part 2a - Remove spawn_task support in plain/chrome mochitests.

https://reviewboard.mozilla.org/r/237444/#review243278

> Should we rename this file now that it doesn't contain the spawn_task function anymore?
> 
> If the reason you didn't do it is because there are more than 300 references to it and that would make the patch unreadable, that's fine. Maybe just file a follow-up in that case if you agree it would be better to rename.

Yeah, it probably makes sense. I'll write a follow-up.

> What is this change for? Or did you mean to also remove the waitForExplicitFinish call?

When I switched the caller in browser_disableglobalhistory.xul to use add_task() rather than spawn_task(), it started calling SimpleTest.finish() implicitly, which made this fail because finish() was called twice.

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8968717 [details]
Bug 1454813: Part 1a - Remove Tasks.jsm support in browser tests.

https://reviewboard.mozilla.org/r/237442/#review243288

> Can we now remove this import? The only other use left seems to be
>   this.Task.Debugging.maintainStack = true;
> and I doubt it's still useful if we no longer start the test tasks using Task.jsm.

Hm. Yeah, I thought we were exporting the `Task` symbol to test scopes, but I didn't read closely enough.

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8968717 [details]
Bug 1454813: Part 1a - Remove Tasks.jsm support in browser tests.

https://reviewboard.mozilla.org/r/237442/#review243288

> Hm. Yeah, I thought we were exporting the `Task` symbol to test scopes, but I didn't read closely enough.

Oh, no, I'm wrong. We definitely do `scope.Task = this.Task;` I don't know how many tests will break if I remove that. I can try, but I'd rather it be a separate patch.

Comment 11

[Florian Quèze [:florian]]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #10)
> Comment on attachment 8968717 [details]
> Bug 1454813: Part 1 - Remove Tasks.jsm support in browser tests.
> 
> https://reviewboard.mozilla.org/r/237442/#review243288
> 
> > Hm. Yeah, I thought we were exporting the `Task` symbol to test scopes, but I didn't read closely enough.
> 
> Oh, no, I'm wrong. We definitely do `scope.Task = this.Task;` I don't know
> how many tests will break if I remove that. I can try, but I'd rather it be
> a separate patch.

Lots of devtools tests apparently, but they seem mostly in devtools/client/commandline/test and devtools/client/debugger/test/mochitest, so we could move the import to head.js files in these folders.
https://searchfox.org/mozilla-central/search?q=%5CbTask.spawn&case=false&regexp=true&path=browser_
I agree this can be in a separate patch or bug.

Comment 12

[Florian Quèze [:florian]]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #8)

> > What is this change for? Or did you mean to also remove the waitForExplicitFinish call?
> 
> When I switched the caller in browser_disableglobalhistory.xul to use
> add_task() rather than spawn_task(), it started calling SimpleTest.finish()
> implicitly, which made this fail because finish() was called twice.

Ok, I think you can also remove the waitForExplicitFinish call then.

Comment 13

[Kris Maglione [:kmag]]

Attachment: Bug 1454813: Part 1b - Stop automatically exposing Task singleton to browser tests.

Now that Task.jsm is deprecated and add_task no longer excepts generators, it
doesn't make sense to continue making this automatically available in test
scopes.

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

Comment 14

[Kris Maglione [:kmag]]

Attachment: Bug 1454813: Part 2b - Rename SpawnTask.js to AddTask.js.

The old name no longer makes sense, since it no longer exports an spawn_task
symbol, and add_task is what we really care about.

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

Comment 15

[Kris Maglione [:kmag]]

Comment on attachment 8968717 [details]
Bug 1454813: Part 1a - Remove Tasks.jsm support in browser tests.

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

Comment 16

[Kris Maglione [:kmag]]

Comment on attachment 8968718 [details]
Bug 1454813: Part 2a - Remove spawn_task support in plain/chrome mochitests.

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

Comment 17

[Kris Maglione [:kmag]]

(In reply to Florian Quèze [:florian] from comment #11)
> (In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're
> blocked) from comment #10)
> > Comment on attachment 8968717 [details]
> > Bug 1454813: Part 1 - Remove Tasks.jsm support in browser tests.
> > 
> > https://reviewboard.mozilla.org/r/237442/#review243288
> > 
> > > Hm. Yeah, I thought we were exporting the `Task` symbol to test scopes, but I didn't read closely enough.
> > 
> > Oh, no, I'm wrong. We definitely do `scope.Task = this.Task;` I don't know
> > how many tests will break if I remove that. I can try, but I'd rather it be
> > a separate patch.
> 
> Lots of devtools tests apparently, but they seem mostly in
> devtools/client/commandline/test and
> devtools/client/debugger/test/mochitest, so we could move the import to
> head.js files in these folders.
> https://searchfox.org/mozilla-central/search?q=%5CbTask.
> spawn&case=false&regexp=true&path=browser_
> I agree this can be in a separate patch or bug.

Those tests use the devtools Task implementation, and import it in the head.js file.

The browser-test eslint config doesn't define this export, so hopefully there aren't any tests that use it without importing it themselves. I did a try run with the definition removed. If there are any tests that rely on it, I'll fix them when that finishes.

Comment 18

[Kris Maglione [:kmag]]

(In reply to Florian Quèze [:florian] from comment #12)
> (In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're
> blocked) from comment #8)
> 
> > > What is this change for? Or did you mean to also remove the waitForExplicitFinish call?
> > 
> > When I switched the caller in browser_disableglobalhistory.xul to use
> > add_task() rather than spawn_task(), it started calling SimpleTest.finish()
> > implicitly, which made this fail because finish() was called twice.
> 
> Ok, I think you can also remove the waitForExplicitFinish call then.

Oh, yeah, good catch.

Comment 19

[Florian Quèze [:florian]]

Comment on attachment 8969025 [details]
Bug 1454813: Part 1b - Stop automatically exposing Task singleton to browser tests.

https://reviewboard.mozilla.org/r/237708/#review243486

\o/

::: commit-message-1506d:3
(Diff revision 1)
> +Bug 1454813: Part 1b - Stop automatically exposing Task singleton to browser tests. r?florian
> +
> +Now that Task.jsm is deprecated and add_task no longer excepts generators, it

Not sure if you meant "expects" or "accepts", but "excepts" looks wrong here.

Comment 20

[Florian Quèze [:florian]]

Comment on attachment 8969026 [details]
Bug 1454813: Part 2b - Rename SpawnTask.js to AddTask.js.

https://reviewboard.mozilla.org/r/237710/#review243488

MozReview makes this long patch painful to even just skim, so I reviewed the changes in testing/mochitest and assumed the rest is just a simple search & replace.

Comment 21

[Florian Quèze [:florian]]

Comment on attachment 8968717 [details]
Bug 1454813: Part 1a - Remove Tasks.jsm support in browser tests.

https://reviewboard.mozilla.org/r/237442/#review243506

::: testing/mochitest/browser-test.js
(Diff revision 3)
>    __expectedMinAsserts: 0,
>    __expectedMaxAsserts: 0,
>  
>    EventUtils: {},
>    SimpleTest: {},
> -  Task: null,

This may be better in part 1b now that you have that separate patch.

Comment 22

[Kris Maglione [:kmag]]

(In reply to Florian Quèze [:florian] from comment #20)
> Comment on attachment 8969026 [details]
> Bug 1454813: Part 2b - Rename SpawnTask.js to AddTask.js.
> 
> https://reviewboard.mozilla.org/r/237710/#review243488
> 
> MozReview makes this long patch painful to even just skim, so I reviewed the
> changes in testing/mochitest and assumed the rest is just a simple search &
> replace.

Yup, it is.

Comment 23

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dad590c53eb70211d3704c49c578c9b3bfbbdc6f
Bug 1454813: Part 1a - Remove Tasks.jsm support in browser tests. r=florian

https://hg.mozilla.org/integration/mozilla-inbound/rev/591a388f9f5968a57c26d2a4b4961509d2c43633
Bug 1454813: Part 1b - Stop automatically exposing Task singleton to browser tests. r=florian

https://hg.mozilla.org/integration/mozilla-inbound/rev/22acceec0478d3aaca5f38292696b522de67c4bf
Bug 1454813: Part 2a - Remove spawn_task support in plain/chrome mochitests. r=florian

https://hg.mozilla.org/integration/mozilla-inbound/rev/c10c705a1feae70c507bb644dcf2867cbe63db6f
Bug 1454813: Part 2b - Rename SpawnTask.js to AddTask.js. r=florian

Comment 24

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/dad590c53eb7
https://hg.mozilla.org/mozilla-central/rev/591a388f9f59
https://hg.mozilla.org/mozilla-central/rev/22acceec0478
https://hg.mozilla.org/mozilla-central/rev/c10c705a1fea