Last Comment Bug 731866 - Copy and port current preference tests to run against the in-content preferences
: Copy and port current preference tests to run against the in-content preferences
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 15
Assigned To: Zuhao(Joe) Chen
:
Mentors:
Depends on: 820815
Blocks: 718011
  Show dependency treegraph
 
Reported: 2012-02-29 16:47 PST by Zuhao(Joe) Chen
Modified: 2012-12-12 06:22 PST (History)
6 users (show)
jaws: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Tests for in-content preferences (72.35 KB, patch)
2012-03-12 16:08 PDT, Zuhao(Joe) Chen
jaws: feedback+
Details | Diff | Review
individual tests (58.15 KB, patch)
2012-04-02 18:25 PDT, Jon Rietveld
no flags Details | Diff | Review
in-content tests (60.17 KB, patch)
2012-04-05 08:56 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Review
copied over tests + browser_bug731866.js patch (41.12 KB, patch)
2012-04-10 15:06 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Review
patch for testing (40.22 KB, patch)
2012-04-18 18:13 PDT, Jon Rietveld
no flags Details | Diff | Review
patch for tests (40.02 KB, patch)
2012-04-20 09:30 PDT, Jon Rietveld
no flags Details | Diff | Review
patch for tests (39.44 KB, patch)
2012-04-20 11:24 PDT, Jon Rietveld
jaws: review+
Details | Diff | Review

Description Zuhao(Joe) Chen 2012-02-29 16:47:05 PST
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 1 Jim Jeffery not reading bug-mail 1/2/11 2012-02-29 17:03:20 PST

*** This bug has been marked as a duplicate of bug 651803 ***
Comment 2 Jared Wein [:jaws] (please needinfo? me) 2012-02-29 18:03:12 PST
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 2012-03-12 16:08:40 PDT
Created attachment 605198 [details] [diff] [review]
Tests for in-content preferences
Comment 4 Jared Wein [:jaws] (please needinfo? me) 2012-03-13 00:56:50 PDT
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 2012-03-13 13:40:12 PDT
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] (mostly unavailable, needinfo open, reviews not) 2012-03-22 06:11:47 PDT
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) 2012-03-28 11:40:29 PDT
Joe: Have you made any progress on this?
Comment 8 Jon Rietveld 2012-04-02 18:25:22 PDT
Created attachment 611680 [details] [diff] [review]
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 2012-04-05 08:56:13 PDT
Created attachment 612562 [details] [diff] [review]
in-content tests
Comment 10 Jared Wein [:jaws] (please needinfo? me) 2012-04-05 11:52:07 PDT
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 2012-04-10 15:06:40 PDT
Created attachment 613790 [details] [diff] [review]
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) 2012-04-11 21:12:37 PDT
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 2012-04-18 18:13:38 PDT
Created attachment 616401 [details] [diff] [review]
patch for testing
Comment 14 Jon Rietveld 2012-04-20 09:30:24 PDT
Created attachment 617001 [details] [diff] [review]
patch for tests
Comment 15 Jon Rietveld 2012-04-20 11:24:40 PDT
Created attachment 617048 [details] [diff] [review]
patch for tests
Comment 16 Jared Wein [:jaws] (please needinfo? me) 2012-05-08 22:41:54 PDT
\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] 2012-05-09 07:44:09 PDT
https://hg.mozilla.org/mozilla-central/rev/8b195889f55c

Note You need to log in before you can comment on or make changes to this bug.