Add Polymer tests for cr-settings-prefs

chrome/test/data/webui/settings/prefs_tests.js

Index: chrome/test/data/webui/settings/prefs_tests.js
diff --git a/chrome/test/data/webui/settings/prefs_tests.js b/chrome/test/data/webui/settings/prefs_tests.js
new file mode 100644
index 0000000000000000000000000000000000000000..95e8f487c94b30d9c6c726e009466fd1ee62cd3c
--- /dev/null
+++ b/chrome/test/data/webui/settings/prefs_tests.js
@@ -0,0 +1,289 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+/** @fileoverview Suite of tests for cr-settings-prefs. */
+cr.define('cr_settings_prefs', function() {
+  /**
+   * Mock of chrome.settingsPrivate API.
+   * @constructor
+   * @extends {chrome.settingsPrivate}
+   */
+  function MockSettingsApi() {
+    this.prefs = {};
+    this.listener_ = null;
+
+    // Hack alert: bind this instance's onPrefsChanged members to this.
+    this.onPrefsChanged = {
+      addListener: this.onPrefsChanged.addListener.bind(this),
+      removeListener: this.onPrefsChanged.removeListener.bind(this),
+    };
+
+    this.addBooleanPref_('top_level_pref', true);
+    this.addBooleanPref_('browser.enable_flash', false);
+    this.addBooleanPref_('browser.enable_html5', true);
+    this.addNumberPref_('device.overclock', .6);
+    this.addStringPref_('homepage', 'example.com');
+  }
+
+  MockSettingsApi.prototype = {
+    // chrome.settingsPrivate overrides.
+    onPrefsChanged: {
+      addListener: function(listener) {
+        this.listener_ = listener;
+      },
+
+      removeListener: function(listener) {
+        expectNotEquals(null, this.listener_);

[Dan Beam, 2015/09/11 21:49:13]

^ why?

[michaelpg, 2015/09/13 03:01:06]

why not?

I don't much care; nobody calls this anyway.

+        this.listener_ = null;
+      },
+    },
+
+    getAllPrefs: function(callback) {
+      // Send a copy of prefs to keep our internal state private.
+      var prefs = [];
+      for (var key in this.prefs)
+        prefs.push(this.createCopy_(this.prefs[key]));
+
+      setTimeout(callback.bind(null, prefs));
+    },
+
+    setPref: function(key, value, pageId, callback) {
+      var pref = this.prefs[key];
+      assertNotEquals(undefined, pref);
+      assertEquals(typeof value, typeof pref.value);
+      assertEquals(Array.isArray(value), Array.isArray(pref.value));
+
+      if (this.failNextSetPref_) {
+        setTimeout(callback.bind(null, false));
+        this.failNextSetPref_ = false;
+        return;
+      }
+
+      // TODO(michaelpg): support list and dict prefs.
+      var changed = this.prefs[key].value != value;
+      this.prefs[key].value = value;
+      setTimeout(function() {
+        callback(true);
+        // Like chrome.settingsPrivate, send a notification when prefs change.
+        if (changed)
+          this.sendPrefChanges([key], [value]);
+      }.bind(this));
+    },
+
+    getPref: function(key, callback) {
+      var pref = this.prefs[key];
+      assertNotEquals(undefined, pref);
+
+      setTimeout(callback.bind(null, this.createCopy_(pref)));
+    },
+
+    // Functions used by tests.
+
+    /** Instructs the API to fail when setPref is next called. */
+    failNextSetPref: function() {
+      this.failNextSetPref_ = true;
+    },
+
+    /**
+     * Notifies the listener of pref changes.
+     * @param {!Array<string>} keys
+     * @param {!Array<*>} values
+     */
+    sendPrefChanges: function(keys, values) {
+      assertNotEquals(null, this.listener_);

[Dan Beam, 2015/09/11 21:49:12]

assert(this.listener_); (or just remove this because it'll blow up later)

[michaelpg, 2015/09/13 03:01:06]

I can't figure out why, but assert doesn't play nicely with the rest of the
test.

Unlike the other assertion functions, assert is defined in
ui/webui/resources/js/assert.js, not test_api.js. But it should still work...
yet calling it results in mocha catching a global error which it can't provide
any information for.

I've verified that assert.toString() is the same as in assert.js. And if I copy
and paste the function from assert.js into test_api.js, calling it works as
expected (we get a nice pretty error message). I can only suspect test_api.js is
intercepting the call or the exception in some weird way.

+
+      var prefs = [];
+      for (var i = 0; i < keys.length; i++) {
+        var key = keys[i];
+        var pref = this.prefs[key];
+        pref.value = values[i];
+        prefs.push(this.createCopy_(pref));
+      }
+
+      setTimeout(this.listener_.bind(null, prefs));
+    },
+
+    // Private methods for use by the mock API.
+
+    /**
+     * @param {!chrome.settingsPrivate.PrefType} type
+     * @param {string} key
+     * @param {*} value
+     */
+    addPref_: function(type, key, value) {
+      this.prefs[key] = {
+        type: type,
+        key: key,
+        value: value,
+      };
+    },
+
+    /**
+     * @param {string} key
+     * @param {boolean} value
+     */
+    addBooleanPref_: function(key, value) {
+      this.addPref_('BOOLEAN', key, value);

[stevenjb, 2015/09/11 21:03:47]

chrome.settingsPrivate.PrefType.BOOLEAN (throughout)

[Dan Beam, 2015/09/11 21:49:12]

are these values ('BOOLEAN', 'STRING', 'NUMBER') in an @enum {string} somewhere?

[michaelpg, 2015/09/13 03:01:06]

Done.

[michaelpg, 2015/09/13 03:01:06]

Done.

+    },
+
+    /**
+     * @param {string} key
+     * @param {number} value
+     */
+    addNumberPref_: function(key, value) {
+      this.addPref_('NUMBER', key, value);
+    },
+
+    /**
+     * @param {string} key
+     * @param {string} value
+     */
+    addStringPref_: function(key, value) {
+      this.addPref_('STRING', key, value);
+    },
+
+    /**
+     * Creates a deep copy of the preference.
+     * @param {chrome.settingsPrivate.PrefObject} pref
+     * @return {chrome.settingsPrivate.PrefObject}
+     */
+    createCopy_: function(pref) {
+      return JSON.parse(JSON.stringify(pref));

[stevenjb, 2015/09/11 21:03:47]

Can we use Object.Assign here?
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...

[Dan Beam, 2015/09/11 21:49:12]

I guess using Object.create() here would be too much of a hack?

[michaelpg, 2015/09/13 03:01:06]

Done.

[michaelpg, 2015/09/13 03:01:06]

Used Object.assign instead.

+    },
+  };
+
+  /**
+   * Helper function to deal with asynchronicity by repeatedly queueing
+   * tasks to check if the callback returns true.
+   * @param {function(): boolean} callback
+   * @return {Promise}
+   */
+  function waitUntilTrue(callback) {
+    return new Promise(function(resolve, reject) {
+      var interval = setInterval(function() {

[Dan Beam, 2015/09/11 21:49:12]

ew

[michaelpg, 2015/09/13 03:01:06]

agreed. not sure how to solve this, though, without imposing specifications on
how many tasks (setTimeout) the API and UI are allowed to post.

we could make things easier by making the mock API synchronous, but I think our
code should be allowed to assume the API is async.

+        if (callback.call()) {
+          clearInterval(interval);
+          resolve();
+        }
+      });

[Dan Beam, 2015/09/11 21:49:12]

wait, do this every 0ms?

[michaelpg, 2015/09/13 03:01:06]

Yeah, it posts a task each time. If a function posts five tasks (using
setTimeout) this function will run at most five times.

So these calls are interleaved with the running test; it doesn't spin for
dozens/hundreds of cycles. Changing the timeout from 0ms to 100ms is unlikely to
save much work and will just slow down the test (albeit not by much).

Maybe we should impose a limit, e.g. can't be called more than 10 times?

+    });
+  }
+
+  function registerTests() {
+    suite('CrSettingsPrefs', function() {
+      /**
+       * Prefs instance created before each test.
+       * @type {CrSettingsPrefs}
+       */
+      var prefs;
+
+      /** @type {MockSettingsApi} */
+      var mockApi = null;
+
+      // Initialize a <cr-settings-prefs> element before each test.
+      setup(function(done) {
+        mockApi = new MockSettingsApi();
+        // TODO(michaelpg): don't use global variables to inject the API.
+        window.mockApi = mockApi;
+
+        // Create and attach the <cr-settings-prefs> element.
+        PolymerTest.clearBody();
+        prefs = document.createElement('cr-settings-prefs');
+        document.body.appendChild(prefs);

[Dan Beam, 2015/09/11 21:49:13]

damn it http://jsfiddle.net/11hefdtv/

[michaelpg, 2015/09/13 03:01:06]

Yyyyeeeppp. The browser creates and initializes everything in
document.createElement and returns the element ready-to-go...

"factoryImpl" should be called "convenientlySetStuffAfterCreation" because
that's why it was written -- it's called with the parameters to a "constructor"
which simply calls document.createElement followed by el.factoryImpl(arguments).

+
+        window.mockApi = undefined;
+
+        // Wait for CrSettingsPrefs.INITIALIZED.
+        if (!CrSettingsPrefs.isInitialized) {

[Dan Beam, 2015/09/11 21:49:12]

can haz promise instead?  CrSettingsPrefs.initialized.then()?

maybe todo?

[michaelpg, 2015/09/13 03:01:06]

Good idea, added the todo to prefs_types.js.

+          var listener = function() {
+            document.removeEventListener(CrSettingsPrefs.INITIALIZED, listener);
+            done();
+          };
+          document.addEventListener(CrSettingsPrefs.INITIALIZED, listener);
+          return;
+        }
+
+        done();
+      });
+
+      test('receives and caches prefs', function() {
+        // Test that each pref has been successfully copied to the Polymer
+        // |prefs| property.
+        for (var key in mockApi.prefs) {
+          var expectedPref = mockApi.prefs[key];
+          var actualPref = prefs.prefs;
+          // Find the preference.
+          var path = key.split('.');
+          var skipPref = false;
+          for (var part of path) {
+            actualPref = actualPref[part];
+            if (!expectNotEquals(undefined, actualPref)) {
+              // We've already registered an error, so skip the pref.
+              skipPref = true;
+              break;
+            }
+          }

[stevenjb, 2015/09/11 21:03:47]

nit: Maybe wrap the above in a function to avoid the nested break/continue which
is a bit awkward, e.g.
  var actualPref = getActualPref(key, pref);
  if (!expectNotEquals(undefined, actualPref))
    continue;

[michaelpg, 2015/09/13 03:01:06]

Done.

+          if (skipPref)
+            continue;
+
+          expectEquals(expectedPref.key, actualPref.key);
+          expectEquals(expectedPref.type, actualPref.type);
+          if (expectedPref.type != chrome.settingsPrivate.LIST) {
+            expectEquals(expectedPref.value, actualPref.value);
+          } else {
+            expectEquals(JSON.stringify(expectedPref.value),
+                         JSON.stringify(actualPref.value));

[Dan Beam, 2015/09/11 21:49:12]

just do this for numbers/integers as well?

[michaelpg, 2015/09/13 03:01:06]

but but but... it's perf season!

+          }
+        }
+      });
+
+      test('forwards pref changes to API', function() {
+        // Test that cr-settings-prefs uses the setPref API.
+        prefs.set('prefs.browser.enable_flash.value', true);
+        expectTrue(mockApi.prefs['browser.enable_flash'].value);

[Dan Beam, 2015/09/11 21:49:13]

\n

[michaelpg, 2015/09/13 03:01:06]

Done.

+        prefs.set('prefs.device.overclock.value', 1.2);
+        expectEquals(1.2, mockApi.prefs['device.overclock'].value);

[Dan Beam, 2015/09/11 21:49:12]

\n

[michaelpg, 2015/09/13 03:01:06]

Done.

+        prefs.set('prefs.homepage.value', 'chromium.org');
+        expectEquals('chromium.org', mockApi.prefs['homepage'].value);
+
+        // Test that when setPref fails, the pref is reverted locally.
+        mockApi.failNextSetPref();
+        prefs.set('prefs.browser.enable_flash.value', false);

[Dan Beam, 2015/09/11 21:49:13]

\n

[michaelpg, 2015/09/13 03:01:06]

Done.

+        mockApi.failNextSetPref();
+        prefs.set('prefs.device.overclock.value', 2.0);

[Dan Beam, 2015/09/11 21:49:13]

\n

[michaelpg, 2015/09/13 03:01:06]

Done.

+        mockApi.failNextSetPref();
+        prefs.set('prefs.homepage.value', 'invalid-url');
+
+        // Multiple asynchronous round trips need to happen.
+        return Promise.all([
+          waitUntilTrue(function() {
+            return prefs.prefs.browser.enable_flash.value;
+          }),
+          waitUntilTrue(function() {
+            return prefs.prefs.device.overclock.value == 1.2;
+          }),
+          waitUntilTrue(function() {
+            return prefs.prefs.homepage.value == 'chromium.org';
+          }),
+        ]);
+      });
+
+      test('responds to API changes', function(done) {
+        mockApi.sendPrefChanges(['top_level_pref'], [false]);
+        PolymerTest.async(function() {
+          expectEquals(false, prefs.prefs.top_level_pref.value);
+
+          mockApi.sendPrefChanges(['top_level_pref'], [true]);
+        }).async(function() {

[Dan Beam, 2015/09/11 21:49:12]

can't PolymerTest.async() just return a promise and we can do
PolymerTest.async().then()?

[michaelpg, 2015/09/13 03:01:06]

no, we want to chain this logic more than once ("call this function
asynchronously and then resolve asynchronously"). Putting the expectEquals in a
promise instead of using .async means it would run before any setTimeouts posted
inside of fn(), which in practice means pref is checked before the prefs element
gets a chance to react to the mock API.

In this case we could do .then(setTimeout(...)) but PolymerTest.async lets us do
that *and* makes it prettier IMO.

+          expectEquals(true, prefs.prefs.top_level_pref.value);
+          done();
+        });
+      });
+    });
+  }
+
+  return {
+    registerTests: registerTests,
+  };
+});