Tests and other tweaks to shared/settings.js #406

seanh · 2017-05-26T17:14:22Z

Another little thing that I've extracted out of config code refactoring work:

Rewrite the shared/settings.js unit tests with more detailed ones
Have shared/settings.js export an object with a jsonConfigsFrom(document) function rather than just exporting a top-level function.

The way I'm intending for shared/settings.js to be used is as a container for helper / utility functions that are called by both the annotator and the sidebar config code, and I expect this to contain more than just one function in the future, so I'm making space for it.

Also "settings" in the client can be read from lots of places: the URL, js-hypothesis-config tags in the host page, or in the app.html, window.hypothesisConfig() in the host page, the annotator+html link... This jsonConfigsFrom(document) is really just a utility function for parsing JSON tags out of (any) document, it doesn't return "the settings" as a whole, so I think it needs a more specific name than settings()
Also rewrote the function's docstring

I'd like to do more work to fix a couple of issues that the tests found:

Stop this function from throwing JSON parsing errors. In every case where we use the function I think the desired behaviour would be to catch and log the error (in practice in some cases we do that and in some cases we don't catch it and crash), so the function should do the catch and log itself
Stop this function from returning nonsense when a json tag doesn't contain a top-level object, as a few of the tests show

But I've run out of time for this week so I'll submit a separate pr for those changes later

codecov-io · 2017-05-26T17:14:25Z

Codecov Report

Merging #406 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #406      +/-   ##
==========================================
+ Coverage   89.63%   89.65%   +0.02%     
==========================================
  Files         123      123              
  Lines        4910     4910              
  Branches      846      846              
==========================================
+ Hits         4401     4402       +1     
+ Misses        509      508       -1

Impacted Files	Coverage Δ
src/annotator/config.js	`100% <100%> (ø)`	⬆️
src/shared/settings.js	`100% <100%> (ø)`	⬆️
src/sidebar/components/markdown.js	`93.15% <0%> (+1.36%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ba3b75...5a33ce1. Read the comment docs.

seanh · 2017-05-26T17:16:40Z

src/annotator/config.js

@@ -18,7 +18,7 @@ function config(window_) {

  // Parse config from `<script class="js-hypothesis-config">` tags
  try {
-    Object.assign(options, settings(window_.document));
+    Object.assign(options, settings.jsonConfigsFrom(window_.document));


I think this is much clearer, jsonConfigsFrom(document) tells me that this is reading the JSON objs from the document (as opposed to the variety of other options/settings that the client reads from documents), without me having to go look in settings.js

seanh · 2017-05-26T17:17:29Z

src/shared/settings.js

@@ -12,17 +12,22 @@ function assign(dest, src) {
 }

 /**
- * Return application configuration information from the host page.


This was actually wrong - the function isn't always used to read settings from the host page

seanh · 2017-05-26T17:18:48Z

src/shared/settings.js

 *
- * Exposes shared application settings, read from script tags with the


This is also misleading. It's a shared utility function, yes. But "shared application settings" is misleading. Not every setting read from a js-hypothesis-config is actually shared between the three legs of the app (annotator, boot and sidebar), nor is every setting that is shared read from this function.

seanh · 2017-05-26T17:19:42Z

src/shared/settings.js

+ * 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).


I've left the exception that this function throws (on invalid JSON) still undocumented, as I intend to send a PR to stop iut from throwing the exception anyway

seanh · 2017-05-26T17:20:25Z

src/shared/test/settings-test.js

+        appendJSHypothesisConfig(document, '["a", "b", "c"]');
+      });
+
+      it('returns the array, parsed into an object', function() {


We should fix this in a separate pr, it should ignore top-level arrays (but log a warning)

seanh · 2017-05-26T17:20:38Z

src/shared/test/settings-test.js

+        appendJSHypothesisConfig(document, '"hi"');
+      });
+
+      it('returns the string, parsed into an object', function() {


We should fix this in a separate pr, it should ignore top-level strings (but log a warning)

Heh. LOLJS :)

seanh · 2017-05-26T17:22:04Z

src/shared/test/settings-test.js

+        appendJSHypothesisConfig(document, 'this is not valid json');
+      });
+
+      it('throws a SyntaxError', function() {


We should fix this in a follow up pr: it should log a warning, and ignore the invalid json, not throw

robertknight · 2017-05-31T11:08:03Z

Stop this function from throwing JSON parsing errors. In every case where we use the function I think the desired behaviour would be to catch and log the error

Unhandled errors in the sidebar code get reported to us via Raven, so if you catch the error in the context of the sidebar and log an error, that could actually have the effect of hiding the problem from us.

robertknight · 2017-05-31T11:10:50Z

src/shared/settings.js

+ * setting names, scripts further down in the document override those further
+ * up).
+ *
+ * @param {Document|Element} document - The root element to search for.


The end of the sentence ("... search for script tags") is missing here.

robertknight · 2017-05-31T11:11:44Z

src/shared/test/settings-test.js

+    afterEach('remove js-hypothesis-config tags', function() {
+      var elements = document.querySelectorAll('.js-settings-test');
+      for (var i=0; i < elements.length; i++) {
+        elements[i].parentNode.removeChild(elements[i]);


You can shorten this to elements[i].remove(). Some older code uses removeChild() because Element#remove was not supported in older versions of PhantomJS.

robertknight

LGTM. Renaming and updated descriptions look sensible to me. I suggested a docstring fix and a slightly simpler way to remove elements from the DOM.

Add a new set of unit tests that test more cases.

seanh · 2017-05-31T17:03:04Z

Done both suggestions

seanh added the Needs Review label May 26, 2017

seanh commented May 26, 2017

View reviewed changes

seanh force-pushed the add-tests-for-shared-settings branch from 1981f17 to 9390463 Compare May 26, 2017 17:21

seanh commented May 26, 2017

View reviewed changes

seanh self-assigned this May 30, 2017

seanh mentioned this pull request May 30, 2017

Client config is complicated hypothesis/product-backlog#299

Open

robertknight reviewed May 31, 2017

View reviewed changes

robertknight approved these changes May 31, 2017

View reviewed changes

seanh added 4 commits May 31, 2017 17:56

Rename settings() to jsonConfigsFrom()

6eccb5d

Improve shared/settings.js tests

e0303a3

Add a new set of unit tests that test more cases.

Improve parseJSONConfigsFrom() docstring

d0efb08

Simplify a line of code

5a33ce1

seanh force-pushed the add-tests-for-shared-settings branch from 9390463 to 5a33ce1 Compare May 31, 2017 16:59

seanh merged commit c31cbbe into master May 31, 2017

seanh deleted the add-tests-for-shared-settings branch May 31, 2017 17:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests and other tweaks to shared/settings.js #406

Tests and other tweaks to shared/settings.js #406

seanh commented May 26, 2017

codecov-io commented May 26, 2017 •

edited

Loading

seanh May 26, 2017 •

edited

Loading

seanh May 26, 2017

seanh May 26, 2017

seanh May 26, 2017

seanh May 26, 2017

seanh May 26, 2017

robertknight May 31, 2017

seanh May 26, 2017

robertknight commented May 31, 2017

robertknight May 31, 2017

robertknight May 31, 2017

robertknight left a comment

seanh commented May 31, 2017

		*
		* Exposes shared application settings, read from script tags with the

Tests and other tweaks to shared/settings.js #406

Tests and other tweaks to shared/settings.js #406

Conversation

seanh commented May 26, 2017

codecov-io commented May 26, 2017 • edited Loading

Codecov Report

seanh May 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertknight commented May 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertknight left a comment

Choose a reason for hiding this comment

seanh commented May 31, 2017

codecov-io commented May 26, 2017 •

edited

Loading

seanh May 26, 2017 •

edited

Loading