-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Adding unit tests for lib/shared #3600
Changes from 7 commits
b450758
b50ddc5
bbcaa78
6c8cf80
294525c
319fced
989bfd2
3aae9fc
a56cd13
4b766a5
e25d2e2
4c68bae
139bc86
429b63c
6288a41
645fc91
c19e524
f81eef5
031d640
0f8455f
8303487
388edb5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,92 @@ | ||
var assert = require('assert'); | ||
var sinon = require('sinon'); | ||
var jsdom_global = require('jsdom-global'); | ||
|
||
var canvasMockify = require('./canvas-mock'); | ||
var Activator = require('../lib/shared/Activator'); | ||
|
||
|
||
describe('Activator', function () { | ||
beforeEach(function() { | ||
this.jsdom_global = jsdom_global( | ||
"<div id='mynetwork'></div>", | ||
{ skipWindowCheck: true} | ||
); | ||
canvasMockify(window); | ||
this.container = document.getElementById('mynetwork'); | ||
}); | ||
|
||
afterEach(function() { | ||
this.jsdom_global(); | ||
this.container.remove(); | ||
this.container = undefined; | ||
}); | ||
|
||
describe('constructor', function () { | ||
|
||
it('sets defaults', function () { | ||
var activator = new Activator(this.container); | ||
assert.equal(activator.active, false); | ||
}); | ||
|
||
it('creates overlay', function () { | ||
var activator = new Activator(this.container); | ||
assert.equal(activator.dom.container.children[0].className, 'vis-overlay'); | ||
}); | ||
}); | ||
|
||
describe('activate', function () { | ||
it('emits an `activate` event', function () { | ||
var eventSpy = sinon.spy(); | ||
var activator = new Activator(this.container); | ||
activator.on('activate', eventSpy); | ||
activator.activate(); | ||
assert.equal(activator.active, true); | ||
assert(eventSpy.called, 'Event did not fire.'); | ||
assert(eventSpy.calledOnce, 'Event fired more than once'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I sort of half understand what There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this case |
||
}); | ||
|
||
it('emits a `change` event', function () { | ||
var eventSpy = sinon.spy(); | ||
var activator = new Activator(this.container); | ||
activator.on('change', eventSpy); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't help noticing in the code that What about consecutive same calls, e.g. If this is not documented, I would consider this faultly. Please give it some thought. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
activator.activate(); | ||
assert.equal(activator.active, true); | ||
assert(eventSpy.called, 'Event did not fire.'); | ||
assert(eventSpy.calledOnce, 'Event fired more than once'); | ||
}); | ||
}); | ||
|
||
describe('deactivate', function () { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Describe-block is practically identical to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think drying it would muddy the test state, or make the tests more complex. |
||
it('emits a `deactivate` event', function () { | ||
var eventSpy = sinon.spy(); | ||
var activator = new Activator(this.container); | ||
activator.on('deactivate', eventSpy); | ||
activator.deactivate(); | ||
assert.equal(activator.active, false); | ||
assert(eventSpy.called, 'Event did not fire.'); | ||
assert(eventSpy.calledOnce, 'Event fired more than once'); | ||
}); | ||
|
||
it('emits a `change` event', function () { | ||
var eventSpy = sinon.spy(); | ||
var activator = new Activator(this.container); | ||
activator.on('change', eventSpy); | ||
activator.deactivate(); | ||
assert.equal(activator.active, false); | ||
assert(eventSpy.called, 'Event did not fire.'); | ||
assert(eventSpy.calledOnce, 'Event fired more than once'); | ||
}); | ||
}); | ||
|
||
describe('destroy', function () { | ||
|
||
it('sets inactive, removes keycharm, and removes hammer', function () { | ||
var activator = new Activator(this.container); | ||
activator.destroy(); | ||
assert.equal(activator.active, false); | ||
assert.equal(activator.keycharm, null); | ||
assert.equal(activator.hammer, null); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,123 @@ | ||
var assert = require('assert'); | ||
var sinon = require('sinon'); | ||
var jsdom_global = require('jsdom-global'); | ||
|
||
var canvasMockify = require('./canvas-mock'); | ||
var ColorPicker = require('../lib/shared/ColorPicker').default; | ||
|
||
describe('ColorPicker', function () { | ||
beforeEach(function() { | ||
this.jsdom_global = jsdom_global( | ||
"<div id='mynetwork'></div>", | ||
{ skipWindowCheck: true} | ||
); | ||
canvasMockify(window); | ||
this.container = document.getElementById('mynetwork'); | ||
}); | ||
|
||
afterEach(function() { | ||
this.jsdom_global(); | ||
this.container.remove(); | ||
this.container = undefined; | ||
}); | ||
|
||
describe('constructor', function () { | ||
|
||
it('sets defaults', function () { | ||
var colorPicker = new ColorPicker(); | ||
assert.equal(colorPicker.pixelRatio, 1); | ||
assert.equal(colorPicker.generated, false); | ||
assert.deepEqual(colorPicker.centerCoordinates, {x:289/2, y:289/2}); | ||
assert.equal(colorPicker.r, 289 * 0.49); | ||
assert.deepEqual(colorPicker.color, {r:255,g:255,b:255,a:1.0}); | ||
assert.equal(colorPicker.hueCircle, undefined); | ||
assert.deepEqual(colorPicker.initialColor, {r:255,g:255,b:255,a:1.0}); | ||
assert.equal(colorPicker.previousColor, undefined); | ||
assert.equal(colorPicker.applied, false); | ||
}); | ||
|
||
// TODO: This gets overridden during instantiation - Is this a bug? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the TODO the reason why it's called There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'd say something fishy is going on here. My current view is that passing the device picel ratio as parameter to the constructor is useless. It might as well get removed (breaking change). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree. I tried to write the test, could not make it pass, kept the test as documentation of the strange behaviour. |
||
xit('can overwrite default pixelRation', function () { | ||
var colorPicker = new ColorPicker(777); | ||
assert.equal(colorPicker.pixelRatio, 777); | ||
}); | ||
|
||
}); | ||
|
||
describe('insertTo', function () { | ||
|
||
it('inserts the colorPicker into a div from the DOM', function () { | ||
var colorPicker = new ColorPicker(); | ||
colorPicker.insertTo(this.container); | ||
assert.equal(colorPicker.container, this.container); | ||
assert.equal(this.container.children[this.container.children.length-1], colorPicker.frame); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For my understanding: does this do a deep compare, or is it just a reference compare? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a deep compare |
||
}); | ||
}); | ||
|
||
describe('setUpdateCallback', function () { | ||
|
||
it('prevents non-functions from being set as callback', function () { | ||
var colorPicker = new ColorPicker(); | ||
assert.throws(function () {colorPicker.setUpdateCallback(null);}, Error, null); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm inclined to add the usual suspects here: 'null, undefined, [1,2,3], {a: 42}, 42, "meow"`. Actually, I would consider being able to undo a callback (with Same applies to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. addressed |
||
}); | ||
}); | ||
|
||
describe('setCloseCallback', function () { | ||
|
||
it('prevents non-functions from being set as callback', function () { | ||
var colorPicker = new ColorPicker(); | ||
assert.throws(function () {colorPicker.setCloseCallback(null);}, Error, null); | ||
}); | ||
}); | ||
|
||
describe('_hide', function () { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be expecting a test on setting parameter |
||
|
||
it('runs updateCallback when applied', function () { | ||
var callback = sinon.spy(); | ||
var colorPicker = new ColorPicker(); | ||
colorPicker.setUpdateCallback(callback); | ||
colorPicker.applied = true; | ||
colorPicker._hide(); | ||
assert.equal(callback.callCount, 1); | ||
}); | ||
|
||
it('does not run updateCallback when not applied', function () { | ||
var callback = sinon.spy(); | ||
var colorPicker = new ColorPicker(); | ||
colorPicker.setUpdateCallback(callback); | ||
colorPicker.applied = false; | ||
colorPicker._hide(); | ||
assert.equal(callback.callCount, 0); | ||
}); | ||
}); | ||
|
||
describe('_isColorString', function () { | ||
|
||
it('returns color code when color is found', function () { | ||
var colorPicker = new ColorPicker(); | ||
var color = colorPicker._isColorString('black'); | ||
assert.equal(color, '#000000'); | ||
}); | ||
|
||
it('returns undefined when color is not found', function () { | ||
var colorPicker = new ColorPicker(); | ||
var color = colorPicker._isColorString('zing!'); | ||
assert.equal(color, undefined); | ||
}); | ||
}); | ||
|
||
describe('setColor', function () { | ||
|
||
it('does not change when \'none\'', function () { | ||
var colorPicker = new ColorPicker(); | ||
colorPicker.setColor('none'); | ||
assert.deepEqual(colorPicker.color, { r: 255, g: 255, b: 255, a: 1 }); | ||
}); | ||
|
||
it('handles null', function () { | ||
var colorPicker = new ColorPicker(); | ||
colorPicker.setColor('none'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test is incorrect. Reminder to self: fix this. |
||
assert.deepEqual(colorPicker.color, { r: 255, g: 255, b: 255, a: 1 }); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd really prefer it if the brace positioning is the same as the surrounding code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch BTW. It's bitten me before that
typeof null === 'object'
. This should actually be checked in the entire code.