Closed Bug 1253274 Opened 7 years ago Closed 6 years ago

Update browser_bug435325.js to use add_task and BrowserTestUtils

Categories

(Firefox :: General, defect, P5)

Product:
Firefox
Component:
General
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mconley, Assigned: prathiksha, Mentored)

References

Details

Attachments

(1 file)

Bug 1253274 - Update browser_bug435325.js to use add_task and BrowserTestUtils.
59 bytes, text/x-review-board-request
johannh
: review+
Details 
In bug 1099156, we tweaked browser_bug435325.js to work with e10s, but this entire test could be simplified using add_task and some of the good stuff in BrowserTestUtils. We should do that when we have a chance.
maybe we can put this into some well-named test file instead of bug specific file. Do you have any suggestion?
Flags: needinfo?(mconley)
browser_go_online_from_neterror.js ?
Flags: needinfo?(mconley)
On that occasion we should also use BrowserTestUtils.waitForErrorPage instead of waiting for DOMContentLoaded.

This could be a fun task for someone who likes cleaning up tests (who doesn't?).

I'll be happy to mentor.
Blocks: 1334455
Mentor: jhofmann
Priority: -- → P5
Assignee: nobody → prathikshaprasadsuman
Comment on attachment 8857416 [details]
Bug 1253274 - Update browser_bug435325.js to use add_task and BrowserTestUtils.

https://reviewboard.mozilla.org/r/129412/#review133652

Thank you for tackling this, it's a great start but there are even more things we can improve! :)

Please take a look at the comments and let me know if anything is unclear.

::: browser/base/content/test/general/browser_bug435325.js:6
(Diff revision 1)
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  /* Ensure that clicking the button in the Offline mode neterror page makes the browser go online. See bug 435325. */
>  
>  var proxyPrefValue;

I think this can be declared inline.

::: browser/base/content/test/general/browser_bug435325.js:8
(Diff revision 1)
>  
>  /* Ensure that clicking the button in the Offline mode neterror page makes the browser go online. See bug 435325. */
>  
>  var proxyPrefValue;
>  
> -function test() {
> +add_task(function* checkSwitchPagetoOnlineMode() {

Nit: capitalize the "to"

::: browser/base/content/test/general/browser_bug435325.js:9
(Diff revision 1)
>  /* Ensure that clicking the button in the Offline mode neterror page makes the browser go online. See bug 435325. */
>  
>  var proxyPrefValue;
>  
> -function test() {
> +add_task(function* checkSwitchPagetoOnlineMode() {
>    waitForExplicitFinish();

You don't need to call this anymore. waitForExplicitFinish is used in old tests to make things asynchronous (the test only finishes when we call finish() at the end), but since this task is running a generator function we don't need to do that anymore.

::: browser/base/content/test/general/browser_bug435325.js:20
(Diff revision 1)
>    // reachable in offline mode.  To avoid this, disable any proxy.
>    proxyPrefValue = Services.prefs.getIntPref("network.proxy.type");
>    Services.prefs.setIntPref("network.proxy.type", 0);
>  
>    Services.prefs.setBoolPref("browser.cache.disk.enable", false);
>    Services.prefs.setBoolPref("browser.cache.memory.enable", false);

You can set the two prefs above using SpecialPowers.pushPrefEnv (https://searchfox.org/mozilla-central/rev/4bd7a206dea5382c97a8a0c30beef668cc449f5b/testing/specialpowers/content/specialpowersAPI.js#1035) instead.

Here's an example of how to use it:
https://searchfox.org/mozilla-central/rev/4bd7a206dea5382c97a8a0c30beef668cc449f5b/browser/base/content/test/general/browser_trackingUI_6.js#21

You can then also remove them from the registerCleanupFunction call below.

::: browser/base/content/test/general/browser_bug435325.js:22
(Diff revision 1)
>    Services.prefs.setIntPref("network.proxy.type", 0);
>  
>    Services.prefs.setBoolPref("browser.cache.disk.enable", false);
>    Services.prefs.setBoolPref("browser.cache.memory.enable", false);
>  
> +  info("Loading the net error page in the offline mode and making sure 'Try again' switches it to online mode");

I don't think we need this log, the test is simple enough :)

::: browser/base/content/test/general/browser_bug435325.js:30
(Diff revision 1)
> +  let NetErrorLoaded;
> +  yield BrowserTestUtils.openNewForegroundTab(gBrowser, () => {
> -  gBrowser.selectedTab = gBrowser.addTab("http://example.com/");
> +    gBrowser.selectedTab = gBrowser.addTab("http://example.com/");
> +    browser = gBrowser.selectedBrowser;
> +    NetErrorLoaded = BrowserTestUtils.waitForErrorPage(browser);
> +  }, false);

This should work, but using BrowserTestUtils.withNewTab is probably a lot cleaner here.

This file shows a relatively simple example:

https://searchfox.org/mozilla-central/rev/4bd7a206dea5382c97a8a0c30beef668cc449f5b/browser/components/sessionstore/test/browser_duplicate_history.js#9

::: browser/base/content/test/general/browser_bug435325.js:34
(Diff revision 1)
> -  mm.addMessageListener("Test:DOMContentLoaded", pageloaded);
> -  mm.loadFrameScript("data:," + contentScript, true);
> -}
>  
> -function checkPage(data) {
>    ok(Services.io.offline, "Setting Services.io.offline to true.");

I know you didn't write it but I don't see how this assertion is in any way useful. I think we can remove it. (If it were used for anything it should really have a comment and/or a better assertion message).

::: browser/base/content/test/general/browser_bug435325.js:37
(Diff revision 1)
>  
> -function checkPage(data) {
>    ok(Services.io.offline, "Setting Services.io.offline to true.");
> +  yield NetErrorLoaded;
>  
> -  is(data.uri.substring(0, 27),
> +  ok(content.document.documentURI.startsWith("about:neterror?e=netOffline"), "Should be showing error page");

Hm, I'm not sure if this will work here.

Doing this kind of assertion is a good idea though, why don't you just move it inside the ContentTask.spawn block below? :)

::: browser/base/content/test/general/browser_bug435325.js:44
(Diff revision 1)
>    // Re-enable the proxy so example.com is resolved to localhost, rather than
>    // the actual example.com.
>    Services.prefs.setIntPref("network.proxy.type", proxyPrefValue);
>  
> -  Services.obs.addObserver(function observer(aSubject, aTopic) {
> -    ok(!Services.io.offline, "After clicking the Try Again button, we're back " +
> +  // Click on the 'Try again' button.
> +  yield ContentTask.spawn(browser, null, function* () {

Nit: No space after function*

::: browser/base/content/test/general/browser_bug435325.js
(Diff revision 1)
> -  Services.obs.addObserver(function observer(aSubject, aTopic) {
> -    ok(!Services.io.offline, "After clicking the Try Again button, we're back " +
> +  // Click on the 'Try again' button.
> +  yield ContentTask.spawn(browser, null, function* () {
> -                             "online.");
> -    Services.obs.removeObserver(observer, "network:offline-status-changed");
> -    finish();
> -  }, "network:offline-status-changed", false);

Note that you still have to listen to the network:offline-status-changed event to be called after you clicked the button. You can use the TestUtils.topicObserved function for that.

https://searchfox.org/mozilla-central/rev/4bd7a206dea5382c97a8a0c30beef668cc449f5b/testing/modules/TestUtils.jsm#48
Attachment #8857416 - Flags: review?(jhofmann) → review-
Comment on attachment 8857416 [details]
Bug 1253274 - Update browser_bug435325.js to use add_task and BrowserTestUtils.

https://reviewboard.mozilla.org/r/129412/#review133652

Okay. I'll make the changes. Thank you :))
Comment on attachment 8857416 [details]
Bug 1253274 - Update browser_bug435325.js to use add_task and BrowserTestUtils.

https://reviewboard.mozilla.org/r/129412/#review142770

This looks great already! Thanks for your work. There's another thing we need to do: Bug 1353542 recently switched tests away from function* and yield to using async/await (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function). We should do the same for this test, it's just a matter of replacing some keywords.

There are also some minor issues to resolve. :)

Thank you!

::: browser/base/content/test/general/browser_bug435325.js:16
(Diff revision 2)
>    // Tests always connect to localhost, and per bug 87717, localhost is now
>    // reachable in offline mode.  To avoid this, disable any proxy.
> -  proxyPrefValue = Services.prefs.getIntPref("network.proxy.type");
> -  Services.prefs.setIntPref("network.proxy.type", 0);
> -
> -  Services.prefs.setBoolPref("browser.cache.disk.enable", false);
> +  var proxyPrefValue = yield SpecialPowers.getIntPref("network.proxy.type");
> +  yield SpecialPowers.pushPrefEnv({"set": [["network.proxy.type", 0]]});
> +  yield SpecialPowers.pushPrefEnv({"set": [["browser.cache.disk.enable", false]]});
> +  yield SpecialPowers.pushPrefEnv({"set": [["browser.cache.memory.enable", false]]});

You don't need to call pushPrefEnv multiple times in a row, the set field accepts an array of multiple values to set, example:

https://searchfox.org/mozilla-central/rev/ae24a3c83d22e0e35aecfd9049c2b463ca7e045b/browser/base/content/test/forms/browser_selectpopup.js#214

::: browser/base/content/test/general/browser_bug435325.js:19
(Diff revision 2)
> -  Services.prefs.setBoolPref("browser.cache.memory.enable", false);
> -
> -  gBrowser.selectedTab = gBrowser.addTab("http://example.com/");
>  
> -  let contentScript = `
> -    let listener = function () {
> +  yield BrowserTestUtils.withNewTab("about:blank", function* (browser) {
> +    let NetErrorLoaded = BrowserTestUtils.waitForErrorPage(browser);

Nit: don't capitalize the variable name

::: browser/base/content/test/general/browser_bug435325.js:26
(Diff revision 2)
> -  is(data.uri.substring(0, 27),
> -     "about:neterror?e=netOffline", "Loading the Offline mode neterror page.");
>  
> -  // Re-enable the proxy so example.com is resolved to localhost, rather than
> +    // Re-enable the proxy so example.com is resolved to localhost, rather than
> -  // the actual example.com.
> +    // the actual example.com.
> -  Services.prefs.setIntPref("network.proxy.type", proxyPrefValue);
> +    yield SpecialPowers.pushPrefEnv({"set": [["network.proxy.type", proxyPrefValue]]});

You can use SP.pushPrefEnv({"clear": [["network.proxy.type"]]} instead.
Attachment #8857416 - Flags: review?(jhofmann)
Comment on attachment 8857416 [details]
Bug 1253274 - Update browser_bug435325.js to use add_task and BrowserTestUtils.

https://reviewboard.mozilla.org/r/129412/#review142770

I'll do that. Thank you :)
Comment on attachment 8857416 [details]
Bug 1253274 - Update browser_bug435325.js to use add_task and BrowserTestUtils.

https://reviewboard.mozilla.org/r/129412/#review143340

Looks great now, thanks!

::: browser/base/content/test/general/browser_bug435325.js:13
(Diff revision 3)
>    // Go offline and disable the proxy and cache, then try to load the test URL.
>    Services.io.offline = true;
>  
>    // Tests always connect to localhost, and per bug 87717, localhost is now
>    // reachable in offline mode.  To avoid this, disable any proxy.
> -  proxyPrefValue = Services.prefs.getIntPref("network.proxy.type");
> +  await SpecialPowers.pushPrefEnv({"set": [["network.proxy.type", 0], ["browser.cache.disk.enable", false], ["browser.cache.memory.enable", false]]});

Nit: please format this over multiple lines
Attachment #8857416 - Flags: review?(jhofmann) → review+
Comment on attachment 8857416 [details]
Bug 1253274 - Update browser_bug435325.js to use add_task and BrowserTestUtils.

https://reviewboard.mozilla.org/r/129412/#review143408

Ugh sorry I think I know why the test is failing, my advice here was wrong:

> You can use SP.pushPrefEnv({"clear": [["network.proxy.type"]]} instead.

Mochitest uses the built-in proxy settings and we need to restore it properly (without clearing) to keep the tests working (which is a bit annoying).

::: browser/base/content/test/general/browser_bug435325.js:14
(Diff revision 4)
>    Services.io.offline = true;
>  
>    // Tests always connect to localhost, and per bug 87717, localhost is now
>    // reachable in offline mode.  To avoid this, disable any proxy.
> -  proxyPrefValue = Services.prefs.getIntPref("network.proxy.type");
> -  Services.prefs.setIntPref("network.proxy.type", 0);
> +  await SpecialPowers.pushPrefEnv({"set": [["network.proxy.type", 0], ["browser.cache.disk.enable", false],
> +    ["browser.cache.memory.enable", false]]});

You could make this look a bit nicer, e.g.

await SpecialPowers.pushPrefEnv({"set": [
  ["network.proxy.type", 0],
  ["browser.cache.disk.enable", false],
  ["browser.cache.memory.enable", false],
]});
Comment on attachment 8857416 [details]
Bug 1253274 - Update browser_bug435325.js to use add_task and BrowserTestUtils.

https://reviewboard.mozilla.org/r/129412/#review143408

Johann, any idea why the test is failing on windows? :)
(In reply to Prathiksha from comment #15)
> Comment on attachment 8857416 [details]
> Bug 1253274 - Update browser_bug435325.js to use add_task and
> BrowserTestUtils.
> 
> https://reviewboard.mozilla.org/r/129412/#review143408
> 
> Johann, any idea why the test is failing on windows? :)

Oh, sorry, I missed this comment. That's called an "intermittent failure", an error that is just caused by flaky tests or the build machines having a bad day. Intermittent failures are not caused by you and can be safely ignored (unless you caused them ;)). Good indicators for intermittent tests are:

- Is the failing test already associated with a known intermittent (it's shown in the Treeherder UI)?
- Is the failing test completely unrelated to your change?
- Is the failing test only failing on one platform?

You can go ahead and land!
Haha. Ok. Thanks :)
Keywords: checkin-needed
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s a35786fe5b30 -d 97a22f66e322: rebasing 396588:a35786fe5b30 "Bug 1253274 - Update browser_bug435325.js to use add_task and BrowserTestUtils. r=johannh" (tip)
merging browser/base/content/test/general/browser_bug435325.js
warning: conflicts while merging browser/base/content/test/general/browser_bug435325.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment on attachment 8857416 [details]
Bug 1253274 - Update browser_bug435325.js to use add_task and BrowserTestUtils.

https://reviewboard.mozilla.org/r/129412/#review144066

Let me know if I can help you rebase this. There's one more thing you could do since you haven't landed this yet: rename the file to browser_go_online_from_neterror.js, as suggested in comment 2.

Also two more nits to fix:

::: browser/base/content/test/general/browser_bug435325.js:13
(Diff revision 5)
>    // Go offline and disable the proxy and cache, then try to load the test URL.
>    Services.io.offline = true;
>  
>    // Tests always connect to localhost, and per bug 87717, localhost is now
>    // reachable in offline mode.  To avoid this, disable any proxy.
> -  proxyPrefValue = Services.prefs.getIntPref("network.proxy.type");
> +  var proxyPrefValue = SpecialPowers.getIntPref("network.proxy.type");

Please use let instead of var.

::: browser/base/content/test/general/browser_bug435325.js:17
(Diff revision 5)
>    // reachable in offline mode.  To avoid this, disable any proxy.
> -  proxyPrefValue = Services.prefs.getIntPref("network.proxy.type");
> -  Services.prefs.setIntPref("network.proxy.type", 0);
> -
> -  Services.prefs.setBoolPref("browser.cache.disk.enable", false);
> -  Services.prefs.setBoolPref("browser.cache.memory.enable", false);
> +  var proxyPrefValue = SpecialPowers.getIntPref("network.proxy.type");
> +  await SpecialPowers.pushPrefEnv({"set": [
> +    ["network.proxy.type", 0],
> +    ["browser.cache.disk.enable", false],
> +    ["browser.cache.memory.enable", false]

Put a trailing comma here
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/23dfbd6437b8
Update browser_bug435325.js to use add_task and BrowserTestUtils. r=johannh
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/23dfbd6437b8
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.