Closed
Bug 1219504
Opened 9 years ago
Closed 9 years ago
Modernize some browser-chrome tests
Categories
(Firefox :: General, defect)
Tracking
()
RESOLVED
FIXED
Firefox 45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: billm, Assigned: billm)
References
Details
Attachments
(34 files, 1 obsolete file)
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.
Assignee | ||
Comment 1•9 years ago
|
||
This patch extends BrowserTestUtils.browserLoaded so that you can decide whether a particular load is of interest to you.
Attachment #8680350 -
Flags: review?(mconley)
Assignee | ||
Comment 2•9 years ago
|
||
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)
Assignee | ||
Comment 3•9 years ago
|
||
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)
Assignee | ||
Comment 4•9 years ago
|
||
For the remaining patches I don't have much to say...
Attachment #8680354 -
Flags: review?(mconley)
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8680355 -
Flags: review?(felipc)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8680356 -
Flags: review?(felipc)
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8680357 -
Flags: review?(felipc)
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8680358 -
Flags: review?(felipc)
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8680359 -
Flags: review?(mrbkap)
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8680360 -
Flags: review?(mrbkap)
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8680361 -
Flags: review?(mrbkap)
Assignee | ||
Comment 12•9 years ago
|
||
Attachment #8680362 -
Flags: review?(mrbkap)
Assignee | ||
Comment 13•9 years ago
|
||
Attachment #8680363 -
Flags: review?(jryans)
Assignee | ||
Comment 14•9 years ago
|
||
Attachment #8680365 -
Flags: review?(jmathies)
Assignee | ||
Comment 15•9 years ago
|
||
Attachment #8680366 -
Flags: review?(jmathies)
Assignee | ||
Comment 16•9 years ago
|
||
Attachment #8680367 -
Flags: review?(jmathies)
Assignee | ||
Comment 17•9 years ago
|
||
Attachment #8680368 -
Flags: review?(jmathies)
Assignee | ||
Comment 18•9 years ago
|
||
Attachment #8680369 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 19•9 years ago
|
||
Attachment #8680370 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 20•9 years ago
|
||
Attachment #8680371 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 21•9 years ago
|
||
Attachment #8680372 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 22•9 years ago
|
||
Attachment #8680374 -
Flags: review?(MattN+bmo)
Assignee | ||
Comment 23•9 years ago
|
||
Attachment #8680375 -
Flags: review?(MattN+bmo)
Assignee | ||
Updated•9 years ago
|
Attachment #8680375 -
Attachment is obsolete: true
Attachment #8680375 -
Flags: review?(MattN+bmo)
Assignee | ||
Comment 24•9 years ago
|
||
Attachment #8680376 -
Flags: review?(MattN+bmo)
Assignee | ||
Comment 25•9 years ago
|
||
Attachment #8680377 -
Flags: review?(MattN+bmo)
Assignee | ||
Comment 26•9 years ago
|
||
Attachment #8680378 -
Flags: review?(MattN+bmo)
Assignee | ||
Comment 27•9 years ago
|
||
Attachment #8680380 -
Flags: review?(mconley)
Assignee | ||
Comment 28•9 years ago
|
||
Attachment #8680381 -
Flags: review?(mconley)
Assignee | ||
Comment 29•9 years ago
|
||
Attachment #8680382 -
Flags: review?(mconley)
Assignee | ||
Comment 30•9 years ago
|
||
Attachment #8680383 -
Flags: review?(dtownsend)
Assignee | ||
Comment 31•9 years ago
|
||
Attachment #8680385 -
Flags: review?(mrbkap)
Assignee | ||
Comment 32•9 years ago
|
||
Attachment #8680386 -
Flags: review?(dtownsend)
Assignee | ||
Comment 33•9 years ago
|
||
Attachment #8680387 -
Flags: review?(MattN+bmo)
Assignee | ||
Comment 34•9 years ago
|
||
Attachment #8680389 -
Flags: review?(mrbkap)
Assignee | ||
Comment 35•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8680374 -
Flags: review?(MattN+bmo) → review+
Updated•9 years ago
|
Attachment #8680376 -
Flags: review?(MattN+bmo) → review+
Updated•9 years ago
|
Attachment #8680377 -
Flags: review?(MattN+bmo) → review+
Updated•9 years ago
|
Attachment #8680378 -
Flags: review?(MattN+bmo) → review+
Comment 36•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8680369 -
Flags: review?(gijskruitbosch+bugs) → review+
Updated•9 years ago
|
Attachment #8680370 -
Flags: review?(gijskruitbosch+bugs) → review+
Updated•9 years ago
|
Attachment #8680371 -
Flags: review?(gijskruitbosch+bugs) → review+
Updated•9 years ago
|
Attachment #8680372 -
Flags: review?(gijskruitbosch+bugs) → review+
Updated•9 years ago
|
Attachment #8680380 -
Flags: review?(mconley) → review+
Updated•9 years ago
|
Attachment #8680381 -
Flags: review?(mconley) → review+
Comment 37•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8680365 -
Flags: review?(jmathies) → review+
Comment 38•9 years ago
|
||
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 39•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8680368 -
Flags: review?(jmathies) → review+
Updated•9 years ago
|
Attachment #8680385 -
Flags: review?(mrbkap) → review+
Updated•9 years ago
|
Attachment #8680386 -
Flags: review?(dtownsend) → review+
Updated•9 years ago
|
Attachment #8680389 -
Flags: review?(mrbkap) → review+
Updated•9 years ago
|
Attachment #8680391 -
Flags: review?(mconley) → review+
Updated•9 years ago
|
Attachment #8680362 -
Flags: review?(mrbkap) → review+
Updated•9 years ago
|
Attachment #8680361 -
Flags: review?(mrbkap) → review+
Comment 40•9 years ago
|
||
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 41•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8680358 -
Flags: review?(felipc) → review+
Updated•9 years ago
|
Attachment #8680356 -
Flags: review?(felipc) → review+
Comment 42•9 years ago
|
||
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.
Updated•9 years ago
|
Attachment #8680355 -
Flags: review?(felipc) → review+
Comment 43•9 years ago
|
||
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 44•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8680352 -
Flags: review?(mconley) → review+
Comment 45•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8680354 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 46•9 years ago
|
||
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 47•9 years ago
|
||
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+
Comment 48•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/77f868daa629 https://hg.mozilla.org/integration/mozilla-inbound/rev/90b0dcd99711
Comment 49•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/77f868daa629 https://hg.mozilla.org/mozilla-central/rev/90b0dcd99711
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
You need to log in
before you can comment on or make changes to this bug.
Description
•