DataSet - Adds unit tests for add, setOptions and on/off #3394
Conversation
There was a problem hiding this comment.
In general, I find the splitting of the previous tests too fine-grained. This is probably me being an old fart.
More of concern, please ensure that the state is always well-defined for every test; above all, don't make assumptions about the order of execution of tests.
test/DataSet.test.js
Outdated
| data.add({id: 1, content: 'Item 1', start: new Date(now.valueOf())}); | ||
| data.add({id: 2, content: 'Item 2', start: now.toISOString()}); | ||
| data.add([ | ||
| //{id: 3, content: 'Item 3', start: moment(now)}, // TODO: moment fails, not the same instance |
There was a problem hiding this comment.
If you're taking out the moment-require, might as well remove this as well.
test/DataSet.test.js
Outdated
| {id: 3, content: 'Item 3', start: now}, | ||
| {id: 4, content: 'Item 4', start: '/Date(' + now.valueOf() + ')/'} | ||
| ]); | ||
| var sort = function (a, b) { |
There was a problem hiding this comment.
Perfectly fine to extract this from the it-blocks, but it looks kind of lost here, because it's not used in the next it(){}.
Either take it to the top of describe() or put it above the first it(){} where it's being used.
Also, this can become a plain function.
test/DataSet.test.js
Outdated
| return a.id > b.id; | ||
| }; | ||
|
|
||
| it('can handle multiple data types', function () { |
There was a problem hiding this comment.
With respect to usage of variable data, defined globally in the describe-block:
You are making a potentially dangerous assumption here, namely that the tests will all run, and will always run in the same order.
From the mocha command line usage:
-g, --grep <pattern> only run tests matching <pattern>
-f, --fgrep <string> only run tests containing <string>
I.e. it is possible to run specific tests, not all of them. In addition, a fix to mocha for optionally randomizing the test order is pending.
While this is not directly an issue with the current tests, it's something which time or a user can (and therefore will) break.
My suggestions here are:
- Wrap the initialization of
datain an initialization function, call this in everyit-block - and/or initialize the data in a
beforeEachblock.
These can be combined. The goal in any case is to allow the it-blocks to run independently.
test/DataSet.test.js
Outdated
| end: 'Date' | ||
| } | ||
| }); | ||
| // add single items with different date types |
There was a problem hiding this comment.
See comment gelow on initialization of used data. In any case, don't leave this init code to run 'bare'.
test/DataSet.test.js
Outdated
| return a.id > b.id; | ||
| }; | ||
|
|
||
| it('can handle multiple data types', function () { | ||
|
|
||
| var items = data.get(); |
There was a problem hiding this comment.
Ensure that var data is always in a well-defined state before running any check on it. This comment applies to all usages of data in this file.
test/DataSet.test.js
Outdated
| }); | ||
|
|
||
| describe('add', function () { | ||
| var dataset = new DataSet([]); |
There was a problem hiding this comment.
See comments in previous describe-blocks on usage of bare data. Please just ensure that data is re-initialized upon execution of every it-block.
| @@ -282,7 +285,111 @@ describe('DataSet', function () { | |||
| {id: 3, content: 'Item 3'}, | |||
| {id: 4, content: 'Item 4'} | |||
| ]); | |||
| }); | |||
|
|
|||
| describe('add', function () { | |||
There was a problem hiding this comment.
👍 For new tests. Looks clean except for the data initialization thing.
This is stuff which is assumed to 'just work'. Which is exactly the reason why they should be regression-tested.
There was a problem hiding this comment.
Has been addressed
macleodbroad-wf
force-pushed
the
mbroad/unittest/dataset
branch
from
a51410b
to
05bfba6
Compare
|
@wimrijnders - I've addressed your comments |
| @@ -122,7 +117,7 @@ DataSet.prototype.setOptions = function(options) { | |||
|
|
|||
| /** | |||
| * Subscribe to an event, add an event listener | |||
| * @param {String} event Event name. Available events: 'put', 'update', | |||
| * @param {String} event Event name. Available events: 'add', 'update', | |||
There was a problem hiding this comment.
Yesterday, I learned this should be {string}. But I suppose that the other PR will handle that.
There was a problem hiding this comment.
Yes, to be addressed in a follow up PR.
lib/DataSet.js
Outdated
| @@ -234,6 +216,7 @@ DataSet.prototype.add = function (data, senderId) { | |||
| * @param {Object | Array} data | |||
| * @param {String} [senderId] Optional sender id | |||
| * @return {Array} updatedIds The ids of the added or updated items | |||
| * @throws {Error} Error - Unknown Datatype | |||
There was a problem hiding this comment.
Second Error shouldn't be there, just the description.
|
tests are failing again. Not because of this PR |
* Adds unit tests for add, setOptions and on/off * removes extraneous return value name in @throws tag of jsdoc
@yotamberk @wimrijnders