1219504 - Modernize some browser-chrome tests

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This bug covers some work I've been doing to eliminate CPOWs from some browser-chrome tests. I started working on this for bug 967873, but it's generally useful stuff since we always want to reduce our use of CPOWs. So I'm putting it in a separate bug.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 01 BrowserTestUtils 1

This patch extends BrowserTestUtils.browserLoaded so that you can decide whether a particular load is of interest to you.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 02 content-task 1

This simply allows you to use |this| inside a ContentTask. Basically it means you can add event listeners to the frame script global, which is really useful.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 03 content-task 2

This patch is a little gross but very useful. It allows you to use ok(), is(), and info() inside a ContentTask. It makes it a lot easier to convert existing tests to use ContentTask.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 04 Fix browser_bug441778.js

For the remaining patches I don't have much to say...

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 05 Fix browser_bug553455.js

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 06 Fix browser_bug580965.js

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 07 Fix browser_bug676619.js

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 08 Fix browser_bug902156.js

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 09 Fix browser_findbarClose.js

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 10 Fix browser_popupUI.js

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 11 Fix browser_utilityOverlay.js

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 12 Fix browser_bug1108547.js

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 13 devtools fixes

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 14 Fix browser_522545.js

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 15 Fix browser_URLBarSetURI.js

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 16 Fix browser_bug329212.js

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 17 Fix browser_bug331772

Comment 18

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 18 Fix browser_bug435035.js

Comment 19

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 19 Fix browser_bug561623.js

Comment 20

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 20 Fix browser_bug562649.js

Comment 21

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 21 Fix browser_bug578534.js

Comment 22

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 22 Fix browser_bug579872.js

Comment 23

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 22 Fix browser_bug579872.js

Comment 24

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 23 Fix browser_bug581947.js

Comment 25

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 24 Fix browser_bug594131.js

Comment 26

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 25 Fix browser_bug633691.js

Comment 27

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 26 Fix another devtools thing

Comment 28

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 27 Fix browser_bug431826.js

Comment 29

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 28 Fix browser_force_refresh.js

Comment 30

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 30 Fix xpinstall tests

Comment 31

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 31 Fix browser_bug655273.js

Comment 32

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 32 Fix browser_offline.js

Comment 33

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 33 Fix passwordmgr

Comment 34

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 34 Fix browser_bug441169.js

Comment 35

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 35 Fix browser_522545.js

This test just barely exceeds the 90 second limit with my changes. Looking at runs over the past couple days, it's just under 90 seconds.

Comment 36

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8680350 [details] [diff] [review]
01 BrowserTestUtils 1

Review of attachment 8680350 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with doc update.

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm
@@ +146,3 @@
>     *
>     * @return {Promise}
>     * @resolves When a load event is triggered for the browser.

You'll need to also update this to mention that it resolves to the url of the page that loaded.

Comment 37

[:Gijs (he/him)]

Comment on attachment 8680382 [details] [diff] [review]
28 Fix browser_force_refresh.js

Review of attachment 8680382 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/test/serviceworkers/browser_force_refresh.js
@@ +42,3 @@
>      var cachedLoad = false;
>  
> +    function eventHandler(msg) {

Nit: this is still called eventHandler, but it is really a messageHandler, so it should probably be called that.

You could avoid the s/event/msg.data/ changes in the function body by defining the function as:

function messageHandler({data: event})

because none of the other fields are used. Up to you whether you think that's worth it / clearer.

Comment 38

[:Gijs (he/him)]

Comment on attachment 8680366 [details] [diff] [review]
15 Fix browser_URLBarSetURI.js

Review of attachment 8680366 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/general/browser_URLBarSetURI.js
@@ -74,1 @@
>      info("Tab loaded");

This means this info() will now only happen for the load we're interested in, instead of for every load. I don't think that's bad, because it didn't log the "other" URL that got loaded either, so it probably wasn't super useful to begin with.

Comment 39

[:Gijs (he/him)]

Comment on attachment 8680367 [details] [diff] [review]
16 Fix browser_bug329212.js

Review of attachment 8680367 [details] [diff] [review]:
-----------------------------------------------------------------

s/Fox/Fix/ in the commit message, though maybe you'll squash/fold the commits anyway? :-)

Comment 40

[:Gijs (he/him)]

Comment on attachment 8680360 [details] [diff] [review]
10 Fix browser_popupUI.js

Review of attachment 8680360 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/general/browser_popupUI.js
@@ +2,5 @@
>    waitForExplicitFinish();
>    gPrefService.setBoolPref("dom.disable_open_during_load", false);
>  
>    var browser = gBrowser.selectedBrowser;
> +  BrowserTestUtils.browserLoaded(browser).then(() => {

Nit: might as well nix the browser = gBrowser.selectedBrowser assignment and use it directly, here and below.

Comment 41

[:Gijs (he/him)]

Comment on attachment 8680359 [details] [diff] [review]
09 Fix browser_findbarClose.js

Review of attachment 8680359 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/general/browser_findbarClose.js
@@ +9,1 @@
>    waitForExplicitFinish();

You don't need waitForExplicitFinish() inside add_task.

@@ +21,5 @@
> +  yield new ContentTask.spawn(newTab.linkedBrowser, null, function* () {
> +    let iframe = content.document.getElementById("iframe");
> +    let promise = ContentTaskUtils.waitForEvent(iframe, "load", false);
> +    iframe.src = "http://example.org/";
> +    yield promise;

You could just return the promise here, right?

@@ +29,5 @@
>       "subdocument changes");
>  
>    gFindBar.close();
>    gBrowser.removeTab(newTab);
>    finish();

Likewise, you don't need finish() here.

Comment 42

[:Gijs (he/him)]

Comment on attachment 8680350 [details] [diff] [review]
01 BrowserTestUtils 1

Review of attachment 8680350 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm
@@ +150,5 @@
> +  browserLoaded(browser, includeSubFrames=false, wantLoad=null) {
> +    function isWanted(url) {
> +      if (!wantLoad) {
> +        return true;
> +      } else if (typeof(wantLoad) == "function") {

Driveby nit because I had to look at this for the other reviews: please don't use else after a return.

Comment 43

[Matthew N. [:MattN]]

Comment on attachment 8680387 [details] [diff] [review]
33 Fix passwordmgr

Review of attachment 8680387 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this! I wish we would modernize tests more often.

mozreview makes it much easier to review code movement and indentation changes btw.

Comment 44

[Dave Townsend [:mossop]]

Comment on attachment 8680383 [details] [diff] [review]
30 Fix xpinstall tests

Review of attachment 8680383 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, thanks.

Comment 45

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8680353 [details] [diff] [review]
03 content-task 2

Review of attachment 8680353 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for adding these!

Comment 46

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8680357 [details] [diff] [review]
07 Fix browser_bug676619.js

Mike, I think Felipe is a TRIBE. Would you mind looking at this one?

Comment 47

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8680357 [details] [diff] [review]
07 Fix browser_bug676619.js

Review of attachment 8680357 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for getting this test off CPOWs. Might be nice to put a bullet in this thing and write a new add_task'y test someday, but today is not that day.

Comment 48

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/77f868daa629
https://hg.mozilla.org/integration/mozilla-inbound/rev/90b0dcd99711

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/77f868daa629
https://hg.mozilla.org/mozilla-central/rev/90b0dcd99711