Added partial unit tests for host_controller.js. More to come.

remoting/webapp/crd/js/host_controller_unittest.js

Index: remoting/webapp/crd/js/host_controller_unittest.js
diff --git a/remoting/webapp/crd/js/host_controller_unittest.js b/remoting/webapp/crd/js/host_controller_unittest.js
new file mode 100644
index 0000000000000000000000000000000000000000..cda53ba005641f04dbd3f5a8e73e064fbe0c8571
--- /dev/null
+++ b/remoting/webapp/crd/js/host_controller_unittest.js
@@ -0,0 +1,413 @@
+/**
+ * @fileoverview
+ * Unit tests for host_controller.js.
+ */
+
+(function() {
+
+'use strict';
+
+/** @type {sinon.Mock} */
+var hostListMock = null;
+
+/** @type {sinon.TestStub} */
+var generateUuidStub;
+
+/** @type {remoting.MockHostDaemonFacade} */
+var mockHostDaemonFacade;
+
+/** @type {sinon.TestStub} */
+var hostDaemonFacadeCtorStub;
+
+var FAKE_HOST_PIN = '<FAKE_HOST_PIN>';
+var FAKE_USER_EMAIL = '<FAKE_USER_EMAIL>';
+var FAKE_USER_NAME = '<FAKE_USER_NAME>';
+var FAKE_UUID = '0bad0bad-0bad-0bad-0bad-0bad0bad0bad';
+var FAKE_DAEMON_VERSION = '1.2.3.4';
+var FAKE_HOST_NAME = '<FAKE_HOST_NAME>';
+var FAKE_PUBLIC_KEY = '<FAKE_PUBLIC_KEY>';
+var FAKE_PRIVATE_KEY = '<FAKE_PRIVATE_KEY>';
+var FAKE_AUTH_CODE = '<FAKE_AUTH_CODE>';
+var FAKE_REFRESH_TOKEN = '<FAKE_REFRESH_TOKEN>';
+var FAKE_PIN_HASH = '<FAKE_PIN_HASH>';
+var FAKE_HOST_CLIENT_ID = '<FAKE_HOST_CLIENT_ID>';
+var FAKE_CLIENT_BASE_JID = '<FAKE_CLIENT_BASE_JID>';
+
+/** @type {sinon.Spy|Function} */
+var getCredentialsFromAuthCodeSpy;
+
+/** @type {sinon.Spy|Function} */
+var getPinHashSpy;
+
+/** @type {sinon.Spy|Function} */
+var startDaemonSpy;
+
+/** @type {sinon.Spy} */
+var unregisterHostByIdSpy;
+
+/** @type {sinon.Spy} */
+var onLocalHostStartedSpy;
+
+QUnit.module('host_controller', {
+  beforeEach: function(/** QUnit.Assert */ assert) {
+    chromeMocks.activate(['identity', 'runtime']);
+    chromeMocks.identity.mock$setToken('my_token');
+    remoting.identity = new remoting.Identity();
+    remoting.MockXhr.activate();
+    base.debug.assert(remoting.oauth2 === null);
+    remoting.oauth2 = new remoting.OAuth2();
+    base.debug.assert(remoting.hostList === null);
+    remoting.hostList = /** @type {remoting.HostList} */
+        (Object.create(remoting.HostList.prototype));
+
+    // When the HostList's unregisterHostById method is called, make
+    // sure the argument is correct.
+    unregisterHostByIdSpy =
+        sinon.stub(remoting.hostList, 'unregisterHostById', function(
+            /** string */ hostId) {
+          assert.equal(hostId, FAKE_UUID);
+        });
+
+    // When the HostList's onLocalHostStarted method is called, make
+    // sure the arguments are correct.
+    onLocalHostStartedSpy =
+        sinon.stub(
+            remoting.hostList, 'onLocalHostStarted', function(
+                /** string */ hostName,
+                /** string */ newHostId,
+                /** string */ publicKey) {
+              assert.equal(hostName, FAKE_HOST_NAME);
+              assert.equal(newHostId, FAKE_UUID);
+              assert.equal(publicKey, FAKE_PUBLIC_KEY);
+            });
+
+    hostDaemonFacadeCtorStub = sinon.stub(remoting, 'HostDaemonFacade');
+    mockHostDaemonFacade = new remoting.MockHostDaemonFacade();
+    hostDaemonFacadeCtorStub.returns(mockHostDaemonFacade);
+    generateUuidStub = sinon.stub(base, 'generateUuid');
+    generateUuidStub.returns(FAKE_UUID);
+    getCredentialsFromAuthCodeSpy = sinon.spy(
+        mockHostDaemonFacade, 'getCredentialsFromAuthCode');
+    getPinHashSpy = sinon.spy(mockHostDaemonFacade, 'getPinHash');
+    startDaemonSpy = sinon.spy(mockHostDaemonFacade, 'startDaemon');
+
+    // Set up successful responses from mockHostDaemonFacade.
+    // Individual tests override these values to create errors.
+    mockHostDaemonFacade.features =
+        [remoting.HostController.Feature.OAUTH_CLIENT];
+    mockHostDaemonFacade.daemonVersion = FAKE_DAEMON_VERSION;
+    mockHostDaemonFacade.hostName = FAKE_HOST_NAME;
+    mockHostDaemonFacade.privateKey = FAKE_PRIVATE_KEY;
+    mockHostDaemonFacade.publicKey = FAKE_PUBLIC_KEY;
+    mockHostDaemonFacade.hostClientId = FAKE_HOST_CLIENT_ID;
+    mockHostDaemonFacade.userEmail = FAKE_USER_EMAIL;
+    mockHostDaemonFacade.refreshToken = FAKE_REFRESH_TOKEN;
+    mockHostDaemonFacade.pinHash = FAKE_PIN_HASH;
+    mockHostDaemonFacade.startDaemonResult =
+        remoting.HostController.AsyncResult.OK;
+
+    sinon.stub(remoting.identity, 'getEmail').returns(
+        Promise.resolve(FAKE_USER_EMAIL));
+    sinon.stub(remoting.oauth2, 'getRefreshToken').returns(
+        FAKE_REFRESH_TOKEN);
+  },
+
+  afterEach: function() {
+    getCredentialsFromAuthCodeSpy.restore();
+    generateUuidStub.restore();
+    hostDaemonFacadeCtorStub.restore();
+    remoting.hostList = null;
+    remoting.oauth2 = null;
+    remoting.MockXhr.restore();
+    chromeMocks.restore();
+    remoting.identity = null;
+  }
+});
+
+/**
+ * Install

[Jamie, 2015/04/08 00:16:15]

The comment doesn't seem to have any relation to the function name.

[John Williams, 2015/04/08 20:20:09]

Must've typed errant ^K while I was thinking about something else.

+ * @param {QUnit.Assert} assert
+ * @param {boolean} withAuthCode
+ */
+function queueRegistryResponse(assert, withAuthCode) {
+  var responseJson = {
+      data: {
+        authorizationCode: FAKE_AUTH_CODE
+      }
+  };
+  if (!withAuthCode) {
+    delete responseJson.data.authorizationCode;
+  }
+
+  remoting.MockXhr.setResponseFor(
+      'POST', 'DIRECTORY_API_BASE_URL/@me/hosts',
+      function(/** remoting.MockXhr */ xhr) {
+        assert.deepEqual(
+            xhr.params.jsonContent,
+            { data: {
+              hostId: FAKE_UUID,
+              hostName: FAKE_HOST_NAME,
+              publicKey: FAKE_PUBLIC_KEY
+            } });
+        xhr.setTextResponse(200, JSON.stringify(responseJson));
+      });
+}
+
+/**
+ * @param {remoting.HostController} controller
+ */
+function mockGetClientBaseJid(controller) {
+  sinon.stub(controller, 'getClientBaseJid_').
+      callsArgWith(0, FAKE_CLIENT_BASE_JID);
+};
+
+// Check that hasFeature returns false for missing features.
+QUnit.test('hasFeature returns false', function(assert) {
+  var controller = new remoting.HostController();
+  return new Promise(function(resolve, reject) {
+    controller.hasFeature(
+        remoting.HostController.Feature.PAIRING_REGISTRY,
+        resolve);
+  }).then(function(/** boolean */ result) {
+    assert.equal(result, false);
+  });
+});
+
+// Check that hasFeature returns true for supported features.
+QUnit.test('hasFeature returns true', function(assert) {
+  var controller = new remoting.HostController();
+  return new Promise(function(resolve, reject) {
+    controller.hasFeature(
+        remoting.HostController.Feature.OAUTH_CLIENT,
+        resolve);
+  }).then(function(/** boolean */ result) {
+    assert.equal(result, true);
+  });
+});
+
+// Check what happens when the HostDaemonFacade's getHostName method
+// fails.
+QUnit.test('start with getHostName failure', function(assert) {
+  mockHostDaemonFacade.hostName = null;
+  var controller = new remoting.HostController();

[Jamie, 2015/04/08 00:16:14]

Is the code from here on common to all tests? Can it be factored out?

[John Williams, 2015/04/08 20:20:09]

Done.

[Jamie, 2015/04/08 22:29:29]

IIRC, the controller used to be global and you opted to make it local in an
earlier revision. FWIW, I prefer it global, but my comment was actually
referring to the code following this line that starts the controller and asserts
various things, and which seems to be largely copy/pasted between functions.

[John Williams, 2015/04/08 23:19:55]

Right, I did.  I misunderstood the comment, but I think having it be global is
probably the right way to do now that I've added the mockGetClientBaseJid
function...OTOH, that function is going away in my next CL, so I guess I'll make
it local again in that CL :-/

Anyway, the assertions are such that they're the same in a lot of test cases,
but none of them are the same in *every* case; I think I've moved everything I
can into the beforeEach function.  The failure test cases just happen to be very
similar to each other.

It's possible to factor out some more common functionality into helper
functions, but I think you quickly hit the point of diminishing returns when you
do that in unit tests.  IMHO that what's desirable in test code is often exactly
the opposite of what's desirable in production code.  In this case, I think some
copypasta is OK because being obvious is more important than being concise or
future-proof.

[Jamie, 2015/04/08 23:28:42]

Acknowledged.

+  return new Promise(function(resolve, reject) {
+    controller.start(FAKE_HOST_PIN, true, function() {
+      reject('test failed');
+    }, function(/** remoting.Error */ e) {
+      assert.equal(e.getDetail(), 'getHostName');
+      assert.equal(getCredentialsFromAuthCodeSpy.callCount, 0);

[Jamie, 2015/04/08 00:16:14]

Should it be a test failure if this was called? We definitely want to assert
that startDaemon is not called, but be careful that you're not verifying the
precise order in which the various attributes are queried (which we should be
free to change without breaking tests).

[John Williams, 2015/04/08 20:20:09]

Good point. Changed.

+      assert.equal(unregisterHostByIdSpy.callCount, 0);
+      assert.equal(onLocalHostStartedSpy.callCount, 0);
+      assert.equal(startDaemonSpy.callCount, 0);
+      resolve(null);
+    });
+  });
+});
+
+// Check what happens when the HostDaemonFacade's generateKeyPair
+// method fails.
+QUnit.test('start with generateKeyPair failure', function(assert) {
+  mockHostDaemonFacade.publicKey = null;
+  mockHostDaemonFacade.privateKey = null;
+  var controller = new remoting.HostController();
+  return new Promise(function(resolve, reject) {
+    controller.start(FAKE_HOST_PIN, true, function() {
+      reject('test failed');
+    }, function(/** remoting.Error */ e) {
+      assert.equal(e.getDetail(), 'generateKeyPair');
+      assert.equal(getCredentialsFromAuthCodeSpy.callCount, 0);
+      assert.equal(unregisterHostByIdSpy.callCount, 0);
+      assert.equal(onLocalHostStartedSpy.callCount, 0);
+      assert.equal(startDaemonSpy.callCount, 0);
+      resolve(null);
+    });
+  });
+});
+
+// Check what happens when the HostDaemonFacade's getHostClientId
+// method fails.
+QUnit.test('start with getHostClientId failure', function(assert) {
+  var controller = new remoting.HostController();
+  mockHostDaemonFacade.hostClientId = null;
+  return new Promise(function(resolve, reject) {
+    controller.start(FAKE_HOST_PIN, true, function() {
+      reject('test failed');
+    }, function(/** remoting.Error */ e) {
+      assert.equal(e.getDetail(), 'getHostClientId');
+      assert.equal(getCredentialsFromAuthCodeSpy.callCount, 0);
+      assert.equal(unregisterHostByIdSpy.callCount, 0);
+      assert.equal(onLocalHostStartedSpy.callCount, 0);
+      assert.equal(startDaemonSpy.callCount, 0);
+      resolve(null);
+    });
+  });
+});
+
+// Check what happens when the registry returns an HTTP when we try to
+// register a host.
+QUnit.test('start with host registration failure', function(assert) {
+  var controller = new remoting.HostController();
+  remoting.MockXhr.setEmptyResponseFor(
+      'POST', 'DIRECTORY_API_BASE_URL/@me/hosts', 500);
+  return new Promise(function(resolve, reject) {
+    controller.start(FAKE_HOST_PIN, true, function() {
+      reject('test failed');
+    }, function(/** remoting.Error */ e) {
+      assert.equal(e.getTag(), remoting.Error.Tag.REGISTRATION_FAILED);
+      assert.equal(getCredentialsFromAuthCodeSpy.callCount, 0);
+      assert.equal(unregisterHostByIdSpy.callCount, 0);
+      assert.equal(onLocalHostStartedSpy.callCount, 0);
+      assert.equal(startDaemonSpy.callCount, 0);
+      resolve(null);
+    });
+  });
+});
+
+// Check what happens when the HostDaemonFacade's
+// getCredentialsFromAuthCode method fails.
+QUnit.test('start with getCredentialsFromAuthCode failure', function(assert) {
+  var controller = new remoting.HostController();
+  mockHostDaemonFacade.useEmail = null;
+  mockHostDaemonFacade.refreshToken = null;
+  queueRegistryResponse(assert, true);
+  return new Promise(function(resolve, reject) {
+    controller.start(FAKE_HOST_PIN, true, function() {
+      reject('test failed');
+    }, function(/** remoting.Error */ e) {
+      assert.equal(e.getDetail(), 'getCredentialsFromAuthCode');
+      assert.equal(getCredentialsFromAuthCodeSpy.callCount, 1);
+      assert.equal(unregisterHostByIdSpy.callCount, 0);
+      assert.equal(onLocalHostStartedSpy.callCount, 0);
+      assert.equal(startDaemonSpy.callCount, 0);
+      resolve(null);
+    });
+  });
+});
+
+// Check what happens when the HostDaemonFacade's getPinHash method
+// fails, and verify that getPinHash is called when the registry
+// does't return an auth code.
+QUnit.test('start with getRefreshToken+getPinHash failure', function(assert) {
+  var controller = new remoting.HostController();
+  mockHostDaemonFacade.pinHash = null;
+  queueRegistryResponse(assert, false);
+  return new Promise(function(resolve, reject) {
+    controller.start(FAKE_HOST_PIN, true, function() {
+      reject('test failed');
+    }, function(/** remoting.Error */ e) {
+      assert.equal(e.getDetail(), 'getPinHash');
+      assert.equal(getCredentialsFromAuthCodeSpy.callCount, 0);
+      assert.equal(unregisterHostByIdSpy.callCount, 0);
+      assert.equal(onLocalHostStartedSpy.callCount, 0);
+      assert.equal(startDaemonSpy.callCount, 0);
+      resolve(null);
+    });
+  });
+});
+
+// Check what happens when the HostController's getClientBaseJid_
+// method fails.
+QUnit.test('start with getClientBaseJid_ failure', function(assert) {
+  var controller = new remoting.HostController();
+  queueRegistryResponse(assert, true);
+  sinon.stub(controller, 'getClientBaseJid_').
+      callsArgWith(1, remoting.Error.unexpected('getClientBaseJid_'));
+  return new Promise(function(resolve, reject) {
+    controller.start(FAKE_HOST_PIN, true, function() {
+      reject('test failed');
+    }, function(/** remoting.Error */ e) {
+      assert.equal(e.getDetail(), 'getClientBaseJid_');
+      assert.equal(unregisterHostByIdSpy.callCount, 1);
+      resolve(null);
+    });
+  });
+});
+
+// Check what happens when the HostDaemonFacade's startDaemon method
+// fails and calls its onError argument.
+QUnit.test('start with startDaemon failure', function(assert) {
+  var controller = new remoting.HostController();
+  queueRegistryResponse(assert, true);
+  mockGetClientBaseJid(controller);
+  mockHostDaemonFacade.startDaemonResult = null;

[Jamie, 2015/04/08 00:16:14]

Why not FAILED/FAILED_DIRECTORY?

[John Williams, 2015/04/08 20:20:09]

Added this as a separate case, but there are two distinct ways this method can
"fail".  Either it calls onDone with a failure code, or it calls onError.

[Jamie, 2015/04/08 22:29:29]

Yuck. That sounds like something worth cleaning up as part of your refactoring
(ie, not this CL), if it's not too much additional work.

[John Williams, 2015/04/08 23:19:55]

SGTM.  When I work on legacy code I usually end up itching to do various
refactorings, so I'm happy to do it, provided it doesn't adversely affect me
come perf time.

[Jamie, 2015/04/08 23:28:42]

Acknowledged.

+  return new Promise(function(resolve, reject) {
+    controller.start(FAKE_HOST_PIN, true, function() {
+      reject('test failed');
+    }, function(/** remoting.Error */ e) {
+      assert.equal(e.getDetail(), 'startDaemon');
+      assert.equal(unregisterHostByIdSpy.callCount, 1);
+      assert.equal(onLocalHostStartedSpy.callCount, 0);
+      resolve(null);
+    });
+  });
+});
+
+// Check what happens when the HostDaemonFacade's startDaemon method
+// calls is onDone method with an async error code.
+QUnit.test('start with startDaemon cancelled', function(assert) {
+  var controller = new remoting.HostController();
+  queueRegistryResponse(assert, true);
+  mockGetClientBaseJid(controller);
+  mockHostDaemonFacade.startDaemonResult =
+      remoting.HostController.AsyncResult.CANCELLED;
+  return new Promise(function(resolve, reject) {
+    controller.start(FAKE_HOST_PIN, true, function() {
+      reject('test failed');
+    }, function(/** remoting.Error */ e) {
+      assert.equal(e.getTag(), remoting.Error.Tag.CANCELLED);
+      assert.equal(unregisterHostByIdSpy.callCount, 1);
+      assert.equal(onLocalHostStartedSpy.callCount, 0);
+      resolve(null);
+    });
+  });
+});
+
+// Check what happens when the entire host registration process
+// succeeds.
+[false, true].forEach(function(/** boolean */ consent) {
+  QUnit.test('start succeeds with consent=' + consent, function(assert) {
+    var controller = new remoting.HostController();
+    queueRegistryResponse(assert, true);
+    mockGetClientBaseJid(controller);
+    return new Promise(function(resolve, reject) {
+      controller.start(FAKE_HOST_PIN, consent, function() {
+        assert.equal(getCredentialsFromAuthCodeSpy.callCount, 1);
+        assert.deepEqual(
+            getCredentialsFromAuthCodeSpy.args[0][0],
+            FAKE_AUTH_CODE);
+        assert.equal(getPinHashSpy.callCount, 1);
+        assert.deepEqual(
+            getPinHashSpy.args[0].slice(0, 2),
+            [FAKE_UUID, FAKE_HOST_PIN]);
+
+        assert.equal(unregisterHostByIdSpy.callCount, 0);
+        assert.equal(onLocalHostStartedSpy.callCount, 1);
+        assert.equal(startDaemonSpy.callCount, 1);
+        assert.deepEqual(
+            startDaemonSpy.args[0].slice(0, 2),
+            [{
+              xmpp_login: FAKE_USER_EMAIL,
+              oauth_refresh_token: FAKE_REFRESH_TOKEN,
+              host_owner: FAKE_CLIENT_BASE_JID,
+              host_owner_email: FAKE_USER_EMAIL,
+              host_id: FAKE_UUID,
+              host_name: FAKE_HOST_NAME,
+              host_secret_hash: FAKE_PIN_HASH,
+              private_key: FAKE_PRIVATE_KEY
+            }, consent]);
+        resolve(null);
+      }, reject);
+    });
+  });
+});
+
+// TODO(jrw): Add tests for |stop| method.
+// TODO(jrw): Add tests for |updatePin| method.
+// TODO(jrw): Add tests for |getLocalHostState| method.
+// TODO(jrw): Add tests for |getLocalHostId| method.
+// TODO(jrw): Add tests for |getPairedClients| method.
+// TODO(jrw): Add tests for |deletePairedClient| method.
+// TODO(jrw): Add tests for |clearPairedClients| method.
+// TODO(jrw): Make sure getClientBaseJid_ method is tested.
+
+})();