Improve HostOptions unit tests

Improve HostOptions unit tests

One of the design considerations in https://codereview.chromium.org/1358173005/ was that using base.copyWithoutNullFields made the code future-proof if new options are added. This unit test ensures that that holds true and also improves some of the existing unit tests to catch some obvious errors.

Committed: https://crrev.com/379d57418093504170fd83182011c6f36554b71a
Cr-Commit-Position: refs/heads/master@{#351180}

[Jamie, 2015-09-26 00:40:41]

jamiewalch@chromium.org changed reviewers:
+ kelvinp@chromium.org

[Jamie, 2015-09-26 00:40:41]

ptal

https://codereview.chromium.org/1366093003/diff/20001/remoting/webapp/base/js...
File remoting/webapp/base/js/host_options_unittest.js (right):

https://codereview.chromium.org/1366093003/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/host_options_unittest.js:59: var optionsNew = new
remoting.HostOptions('host-id');
Loading into the same object as we saved would trivially pass if load() was a
no-op, which is not what we want.

https://codereview.chromium.org/1366093003/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/host_options_unittest.js:82: var optionsEmpty = new
remoting.HostOptions('host-id');
This test wasn't really testing anything previously. Now we assert that
non-default values are not reset when loading an object that didn't have an
override set when it was saved.

[kelvinp, 2015-09-28 18:36:41]

lgtm

https://codereview.chromium.org/1366093003/diff/20001/remoting/webapp/base/js...
File remoting/webapp/base/js/host_options_unittest.js (right):

https://codereview.chromium.org/1366093003/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/host_options_unittest.js:59: var optionsNew = new
remoting.HostOptions('host-id');
Nit: optionsLoaded may be a more descriptive name.

https://codereview.chromium.org/1366093003/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/host_options_unittest.js:121:
assert.deepEqual(optionsCopy, base.deepCopy(optionsNew));
Nit: maybe good to assert options['undefined-options] == 42 to more explicitly
call out what the expected behavior is.

[Jamie, 2015-09-28 21:33:31]

The CQ bit was checked by jamiewalch@chromium.org

[Jamie, 2015-09-28 21:33:31]

The patchset sent to the CQ was uploaded after l-g-t-m from kelvinp@chromium.org
Link to the patchset: https://codereview.chromium.org/1366093003/#ps40001
(title: "Reviewer feedback.")

[Jamie, 2015-09-28 21:33:48]

FYI

https://codereview.chromium.org/1366093003/diff/20001/remoting/webapp/base/js...
File remoting/webapp/base/js/host_options_unittest.js (right):

https://codereview.chromium.org/1366093003/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/host_options_unittest.js:59: var optionsNew = new
remoting.HostOptions('host-id');
On 2015/09/28 18:36:40, kelvinp wrote:
> Nit: optionsLoaded may be a more descriptive name. 

Done.

https://codereview.chromium.org/1366093003/diff/20001/remoting/webapp/base/js...
remoting/webapp/base/js/host_options_unittest.js:121:
assert.deepEqual(optionsCopy, base.deepCopy(optionsNew));
On 2015/09/28 18:36:40, kelvinp wrote:
> Nit: maybe good to assert options['undefined-options] == 42 to more explicitly
> call out what the expected behavior is.

Done.

[commit-bot: I haz the power, 2015-09-28 21:34:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1366093003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1366093003/40001

[commit-bot: I haz the power, 2015-09-28 22:42:34]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-09-28 22:43:46]

Patchset 3 (id:??) landed as
https://crrev.com/379d57418093504170fd83182011c6f36554b71a
Cr-Commit-Position: refs/heads/master@{#351180}