MD Settings: FakeSettingsPrivate for tests

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

Index: chrome/test/data/webui/settings/fake_settings_private.js
diff --git a/chrome/test/data/webui/settings/fake_settings_private.js b/chrome/test/data/webui/settings/fake_settings_private.js
new file mode 100644
index 0000000000000000000000000000000000000000..d0c710cb3939f495fabeb96500f231777aaa4ab8
--- /dev/null
+++ b/chrome/test/data/webui/settings/fake_settings_private.js
@@ -0,0 +1,155 @@
+// 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 Fake implementation of chrome.settingsPrivate for testing. */
+cr.define('settings', function() {
+  /**
+   * Creates a deep copy of the object.
+   * @param {!Object} obj
+   * @return {!Object}
+   */
+  function deepCopy(obj) {
+    return JSON.parse(JSON.stringify(obj));

[stevenjb, 2015/11/17 18:24:19]

Any reason we can't use Object.assign here?

[michaelpg, 2015/11/17 22:02:11]

it's not recursive

[stevenjb, 2015/11/17 22:03:55]

Ah, right.

+  }
+
+  /**
+   * Fake of chrome.settingsPrivate API. Override chrome.settingsPrivate by
+   * creating a FakeSettingsPrivate and calling register() on it.
+   * @constructor
+   * @param {Array<!settings.FakeSettingsPrivate.Pref>=} opt_initialPrefs
+   * @extends {chrome.settingsPrivate}
+   */
+  function FakeSettingsPrivate(opt_initialPrefs) {
+    this.prefs = {};
+
+    // Hack alert: bind this instance's onPrefsChanged members to this.
+    this.onPrefsChanged = {
+      addListener: this.onPrefsChanged.addListener.bind(this),
+      removeListener: this.onPrefsChanged.removeListener.bind(this),
+    };
+
+    if (!opt_initialPrefs)
+      return;
+    for (var pref of opt_initialPrefs)
+      this.addPref_(pref.type, pref.key, pref.value);
+  }
+
+  FakeSettingsPrivate.prototype = {
+    // chrome.settingsPrivate overrides.
+    onPrefsChanged: {
+      addListener: function(listener) {
+        this.listener_ = listener;
+      },
+
+      removeListener: function(listener) {
+        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(deepCopy(this.prefs[key]));
+
+      // Run the callback asynchronously to test that the prefs aren't actually
+      // used before they become available.
+      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_) {
+        callback(false);
+        this.failNextSetPref_ = false;
+        return;
+      }
+      assertNotEquals(true, this.disallowSetPref_);
+
+      var changed = JSON.stringify(pref.value) != JSON.stringify(value);
+      pref.value = deepCopy(value);
+      callback(true);
+
+      // Like chrome.settingsPrivate, send a notification when prefs change.
+      if (changed)
+        this.sendPrefChanges([{key: key, value: deepCopy(value)}]);
+    },
+
+    getPref: function(key, callback) {
+      var pref = this.prefs[key];
+      assertNotEquals(undefined, pref);
+      callback(deepCopy(pref));
+    },
+
+    // Functions used by tests.
+
+    /** Replaces chrome.settingsPrivate with FakeSettingsPrivate. */
+    register: function() {
+      this.settingsPrivate_ = chrome.settingsPrivate;
+      chrome.settingsPrivate = this;

[Dan Beam, 2015/11/17 23:02:07]

why is this better?

[michaelpg, 2015/11/17 23:51:51]

The code already uses chrome.settingsPrivate so this means less changing of code
and (to me) is a more elegant way to inject the fake.

[Dan Beam, 2015/11/18 02:01:01]

generally prefer dependency injection to changing globals.  when you change
globals it's very likely to change assumptions that other code relies on.  this
is the reason that modifying global prototypes in JavaScript is discouraged
(i.e. messing with String.prototype, etc.).

i usually don't pipe up because if there's no existing tests anything's better
than nothing.  but in this case you're undoing something that was already
dependency injected.  you can't inject the global into the ctor like typical
dependency injection because you want to use the element declaratively but that
doesn't prevent you from just doing setSettingsPrivate() that changes
this.settingsPrivate_ (or this.settingsApi_).  unless you actually need to use
the settingsPrivate in the ctor, we can use set*() stuff.  if you do need to use
the settingsPrivate in the ctor, change the ctor to do less, then call
setSettingsPrivate(), then initialize() (<-- and move the code that accesses
settingsPrivate to initialize()). 

[michaelpg, 2015/11/18 02:03:44]

OK, that's doable. Would you help me figure out how to make Closure treat
this.settingsApi_ as if it matched the externs of chrome.settingsPrivate?

[Dan Beam, 2015/11/18 02:26:34]

/** @interface */
function SettingsPrivate() {}

SettingsPrivate.prototype = {
  /** Gets a thing. */
  getThing: assertNotReached,
};

/** @implements {SettingsPrivate} */
function RealSettingsPrivate() {}

RealSettingsPrivate.prototype = {
  getThing: function() {
    chrome.settingsPrivate.getThing();
  },
};

/** @implements {SettingsPrivate} */
function FakeSettingsPrivate() {}

FakeSettingsPrivate.prototype = {
  getThing: createFunctionMock(),
};

+    },
+
+    /** Restores chrome.settingsPrivate. */
+    unregister: function() {
+      chrome.settingsPrivate = this.settingsPrivate_;
+    },
+
+    /** Instructs the API to return a failure when setPref is next called. */
+    failNextSetPref: function() {
+      this.failNextSetPref_ = true;
+    },
+
+    /** Instructs the API to assert (fail the test) if setPref is called. */
+    disallowSetPref: function() {
+      this.disallowSetPref_ = true;
+    },
+
+    allowSetPref: function() {
+      this.disallowSetPref_ = false;
+    },
+
+    /**
+     * Notifies the listener of pref changes.
+     * @param {!Object<{key: string, value: *}>} changes
+     */
+    sendPrefChanges: function(changes) {
+      var prefs = [];
+      for (var change of changes) {
+        var pref = this.prefs[change.key];
+        assertNotEquals(undefined, pref);
+        pref.value = change.value;
+        prefs.push(deepCopy(pref));
+      }
+      this.listener_(prefs);
+    },
+
+    // Private methods for use by the fake 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,
+      };
+    },
+  };
+
+  return {FakeSettingsPrivate: FakeSettingsPrivate};
+});
+
+/**
+ * @type {Array<{key: string,
+ *               type: chrome.settingsPrivate.PrefType,
+ *               values: !Array<*>}>}
+ */
+settings.FakeSettingsPrivate.Pref

[Dan Beam, 2015/11/17 23:01:15]

nit: ; at end

[michaelpg, 2015/11/17 23:51:51]

Done.