Closed Bug 1259431 Opened 4 years ago Closed 4 years ago

Fix test_feed_discovery.html to work on e10s

Categories

(Firefox Graveyard :: RSS Discovery and Preview, defect)

defect
Not set

Tracking

(e10s+, firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
e10s + ---
firefox48 --- fixed

People

(Reporter: gsvelto, Assigned: gsvelto)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

As per title.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
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)
Needinfo to felipe as he is tracking e10s test fixes and this one is moving from mochitest-plain to mochitest-browser
Flags: needinfo?(felipc)
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-
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 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+
Thanks for the review! Added the final change and pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/ab6ca47677a8
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)
https://hg.mozilla.org/mozilla-central/rev/ab6ca47677a8
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.