Closed Bug 1219504 Opened 4 years ago Closed 4 years ago

Modernize some browser-chrome tests

Categories

(Firefox :: General, defect)

34 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
firefox45 --- fixed

People

(Reporter: billm, Assigned: billm)

References

Details

Attachments

(34 files, 1 obsolete file)

2.26 KB, patch
mconley
: review+
Details | Diff | Splinter Review
999 bytes, patch
mconley
: review+
Details | Diff | Splinter Review
7.29 KB, patch
mconley
: review+
Details | Diff | Splinter Review
1.65 KB, patch
mconley
: review+
Details | Diff | Splinter Review
3.25 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
1.18 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
5.22 KB, patch
mconley
: review+
Details | Diff | Splinter Review
6.35 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
2.40 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
1.43 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
1.71 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
4.86 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
5.14 KB, patch
jryans
: review+
Details | Diff | Splinter Review
1.33 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
1.11 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
1.79 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
1.52 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
1.22 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
1.55 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
1.29 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
1.58 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
1.56 KB, patch
MattN
: review+
Details | Diff | Splinter Review
1.62 KB, patch
MattN
: review+
Details | Diff | Splinter Review
1.30 KB, patch
MattN
: review+
Details | Diff | Splinter Review
1.21 KB, patch
MattN
: review+
Details | Diff | Splinter Review
2.81 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
3.72 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
3.32 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
7.42 KB, patch
mossop
: review+
Details | Diff | Splinter Review
1.28 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
1.54 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
13.24 KB, patch
MattN
: review+
Details | Diff | Splinter Review
2.84 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
1.04 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
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.
This patch extends BrowserTestUtils.browserLoaded so that you can decide whether a particular load is of interest to you.
Attachment #8680350 - Flags: review?(mconley)
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.
Attachment #8680352 - Flags: review?(mconley)
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.
Attachment #8680353 - Flags: review?(mconley)
For the remaining patches I don't have much to say...
Attachment #8680354 - Flags: review?(mconley)
Attachment #8680375 - Attachment is obsolete: true
Attachment #8680375 - Flags: review?(MattN+bmo)
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.
Attachment #8680391 - Flags: review?(mconley)
Attachment #8680374 - Flags: review?(MattN+bmo) → review+
Attachment #8680376 - Flags: review?(MattN+bmo) → review+
Attachment #8680377 - Flags: review?(MattN+bmo) → review+
Attachment #8680378 - Flags: review?(MattN+bmo) → review+
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.
Attachment #8680350 - Flags: review?(mconley) → review+
Attachment #8680363 - Flags: review?(jryans) → review+
Attachment #8680369 - Flags: review?(gijskruitbosch+bugs) → review+
Attachment #8680370 - Flags: review?(gijskruitbosch+bugs) → review+
Attachment #8680371 - Flags: review?(gijskruitbosch+bugs) → review+
Attachment #8680372 - Flags: review?(gijskruitbosch+bugs) → review+
Attachment #8680380 - Flags: review?(mconley) → review+
Attachment #8680381 - Flags: review?(mconley) → review+
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.
Attachment #8680382 - Flags: review?(mconley) → review+
Attachment #8680365 - Flags: review?(jmathies) → review+
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.
Attachment #8680366 - Flags: review?(jmathies) → review+
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? :-)
Attachment #8680367 - Flags: review?(jmathies) → review+
Attachment #8680368 - Flags: review?(jmathies) → review+
Attachment #8680385 - Flags: review?(mrbkap) → review+
Attachment #8680386 - Flags: review?(dtownsend) → review+
Attachment #8680389 - Flags: review?(mrbkap) → review+
Attachment #8680391 - Flags: review?(mconley) → review+
Attachment #8680362 - Flags: review?(mrbkap) → review+
Attachment #8680361 - Flags: review?(mrbkap) → review+
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.
Attachment #8680360 - Flags: review?(mrbkap) → review+
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.
Attachment #8680359 - Flags: review?(mrbkap) → review+
Attachment #8680358 - Flags: review?(felipc) → review+
Attachment #8680356 - Flags: review?(felipc) → review+
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.
Attachment #8680355 - Flags: review?(felipc) → review+
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.
Attachment #8680387 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8680383 [details] [diff] [review]
30 Fix xpinstall tests

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

Nice, thanks.
Attachment #8680383 - Flags: review?(dtownsend) → review+
Attachment #8680352 - Flags: review?(mconley) → review+
Comment on attachment 8680353 [details] [diff] [review]
03 content-task 2

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

Thanks for adding these!
Attachment #8680353 - Flags: review?(mconley) → review+
Attachment #8680354 - Flags: review?(mconley) → review+
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?
Attachment #8680357 - Flags: review?(felipc) → review?(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.
Attachment #8680357 - Flags: review?(mconley) → review+
https://hg.mozilla.org/mozilla-central/rev/77f868daa629
https://hg.mozilla.org/mozilla-central/rev/90b0dcd99711
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
You need to log in before you can comment on or make changes to this bug.