731866 - Copy and port current preference tests to run against the in-content preferences

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Zuhao(Joe) Chen]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_2) AppleWebKit/535.11 (KHTML, like Gecko) Chrome/17.0.963.56 Safari/535.11

Steps to reproduce:

Testing for in-content preferences


Actual results:

Testing for in-content preferences


Expected results:

Testing for in-content preferences

Comment 2

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

This bug is not a duplicate. It is part of the project to move preferences to in-content UI.

There are a number of tests located at /browser/components/preferences/tests that will need to be copied to /browser/components/preferences/in-content/tests

Comment 3

[Zuhao(Joe) Chen]

Attachment: Tests for in-content preferences

Comment 4

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

Comment on attachment 605198 [details] [diff] [review]
Tests for in-content preferences

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

From a cursory glance, this looks to be what in the right direction, but they're still testing against the old user interface.

::: browser/components/preferences/in-content/tests/Makefile.in
@@ +1,2 @@
> +# ***** BEGIN LICENSE BLOCK *****
> +# Version: MPL 1.1/GPL 2.0/LGPL 2.1

MPL 2.0 here.

::: browser/components/preferences/in-content/tests/privacypane_tests.js
@@ +56,5 @@
> +    }
> +  };
> +  Services.ww.registerNotification(observer);
> +
> +  let dialog = openDialog("chrome://browser/content/preferences/preferences.xul", "Preferences",

These tests are opening the old preferences, so they will need to be updated to use the in-content prefs.

These tests should fail if you don't have the patch for the privacy preferences applied :)

Comment 5

[Zuhao(Joe) Chen]

Tried to run the test with the following command:
TEST_PATH=browser/components/preferences/in-content/tests/ make -C obj-ff-dbg/ mochitest-plain

but it's giving me the error test not found. Any advise?

(In reply to Jared Wein [:jaws] from comment #4)
> Comment on attachment 605198 [details] [diff] [review]
> Tests for in-content preferences
> 
> Review of attachment 605198 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> From a cursory glance, this looks to be what in the right direction, but
> they're still testing against the old user interface.
> 
> ::: browser/components/preferences/in-content/tests/Makefile.in
> @@ +1,2 @@
> > +# ***** BEGIN LICENSE BLOCK *****
> > +# Version: MPL 1.1/GPL 2.0/LGPL 2.1
> 
> MPL 2.0 here.
> 
> ::: browser/components/preferences/in-content/tests/privacypane_tests.js
> @@ +56,5 @@
> > +    }
> > +  };
> > +  Services.ww.registerNotification(observer);
> > +
> > +  let dialog = openDialog("chrome://browser/content/preferences/preferences.xul", "Preferences",
> 
> These tests are opening the old preferences, so they will need to be updated
> to use the in-content prefs.
> 
> These tests should fail if you don't have the patch for the privacy
> preferences applied :)

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Joe: Did you sort out the issues with running the tests? Do you have an updated patch?

(Sorry, I'm a bit behind on the status of things after being off sick.)

Comment 7

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

Joe: Have you made any progress on this?

Comment 8

[Jon Rietveld]

Attachment: individual tests

attempt at getting all the old tests to pass, most seem to be passing but having a few give one or two tests failing

Comment 9

[Jon Rietveld]

Attachment: in-content tests

Comment 10

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

Comment on attachment 612562 [details] [diff] [review]
in-content tests

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

Jon, can you push all your patches and this patch to tryserver to see if these tests pass on all platforms?

::: browser/components/preferences/in-content/preferences.js
@@ +11,5 @@
>  Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
>  
>  function init_all() {
> +  var initFinished = document.createEvent("Event");
> +  initFinished.initEvent("Initialized", true, true);

Move these two lines to the end of init_all, and use |let| instead of |var|.

::: browser/components/preferences/in-content/privacy.js
@@ +163,4 @@
>        this.dependentControls
>            .forEach(function (aElement)
>                     document.getElementById(aElement).disabled = disabled);
> +                   

Extraneous whitespace got added here.

::: browser/components/preferences/in-content/tests/browser_bug705422.js
@@ +36,5 @@
> +        cookieSvc.setCookieString(cookieUri, null, name+"="+value+";", null);
> +    }
> +
> +    // open cookie manager
> +    var cmd = window.openDialog("chrome://browser/content/preferences/in-content/cookies.xul",

Does cookies.xul need to be copied to the in-content folder? Or can we just use the current cookies.xul?

::: browser/components/preferences/in-content/tests/browser_bug731866.js
@@ +16,5 @@
> +  Services.obs.addObserver(observer, "browser-preferences-initialized", false);
> +
> +  // open about:preferences
> +  gBrowser.removeCurrentTab();
> +  gBrowser.selectedTab = gBrowser.addTab("chrome://browser/components/preferences/in-content/preferences.xul");

This should use ABOUT_PREFERENCES_SPEC instead of the full chrome URL.

::: browser/components/preferences/in-content/tests/privacypane_tests.js
@@ +11,5 @@
> +    browser.addEventListener("Initialized", function(aEvent) {
> +      dump("\nInitialized\n");
> +      browser.removeEventListener("Initialized", arguments.callee, true);
> +      setTimeout(function() {
> +  //      is(Services.prefs.getBoolPref("browser.preferences.inContent"), true, "In-content prefs are enabled");

These comments should be removed,

@@ +13,5 @@
> +      browser.removeEventListener("Initialized", arguments.callee, true);
> +      setTimeout(function() {
> +  //      is(Services.prefs.getBoolPref("browser.preferences.inContent"), true, "In-content prefs are enabled");
> +        is(browser.contentWindow.location.href, "about:preferences", "Checking if the preferences tab was opened");
> +        dump("\n here1 \n");

as well as these dump statements,

@@ +36,5 @@
> +//    testFunc(win.document.defaultView);
> +//    gBrowser.removeCurrentTab();
> +//    dump("\n here2 \n");
> +//    testRunner.runNext();
> +//  }, false);

and these comments too :)

@@ +485,5 @@
> +}
> +
> +let testRunner;
> +function run_test_subset(subset) {
> +  let instantApplyOrig = Services.prefs.getBoolPref("browser.preferences.instantApply");

We don't need to store the original pref value, see below.

@@ +496,5 @@
> +    counter: 0,
> +    runNext: function() {
> +      if (this.counter == this.tests.length) {
> +        // cleanup
> +        Services.prefs.setBoolPref("browser.preferences.instantApply", instantApplyOrig);

Services.prefs.clearUserPref("browser.preferences.instantApply");
and use registerCleanupFunction at the beginning of test() to reset this pref.

Comment 11

[Jon Rietveld]

Attachment: copied over tests + browser_bug731866.js patch

permissions and cookies tests were removed and browser_bug731866.js for testing pane switching was added. All pass as well : )

Comment 12

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

Comment on attachment 613790 [details] [diff] [review]
copied over tests + browser_bug731866.js patch

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

::: browser/components/preferences/in-content/preferences.js
@@ +13,5 @@
>  Components.utils.import("resource://services-sync/service.js");
>  
>  function init_all() {
> +  var initFinished = document.createEvent("Event");
> +  initFinished.initEvent("Initialized", true, true);

Please move these two lines to be right before the event is dispatched.

::: browser/components/preferences/in-content/tests/privacypane_tests.js
@@ +7,5 @@
> +    gBrowser.tabContainer.removeEventListener("TabOpen", arguments.callee, true);
> +    let browser = aEvent.originalTarget.linkedBrowser;
> +    browser.addEventListener("Initialized", function(aEvent) {
> +      browser.removeEventListener("Initialized", arguments.callee, true);
> +//      is(Services.prefs.getBoolPref("browser.preferences.inContent"), true, "In-content prefs are enabled");

Hmm, was this meant to be commented out? Is this check failing for some reason?

Comment 13

[Jon Rietveld]

Attachment: patch for testing

Comment 14

[Jon Rietveld]

Attachment: patch for tests

Comment 15

[Jon Rietveld]

Attachment: patch for tests

Comment 16

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

\o/ Pushed to mozilla-inbound. These patches should be merged to mozilla-central within the next day or two. Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b195889f55c

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8b195889f55c