Adding unit tests for lib/shared #3600

macleodbroad-wf · 2017-10-21T12:29:59Z

Adding unit tests and tightening up code as I go.
Tests and code changes will always be in separate commits

…html code coverage report using the command `npm run-script test-cov`

…e coverage stats are complete.

macleodbroad-wf · 2017-10-21T12:52:01Z

Test coverage of lib/shared before this PR:

mbroad · 2017-10-21T13:50:10Z

test/ColorPicker.test.js

+
+    it('handles null', function () {
+      var colorPicker = new ColorPicker();
+      colorPicker.setColor('none');


This test is incorrect. Reminder to self: fix this.

mbroad · 2017-10-21T13:53:17Z

test/Configurator.test.js

+      var config = new Configurator(Network, this.container);
+      config.options.enabled = false;
+      config.setOptions();
+      assert.equal(JSON.stringify(config.options), JSON.stringify(config.defaultOptions));


Missing an assertion

mbroad · 2017-10-21T13:55:58Z

test/Popup.test.js

+      assert(isNaN(popup.y));
+    });
+
+    it('handles null with NaN', function () {


Test description is incorrect

wimrijnders

Can we agree not to do such monster PR any more (that goes for me as well)? It is plain daunting. This particular one would have been easy to carve up on multiples.

On the whole, great changes. Goals is clearly to get the coverage up.

wimrijnders · 2017-10-24T13:41:05Z

lib/shared/Configurator.js

@@ -66,6 +66,10 @@ class Configurator {
        this.options.filter = options.join();
      }
      else if (typeof options === 'object') {
+        if (options == null)


I'd really prefer it if the brace positioning is the same as the surrounding code

Good catch BTW. It's bitten me before that typeof null === 'object'. This should actually be checked in the entire code.

wimrijnders · 2017-10-24T13:43:40Z

lib/util.js

@@ -31,25 +31,6 @@ exports.recursiveDOMDelete = function (DOMobject) {
 };

 /**
- * this function gives you a range between 0 and 1 based on the min and max values in the set, the total sum of all values and the current value.


RIP, you useless code!

wimrijnders · 2017-10-24T13:50:45Z

test/Activator.test.js

+      activator.activate();
+      assert.equal(activator.active, true);
+      assert(eventSpy.called, 'Event did not fire.');
+      assert(eventSpy.calledOnce, 'Event fired more than once');


I sort of half understand what sinon does, so please humor me: what is this assertion good for?

In this case sinon provides a spy, which acts as a sort of accumulator of meta data, about how it was used.
Similar to a mock, except that I'm able to verify whether or not it was called, how many times, and what arguments it was called with.
This test ensures that if code changes were introduced that prevented the callback from being triggered, or caused it to be triggered multiple times, the test would fail.

wimrijnders · 2017-10-24T13:55:59Z

test/Activator.test.js

+    it('emits a `change` event', function () {
+      var eventSpy = sinon.spy();
+      var activator = new Activator(this.container);
+      activator.on('change', eventSpy);


I can't help noticing in the code that changed is fired every single time that activate() or deactivate() is called.

What about consecutive same calls, e.g. activate(); activate();? I would expect change not to fire because the state has not changed.

If this is not documented, I would consider this faultly. Please give it some thought.

I'm not sure what to do about this, but would also consider it outside the scope of this particular issue.

I agree with you, but don't know the impact of making corrective changes.

Would you like me to open an issue to investigate?

wimrijnders · 2017-10-24T13:57:40Z

test/Activator.test.js

+    });
+  });
+
+  describe('deactivate', function () {


Describe-block is practically identical to the active block. Any chance of DRY-ing?

I think drying it would muddy the test state, or make the tests more complex.

wimrijnders · 2017-10-24T14:57:01Z

test/Popup.test.js

+    it('removes frame from container', function () {
+      var popup = new Popup(this.container);
+      popup.destroy();
+      assert.equal(this.container.children.length, 0);


This proves nothing, there may be more children, or previously zero children.
Better compare it against 'previous - 1'.

I disagree. Although the setup is not explicit, I believe this is adequate.

I would prefer seeing the following:

Before: assert that there are no children

after creating popup: assert that there is one child => prove that it's added

after destroy: as is.

wimrijnders · 2017-10-24T15:01:47Z

test/util.test.js

+    });
+  });
+
+  describe('isDate', function () {


Expecting a test on bad values here, to eliminate false positives.
With garbage input, near dates (e.g. 1 past en dof month, leap years feb)

wimrijnders · 2017-10-24T15:02:42Z

test/util.test.js

+      assert.throws(function () {util.convert({}, 'UnknownType');}, Error, null);
+    });
+  });
+  describe('getType', function () {


newlines, also below

wimrijnders · 2017-10-24T15:03:59Z

test/util.test.js

+  });
+
+  describe('easingFunctions', function () {
+    it('take a number and output a number', function () {


What are bad inputs? Tests possible?

These tests are to verify that all easing functions satisfy the required interface. Ie, accepts numeric input and provides numeric output.
Failures of any other type, or acceptance of any other type are irrelevant, imo.

wimrijnders · 2017-10-24T15:05:27Z

test/util.test.js

+    it('arrays of different lengths are not equal', function () {
+      assert.equal(util.equalArray([1, 2, 3], [1, 2]), false)
+    });
+    it('arrays with different content are not equal', function () {


would "2'" === 2 here? Methinks a unit test about this is in order.

mbroad · 2017-10-27T01:05:16Z

@wimrijnders - This is ready for you to re-review & hopefully merge.

wimrijnders

Small questions still. Anything unaddressed I skipped over I consider to be not critical.

wimrijnders · 2017-10-27T09:07:32Z

test/ColorPicker.test.js

+      assert.equal(colorPicker.applied, false);
+      assert.equal(colorPicker.frame.style.display, 'block');
+    });
+  });


You're not going to do this?

wimrijnders · 2017-10-27T09:12:07Z

test/util.test.js

@@ -520,6 +520,15 @@ describe('mergeOptions', function () {
    it('identifies a date string', function () {
      assert.equal(util.isDate(''), false);
    });
+
+    it('identifies non-dates', function () {


😆 I just love how you copied over my 'usual suspects'. You'll encounter them as well in the Network unit tests.

mbroad · 2017-10-27T14:17:21Z

test/Popup.test.js

+      var popup = new Popup(this.container);
+      assert.equal(this.container.children.length, 1);
+      popup.destroy();
+      assert.equal(this.container.children.length, 0);


Although I disagree with this from a purist standpoint, I'll concede to practicality.

# Conflicts: # test/canvas-mock.js

macleodbroad-wf · 2017-10-29T00:54:27Z

@wimrijnders , @yotamberk - This should be ready to go. I've addressed the comments and resolved the merge conflicts.

Inactivity

* Adds code coverage report the output of `npm test` and adds detailed html code coverage report using the command `npm run-script test-cov` * Switch over to using functions in lib/ rather than dist/, so that code coverage stats are complete. * Import vis at the top level to keep ItemSet passing * Remove requirement for dist/vis in TimelineItemSet * Adds tests for Popup * Tests and sinon dependency introduced * Code changes to modules to tighten up code * Corrects broken tests and adds more tests to ColorPicker * Adds additional tests to DataSet * Adds tests for uuid * Removes unused functions from util * Adds tests for utils: recursiveDomDelete, isDate, convert and isType * removes redundant code * Adds additional util tests * Address spacing, and unnecessary tests * Correct test description * Adds isDate tests * Adds sanity check assertions to popup destroy tests

MacLeod Broad added 7 commits October 14, 2017 20:03

Adds code coverage report the output of npm test and adds detailed …

b450758

…html code coverage report using the command `npm run-script test-cov`

Switch over to using functions in lib/ rather than dist/, so that cod…

b50ddc5

…e coverage stats are complete.

Import vis at the top level to keep ItemSet passing

bbcaa78

Remove requirement for dist/vis in TimelineItemSet

6c8cf80

Adds tests for Popup

294525c

Tests and sinon dependency introduced

319fced

Code changes to modules to tighten up code

989bfd2

macleodbroad-wf self-assigned this Oct 21, 2017

macleodbroad-wf added the Work In Progress label Oct 21, 2017

Merge branch 'develop' into mbroad/unittest/lib/shared

3aae9fc

macleodbroad-wf changed the title ~~[Work In Progress] Adding unit tests for lib/shared~~ Adding unit tests for lib/shared Oct 21, 2017

macleodbroad-wf removed the Work In Progress label Oct 21, 2017

MacLeod Broad added 2 commits October 21, 2017 08:45

Merge branch 'develop' into mbroad/code-coverage

a56cd13

Merge branch 'mbroad/code-coverage' into mbroad/unittest/lib/shared

4b766a5

mbroad reviewed Oct 21, 2017

View reviewed changes

test/Popup.test.js Outdated

assert(isNaN(popup.y));

});

it('handles null with NaN', function () {

Copy link

mbroad Oct 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test description is incorrect

MacLeod Broad added 7 commits October 21, 2017 12:53

Corrects broken tests and adds more tests to ColorPicker

e25d2e2

Adds additional tests to DataSet

4c68bae

Adds tests for uuid

139bc86

Removes unused functions from util

429b63c

Adds tests for utils: recursiveDomDelete, isDate, convert and isType

6288a41

removes redundant code

645fc91

Adds additional util tests

c19e524

wimrijnders suggested changes Oct 24, 2017

View reviewed changes

MacLeod Broad added 3 commits October 24, 2017 21:46

Address spacing, and unnecessary tests

f81eef5

Correct test description

031d640

Adds isDate tests

0f8455f

wimrijnders previously requested changes Oct 27, 2017

View reviewed changes

Adds sanity check assertions to popup destroy tests

8303487

mbroad reviewed Oct 27, 2017

View reviewed changes

Merge branch 'develop' into mbroad/unittest/lib/shared

388edb5

# Conflicts: # test/canvas-mock.js

yotamberk approved these changes Nov 4, 2017

View reviewed changes

yotamberk merged commit fb6872d into develop Nov 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding unit tests for lib/shared #3600

Adding unit tests for lib/shared #3600

macleodbroad-wf commented Oct 21, 2017

macleodbroad-wf commented Oct 21, 2017 •

edited

Loading

mbroad Oct 21, 2017

mbroad Oct 21, 2017

mbroad Oct 21, 2017

wimrijnders left a comment

wimrijnders Oct 24, 2017

wimrijnders Oct 24, 2017

wimrijnders Oct 24, 2017

wimrijnders Oct 24, 2017

mbroad Oct 24, 2017

wimrijnders Oct 24, 2017

mbroad Oct 25, 2017

wimrijnders Oct 24, 2017

mbroad Oct 24, 2017

wimrijnders Oct 24, 2017

mbroad Oct 25, 2017

wimrijnders Oct 27, 2017

wimrijnders Oct 24, 2017

wimrijnders Oct 24, 2017

wimrijnders Oct 24, 2017

mbroad Oct 25, 2017

wimrijnders Oct 24, 2017

mbroad commented Oct 27, 2017

wimrijnders left a comment

wimrijnders Oct 27, 2017

wimrijnders Oct 27, 2017

mbroad Oct 27, 2017

macleodbroad-wf commented Oct 29, 2017

Adding unit tests for lib/shared #3600

Adding unit tests for lib/shared #3600

Conversation

macleodbroad-wf commented Oct 21, 2017

macleodbroad-wf commented Oct 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wimrijnders left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbroad commented Oct 27, 2017

wimrijnders left a 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

macleodbroad-wf commented Oct 29, 2017

macleodbroad-wf commented Oct 21, 2017 •

edited

Loading