Merge pull request #406 from hypothesis/add-tests-for-shared-settings

Tests and other tweaks to shared/settings.js

src/annotator/config.js

@@ -18,7 +18,7 @@ function configFrom(window_) {
 
   // Parse config from `<script class="js-hypothesis-config">` tags
   try {
-    Object.assign(config, settings(window_.document));
+    Object.assign(config, settings.jsonConfigsFrom(window_.document));
   } catch (err) {
     console.warn('Could not parse settings from js-hypothesis-config tags',
       err);

src/annotator/test/config-test.js

@@ -2,7 +2,9 @@
 
 var proxyquire = require('proxyquire');
 
-var fakeSettings = sinon.stub();
+var fakeSettings = {
+  jsonConfigsFrom: sinon.stub(),
+};
 var fakeExtractAnnotationQuery = {};
 
 var configFrom = proxyquire('../config', {
@@ -26,8 +28,8 @@ describe('annotator.config', function() {
   });
 
   beforeEach('reset fakeSettings', function() {
-    fakeSettings.reset();
-    fakeSettings.returns({});
+    fakeSettings.jsonConfigsFrom.reset();
+    fakeSettings.jsonConfigsFrom.returns({});
   });
 
   beforeEach('reset fakeExtractAnnotationQuery', function() {
@@ -71,15 +73,16 @@ describe('annotator.config', function() {
 
     configFrom(window_);
 
-    assert.calledOnce(fakeSettings);
-    assert.calledWithExactly(fakeSettings, window_.document);
+    assert.calledOnce(fakeSettings.jsonConfigsFrom);
+    assert.calledWithExactly(
+      fakeSettings.jsonConfigsFrom, window_.document);
   });
 
-  context('when settings() returns a non-empty object', function() {
+  context('when jsonConfigsFrom() returns a non-empty object', function() {
     it('reads the setting into the returned config', function() {
-      // configFrom() just blindly adds any key: value settings that settings()
-      // returns into the returned config object.
-      fakeSettings.returns({foo: 'bar'});
+      // configFrom() just blindly adds any key: value settings that
+      // jsonConfigsFrom() returns into the returns options object.
+      fakeSettings.jsonConfigsFrom.returns({foo: 'bar'});
 
       var config = configFrom(fakeWindow());
 
@@ -87,9 +90,9 @@ describe('annotator.config', function() {
     });
   });
 
-  context('when settings() throws an error', function() {
+  context('when jsonConfigsFrom() throws an error', function() {
     beforeEach(function() {
-      fakeSettings.throws();
+      fakeSettings.jsonConfigsFrom.throws();
     });
 
     it('catches the error', function() {
@@ -117,7 +120,9 @@ describe('annotator.config', function() {
       var window_ = fakeWindow();
       window_.hypothesisConfig = sinon.stub().returns({
         foo: 'fooFromHypothesisConfigFunc'});
-      fakeSettings.returns({foo: 'fooFromJSHypothesisConfigObj'});
+      fakeSettings.jsonConfigsFrom.returns({
+        foo: 'fooFromJSHypothesisConfigObj',
+      });
 
       var config = configFrom(window_);
 
@@ -170,7 +175,7 @@ describe('annotator.config', function() {
       },
     ].forEach(function(test) {
       it(test.name, function() {
-        fakeSettings.returns({showHighlights: test.in});
+        fakeSettings.jsonConfigsFrom.returns({showHighlights: test.in});
 
         var config = configFrom(fakeWindow());
 
@@ -200,12 +205,12 @@ describe('annotator.config', function() {
 
     specify('settings from extractAnnotationQuery override others', function() {
       // Settings returned by extractAnnotationQuery() override ones from
-      // settings() or from window.hypothesisConfig().
+      // jsonConfigsFrom() or from window.hypothesisConfig().
       var window_ = fakeWindow();
       fakeExtractAnnotationQuery.extractAnnotationQuery.returns({
         foo: 'fromExtractAnnotationQuery',
       });
-      fakeSettings.returns({foo: 'fromSettings'});
+      fakeSettings.jsonConfigsFrom.returns({foo: 'fromSettings'});
       window_.hypothesisConfig = sinon.stub().returns({
         foo: 'fromHypothesisConfig',
       });

src/boot/index.js

@@ -12,7 +12,7 @@
 /* global __MANIFEST__ */
 
 var boot = require('./boot');
-var settings = require('../shared/settings')(document);
+var settings = require('../shared/settings').jsonConfigsFrom(document);
 
 boot(document, {
   assetRoot: settings.assetRoot || '__ASSET_ROOT__',

src/shared/settings.js

@@ -12,17 +12,22 @@ function assign(dest, src) {
 }
 
 /**
- * Return application configuration information from the host page.
+ * Return a parsed `js-hypothesis-config` object from the document, or `{}`.
  *
- * Exposes shared application settings, read from script tags with the
- * class `js-hypothesis-config` which contain JSON content.
+ * Find all `<script class="js-hypothesis-config">` tags in the given document,
+ * parse them as JSON, and return the parsed object.
  *
- * If there are multiple such tags, the configuration from each is merged.
+ * If there are no `js-hypothesis-config` tags in the document then return
+ * `{}`.
  *
- * @param {Document|Element} document - The root element to search for
- *                                      <script> settings tags.
+ * If there are multiple `js-hypothesis-config` tags in the document then merge
+ * them into a single returned object (when multiple scripts contain the same
+ * setting names, scripts further down in the document override those further
+ * up).
+ *
+ * @param {Document|Element} document - The root element to search.
  */
-function settings(document) {
+function jsonConfigsFrom(document) {
   var settingsElements =
     document.querySelectorAll('script.js-hypothesis-config');
 
@@ -33,4 +38,6 @@ function settings(document) {
   return config;
 }
 
-module.exports = settings;
+module.exports = {
+  jsonConfigsFrom: jsonConfigsFrom,
+};

src/shared/test/settings-test.js

@@ -2,33 +2,128 @@
 
 var settings = require('../settings');
 
-function createConfigElement(obj) {
-  var el = document.createElement('script');
-  el.type = 'application/json';
-  el.textContent = JSON.stringify(obj);
-  el.classList.add('js-hypothesis-config');
-  el.classList.add('js-settings-test');
-  return el;
-}
-
-function removeJSONScriptTags() {
-  var elements = document.querySelectorAll('.js-settings-test');
-  for (var i=0; i < elements.length; i++) {
-    elements[i].parentNode.removeChild(elements[i]);
-  }
-}
-
 describe('settings', function () {
-  afterEach(removeJSONScriptTags);
+  describe('#jsonConfigsFrom', function() {
+    var jsonConfigsFrom = settings.jsonConfigsFrom;
 
-  it('reads config from .js-hypothesis-config <script> tags', function () {
-    document.body.appendChild(createConfigElement({key:'value'}));
-    assert.deepEqual(settings(document), {key:'value'});
-  });
+    function appendJSHypothesisConfig(document_, jsonString) {
+      var el = document_.createElement('script');
+      el.type = 'application/json';
+      el.textContent = jsonString;
+      el.classList.add('js-hypothesis-config');
+      el.classList.add('js-settings-test');
+      document_.body.appendChild(el);
+    }
+
+    afterEach('remove js-hypothesis-config tags', function() {
+      var elements = document.querySelectorAll('.js-settings-test');
+      for (var i=0; i < elements.length; i++) {
+        elements[i].remove();
+      }
+    });
+
+    context('when there are no JSON scripts', function() {
+      it('returns {}', function() {
+        assert.deepEqual(jsonConfigsFrom(document), {});
+      });
+    });
+
+    context("when there's JSON scripts with no top-level objects", function() {
+      beforeEach('add JSON scripts with no top-level objects', function() {
+        appendJSHypothesisConfig(document, 'null');
+        appendJSHypothesisConfig(document, '23');
+        appendJSHypothesisConfig(document, 'true');
+      });
+
+      it('ignores them', function() {
+        assert.deepEqual(jsonConfigsFrom(document), {});
+      });
+    });
+
+    context("when there's a JSON script with a top-level array", function() {
+      beforeEach('add a JSON script containing a top-level array', function() {
+        appendJSHypothesisConfig(document, '["a", "b", "c"]');
+      });
+
+      it('returns the array, parsed into an object', function() {
+        assert.deepEqual(
+          jsonConfigsFrom(document),
+          {0: 'a', 1: 'b', 2: 'c'}
+        );
+      });
+    });
+
+    context("when there's a JSON script with a top-level string", function() {
+      beforeEach('add a JSON script with a top-level string', function() {
+        appendJSHypothesisConfig(document, '"hi"');
+      });
+
+      it('returns the string, parsed into an object', function() {
+        assert.deepEqual(jsonConfigsFrom(document), {0: 'h', 1: 'i'});
+      });
+    });
+
+    context("when there's a JSON script containing invalid JSON", function() {
+      beforeEach('add a JSON script containing invalid JSON', function() {
+        appendJSHypothesisConfig(document, 'this is not valid json');
+      });
+
+      it('throws a SyntaxError', function() {
+        assert.throws(
+          function() { jsonConfigsFrom(document); },
+          SyntaxError
+        );
+      });
+    });
+
+    context("when there's a JSON script with an empty object", function() {
+      beforeEach('add a JSON script containing an empty object', function() {
+        appendJSHypothesisConfig(document, '{}');
+      });
+
+      it('ignores it', function() {
+        assert.deepEqual(jsonConfigsFrom(document), {});
+      });
+    });
+
+    context("when there's a JSON script containing some settings", function() {
+      beforeEach('add a JSON script containing some settings', function() {
+        appendJSHypothesisConfig(document, '{"foo": "FOO", "bar": "BAR"}');
+      });
+
+      it('returns the settings', function() {
+        assert.deepEqual(
+          jsonConfigsFrom(document),
+          {foo: 'FOO', bar: 'BAR'}
+        );
+      });
+    });
+
+    context('when there are JSON scripts with different settings', function() {
+      beforeEach('add some JSON scripts with different settings', function() {
+        appendJSHypothesisConfig(document, '{"foo": "FOO"}');
+        appendJSHypothesisConfig(document, '{"bar": "BAR"}');
+        appendJSHypothesisConfig(document, '{"gar": "GAR"}');
+      });
+
+      it('merges them all into one returned object', function() {
+        assert.deepEqual(
+          jsonConfigsFrom(document),
+          {foo: 'FOO', bar: 'BAR', gar: 'GAR'}
+        );
+      });
+    });
+
+    context('when multiple JSON scripts contain the same setting', function() {
+      beforeEach('add some JSON scripts with different settings', function() {
+        appendJSHypothesisConfig(document, '{"foo": "first"}');
+        appendJSHypothesisConfig(document, '{"foo": "second"}');
+        appendJSHypothesisConfig(document, '{"foo": "third"}');
+      });
 
-  it('merges settings from all config <script> tags', function () {
-    document.body.appendChild(createConfigElement({a: 1}));
-    document.body.appendChild(createConfigElement({b: 2}));
-    assert.deepEqual(settings(document), {a: 1, b: 2});
+      specify('settings from later in the page override ones from earlier', function() {
+        assert.equal(jsonConfigsFrom(document).foo, 'third');
+      });
+    });
   });
 });

src/sidebar/app.js

@@ -7,7 +7,7 @@ require('../shared/polyfills');
 var raven;
 
 // Read settings rendered into sidebar app HTML by service/extension.
-var settings = require('../shared/settings')(document);
+var settings = require('../shared/settings').jsonConfigsFrom(document);
 
 if (settings.raven) {
   // Initialize Raven. This is required at the top of this file