Remove Task.jsm/spawn_task support from mochitest harnesses

RESOLVED FIXED in Firefox 61

Status

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: kmag, Assigned: kmag)

Tracking

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(4 attachments)

No description provided.
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 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?
Attachment #8968718 - Flags: review?(florian) → review+
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 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 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 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.
(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.
(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.
(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.
(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 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.
Attachment #8969025 - Flags: review?(florian) → review+
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.
Attachment #8969026 - Flags: review?(florian) → review+
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.
Attachment #8968717 - Flags: review?(florian) → review+
(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.
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
Duplicate of this bug: 1448090
Duplicate of this bug: 1392823
You need to log in before you can comment on or make changes to this bug.