Improved settings cache unit tests

- settings cache was appearing as untested because we use rewire!
- rewire was totally unnecessary in this case, so I removed it
- updated to 100% test coverage whilst there, including removing one unreachable branch
- commented some undesirable behaviour I found whilst trying to reach all branches
- I don't want to change the behaviour to return false correctly without having a reason beyond improving coverage

core/shared/settings-cache/cache.js

@@ -28,7 +28,7 @@ const doGet = (key, options) => {
 
     // Don't try to resolve to the value of the setting
     if (options && options.resolve === false) {
-        return settingsCache[key] || null;
+        return settingsCache[key];
     }
 
     // Default behaviour is to try to resolve the value and return that

test/unit/shared/settings-cache.test.js

@@ -5,31 +5,31 @@ const _ = require('lodash');
 const events = require('../../../core/server/lib/common/events');
 
 // Testing  the Private API
-let cache = rewire('../../../core/shared/settings-cache/cache');
+let cache = require('../../../core/shared/settings-cache/cache');
 const publicSettings = require('../../../core/shared/settings-cache/public');
 
 should.equal(true, true);
 
 describe('UNIT: settings cache', function () {
     beforeEach(function () {
-        cache = rewire('../../../core/shared/settings-cache/cache');
+        cache.init(events);
     });
 
     afterEach(function () {
         sinon.restore();
     });
 
-    it('does not auto convert string into number', function () {
+    it('.get() does not auto convert string into number', function () {
         cache.set('key1', {value: '1'});
         (typeof cache.get('key1')).should.eql('string');
     });
 
-    it('does not auto convert string into number: float', function () {
+    it('.get() oes not auto convert string into number: float', function () {
         cache.set('key1', {value: '1.4'});
         (typeof cache.get('key1')).should.eql('string');
     });
 
-    it('stringified JSON get\'s parsed', function () {
+    it('.get() parses stringified JSON', function () {
         cache.set('key2', {value: '{"a":"1","b":"hallo","c":{"d":[]},"e":2}'});
         (typeof cache.get('key2')).should.eql('object');
         cache.get('key2').a.should.eql('1');
@@ -38,13 +38,62 @@ describe('UNIT: settings cache', function () {
         cache.get('key2').e.should.eql(2);
     });
 
-    it('can get all values', function () {
+    it('.get() respects the resolve option', function () {
+        cache.set('foo', {value: 'bar'});
+        cache.get('foo', {resolve: false}).should.be.an.Object().with.property('value', 'bar');
+        cache.get('foo', {resolve: true}).should.be.a.String().and.eql('bar');
+    });
+
+    it('.get() can handle miscellaneous values', function () {
+        // THis value is not set
+        should(cache.get('bar')).be.undefined();
+
+        // Using set with a string instead of an object
+        cache.set('foo', 'bar');
+        should(cache.get('foo')).eql(null);
+
+        // Various built-in values
+        cache.set('null', {value: null});
+        cache.set('nan', {value: NaN});
+
+        cache.set('true', {value: true});
+        cache.set('false', {value: false});
+        cache.set('object', {value: {}});
+        cache.set('array', {value: []});
+
+        should(cache.get('null')).eql(null);
+        should(cache.get('nan')).eql(null);
+
+        should(cache.get('true')).eql(true);
+        should(cache.get('false')).eql(null); // EEEK! this is falsy but should be false
+        should(cache.get('object')).eql({});
+        should(cache.get('array')).eql([]);
+
+        // Built-ins as strings
+        cache.set('empty', {value: ''});
+        cache.set('stringnull', {value: 'null'});
+        cache.set('stringnan', {value: 'NaN'});
+        cache.set('stringtrue', {value: 'true'});
+        cache.set('stringfalse', {value: 'false'});
+        cache.set('stringobj', {value: '{}'});
+        cache.set('stringarr', {value: '[]'});
+
+        should(cache.get('empty')).eql(null);
+        should(cache.get('stringnull')).eql(null);
+        should(cache.get('stringnan')).eql('NaN');
+        should(cache.get('stringtrue')).eql(true);
+        should(cache.get('stringfalse')).eql(null); // EEEK! this is falsy but should be false
+        should(cache.get('stringobj')).eql({});
+        should(cache.get('stringarr')).eql([]);
+    });
+
+    it('.getAll() returns all values', function () {
         cache.set('key1', {value: '1'});
         cache.get('key1').should.eql('1');
         cache.getAll().should.eql({key1: {value: '1'}});
     });
 
-    it('correctly filters and formats public values', function () {
+    it('.getPublic() correctly filters and formats public values', function () {
         cache.set('key1', {value: 'something'});
         cache.set('title', {value: 'hello world'});
         cache.set('timezone', {value: 'PST'});
@@ -62,7 +111,7 @@ describe('UNIT: settings cache', function () {
         cache.getPublic().should.eql(values);
     });
 
-    it('can reset and init without double handling of events', function () {
+    it('.reset() and .init() do not double up events', function () {
         const setSpy = sinon.spy(cache, 'set');
 
         const settingsCollection = {