Closed
Bug 1259431
Opened 8 years ago
Closed 8 years ago
Fix test_feed_discovery.html to work on e10s
Categories
(Firefox Graveyard :: RSS Discovery and Preview, defect)
Firefox Graveyard
RSS Discovery and Preview
Tracking
(e10s+, firefox48 fixed)
RESOLVED
FIXED
Firefox 48
People
(Reporter: gsvelto, Assigned: gsvelto)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
8.06 KB,
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
As per title.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•8 years ago
|
||
This patch fixes the test_feed_discovery.html mochitest when e10s is turned on by changing it into a mochitest-browser test and greatly simplifying its structure. Previously the test would spin on a timer waiting for a list of feeds, now it runs synchronously once the page has been loaded. I'm not sure who to ask for review; Jared you touched this test some time ago, could you do the review? If not would you know who to redirect it to? The try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=15f617a19aa1
Attachment #8734365 -
Flags: review?(jaws)
Comment 2•8 years ago
|
||
Needinfo to felipe as he is tracking e10s test fixes and this one is moving from mochitest-plain to mochitest-browser
Flags: needinfo?(felipc)
Updated•8 years ago
|
Comment 3•8 years ago
|
||
Comment on attachment 8734365 [details] [diff] [review] [PATCH] Fix test_feed_discovery.html to work with e10s enabled Review of attachment 8734365 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the request, I am comfortable handling the review for it. This is close, bug r- because of the CPOW. ::: browser/base/content/test/general/browser_feed_discovery.js @@ +1,4 @@ > +/** > + * Any copyright is dedicated to the Public Domain. > + * http://creativecommons.org/publicdomain/zero/1.0/ > + */ We don't need the license header on tests. @@ +7,5 @@ > + > +/** Test for Bug 377611 **/ > + > +function test() { > + waitForExplicitFinish(); Please change this test from a test()/waitForExplicitFinish() style to an add_task(function*(){}) style. This will allow you to remove the waitForExplicitFinish()/finish(), as well as use yield to yield for BrowserTestUtils.browserLoaded and a yield for the ContentTask that is requested below. @@ +15,5 @@ > + registerCleanupFunction(() => gBrowser.removeCurrentTab()); > + > + BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser).then(() => { > + let browser = gBrowser.selectedBrowser; > + let window = gBrowser.selectedBrowser.contentWindow; window is unused @@ +21,5 @@ > + > + ok(discovered.length > 0, "some feeds should be discovered"); > + > + let feeds = []; > + nit, remove this blank line @@ +26,5 @@ > + for (let aFeed of discovered) { > + feeds[aFeed.href] = true; > + } > + > + for (let aLink of browser.contentDocument.getElementsByTagName("link")) { This will use a CPOW on e10s. You need to use a ContentTask.spawn here.
Attachment #8734365 -
Flags: review?(jaws) → review-
Assignee | ||
Comment 4•8 years ago
|
||
Thanks for the review Jared! Here's an updated patch with your comments addressed, this is the associated try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2005e7c3a0bf
Attachment #8734365 -
Attachment is obsolete: true
Attachment #8735086 -
Flags: review?(jaws)
Comment 5•8 years ago
|
||
Comment on attachment 8735086 [details] [diff] [review] [PATCH v2] Fix test_feed_discovery.html to work with e10s enabled Review of attachment 8735086 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/general/browser_feed_discovery.js @@ +14,5 @@ > + ok(discovered.length > 0, "some feeds should be discovered"); > + > + let feeds = []; > + for (let aFeed of discovered) { > + feeds[aFeed.href] = true; Arrays expect an integer index. You should change feeds to be an object instead of an array, as this test is actually just defining properties on the array. `let feeds = {};` Everything else with it should work the same.
Attachment #8735086 -
Flags: review?(jaws) → review+
Assignee | ||
Comment 6•8 years ago
|
||
Thanks for the review! Added the final change and pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/ab6ca47677a8
Comment 7•8 years ago
|
||
Thanks for the needinfo, Jared. I was waiting for it to make to central, but since it's already on inbound I went ahead and updated the spreadsheet with this information.
Flags: needinfo?(felipc)
Comment 8•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ab6ca47677a8
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Updated•5 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•