1257785 - Fix test_offlineNotification.html to work on e10s

Status: NEW

(Core :: General, defect)

Description

[Gabriele Svelto [:gsvelto] (PTO)]

+++ This bug was initially created as a clone of Bug #1257488 +++

As per title. I'm not sure if this also belongs to the Networking: Cache component like 1257488 as the original bug that introduced the test didn't (bug 462856) so I'm leaving it in general for the time being.

Comment 1

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH] Fix test_offlineNotification.html to work with e10s enabled

Comment 2

[Gabriele Svelto [:gsvelto] (PTO)]

The current patch times out when sent to Try, there must be some race in my code or possibly some issue with the exit condition.

Comment 3

[Gabriele Svelto [:gsvelto] (PTO)]

The issue I'm seeing on Try is related to the remote location of the third test frame, I'll try to fix that.

Comment 4

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH v2] Fix test_offlineNotification.html to work with e10s enabled

I'm asking the review to you Dave as you were the author of the original test, if this is not appropriate please redirect the review.

This patch changes the test_offlineNotificaition.html plan mochitest into a mochitest-browser test, cleans it up and ensures that it works correctly in e10s mode. The test is essentially unchanged, save for some minor cleanups but it does away with the requirement to simulate the clicks on the notifications because pushing the offline-apps.allow_by_default=true preference is enough to get the same result.

The try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5709c4e497cc

Comment 5

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH v3] Fix test_offlineNotification.html to work with e10s enabled

Refreshsed the patch, Jared could you help me by reviewing this? This is a change similar to the one I did in bug 1259431 but with the significant difference that due to the nature of the test I was unable to rewrite in a cleaner way. It's basically a straightforward port of the old test.

The try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=819f8a5517cf

Comment 6

[Honza Bambas (:mayhemer)]

To save you some cycles here, note that we are currently discussing disabling/removing appcache to happen very soon.  I'd wait with review/further dev on this until the decision is made.

Comment 7

[Gabriele Svelto [:gsvelto] (PTO)]

Thanks for the thumbs up; I heard that appcache was going away which is why I tried to keep this change as straightforward as possible.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8737743 [details] [diff] [review]
[PATCH v3] Fix test_offlineNotification.html to work with e10s enabled

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

Without seeing any patches still on bug 1237782, I'll go ahead with this review.

::: browser/base/content/test/general/browser_offlineNotification.js
@@ +38,5 @@
> +  ++count;
> +  is(evt.type, expectedEvent, "Wrong event!");
> +}
> +
> +function testEventHandling(w) {

s/w/win/

@@ +39,5 @@
> +  is(evt.type, expectedEvent, "Wrong event!");
> +}
> +
> +function testEventHandling(w) {
> +  var events = [ "checking",

s/var/let/

@@ +47,5 @@
> +                 "progress",
> +                 "updateready",
> +                 "cached",
> +                 "obsolete"];
> +  var e;

s/e/event

@@ +77,5 @@
> +  is(count, 1, "Wrong number events!");
> +}
> +
> +function test() {
> +  waitForExplicitFinish();

Ideally since this test is being rewritten it would be written in the add_task style.

@@ +79,5 @@
> +
> +function test() {
> +  waitForExplicitFinish();
> +
> +  Services.prefs.setBoolPref("offline-apps.allow_by_default", true);

This can use SpecialPowers.pushPrefEnv so we won't need the cleanup function.

@@ +83,5 @@
> +  Services.prefs.setBoolPref("offline-apps.allow_by_default", true);
> +
> +  // Open a new tab.
> +  gBrowser.selectedTab = gBrowser.addTab(URL);
> +  registerCleanupFunction(() => gBrowser.removeCurrentTab());

This could be written with BrowserTestUtils.jsm's BrowserTestUtils.withNewTab that will automatically take care of closing the tab for you and will wait until the tab is loaded before execution of the supplied taskFn.

Comment 9

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> Ideally since this test is being rewritten it would be written in the
> add_task style.

Yeah, unfortunately the way we need to cleanup after ourselves by removing the offline-app makes changing the test tricky and pushing "offline-apps.allow_by_default",false isn't enough (glancing at the affected code it seems that the pref works only one way). Also, since this functionality is supposed to go away soon the tests will go away too hence I didn't want to spend too much time refactoring them.

> This can use SpecialPowers.pushPrefEnv so we won't need the cleanup function.

Right.

> This could be written with BrowserTestUtils.jsm's
> BrowserTestUtils.withNewTab that will automatically take care of closing the
> tab for you and will wait until the tab is loaded before execution of the
> supplied taskFn.

OK, I'll do that.

Comment 10

[Gabriele Svelto [:gsvelto] (PTO)]

Actually scratch my previous comment, I just discovered that the way I've written this test is racy because the cache update events can be sent before the tab has been loaded (and I can reproduce the situation rather easily on my machine). So I'm rewriting it in the add_task() style making sure that I attach the event handlers without waiting for the tab to be fully loaded.

Comment 11

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH v4] Fix test_offlineNotification.html to work with e10s enabled

Sorry for asking to re-review this but the changes I made to turn it into the add_task() style are rather extensive. I'm now opening the tab without explicitly waiting for it to be loaded, then immediately waiting for the success events, running the tests, waiting for the events to trigger and finally cleaning up everything as was done before.

This style and the use of BrowserTestUtils.waitForEvent() did indeed make the test a lot more terse and understandable IMHO.

The try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=29a580a6103b

Comment 12

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8740980 [details] [diff] [review]
[PATCH v4] Fix test_offlineNotification.html to work with e10s enabled

Ugh, the try run came out all broken, I'll check what's wrong first and then ask for review again.

Comment 13

[Gabriele Svelto [:gsvelto] (PTO)]

I've spent two more days trying to fix this but I cannot come up with an approach that does not yield intermittent failures. Since I can't seem to make any progress and since the feature that's being tested is going away I suggest we just leave this bug disabled on e10s and remove it once the appcache is gone.