From b0481d3220135ea81fed4a9069f1e28b8826ec7c Mon Sep 17 00:00:00 2001 From: wimrijnders Date: Sat, 16 Sep 2017 21:13:20 +0200 Subject: [PATCH] Strapping down of Extend-routines in util.js (#3442) * The next fix on Travis unit test failure This is the next escalation on the war against the Travis unit tests failing (which came into being by yours truly). By accident, I could recreate the unit test failure on my development machine. This led to a more directed effort to squash the bug. The insight here is that test `(window === undefined)` fails, but `(typeof window === 'undefined`)` succeeds. This undoubtedly has to do with the special status `window` has as a global object. Changes: - Added check on presence of `window` in `Canvas._requestNextFrame()`, fixed local source errors. - Added catch clause in `CanvasRendered._determinePixelRatio()` - small fix: raised timeout for the network `worldCup2014` unit test * Preliminary refactoring in utils.js * Added unit tests for extend routines, commenting and small fixes * More unit tests for extend routines * - Completed unit tests for extend routines in - Small fixes and cleanup in `util.js` - Removed `util.protoExtend()`, not used anywhere * Added unit tests for known font options --- lib/util.js | 230 ++++++++++++++--------------- test/Network.test.js | 59 ++++++++ test/util.test.js | 341 ++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 509 insertions(+), 121 deletions(-) diff --git a/lib/util.js b/lib/util.js index e65d973a..cced85de 100644 --- a/lib/util.js +++ b/lib/util.js @@ -107,24 +107,49 @@ exports.assignAllKeys = function (obj, value) { /** - * Fill an object with a possibly partially defined other object. Only copies values if the a object has an object requiring values. + * Copy property from b to a if property present in a. + * If property in b explicitly set to null, delete it if `allowDeletion` set. + * + * Internal helper routine, should not be exported. Not added to `exports` for that reason. + * + * @param {object} a target object + * @param {object} b source object + * @param {string} prop name of property to copy to a + * @param {boolean} allowDeletion if true, delete property in a if explicitly set to null in b + * @private + */ +function copyOrDelete(a, b, prop, allowDeletion) { + var doDeletion = false; + if (allowDeletion === true) { + doDeletion = (b[prop] === null && a[prop] !== undefined); + } + + if (doDeletion) { + delete a[prop]; + } else { + a[prop] = b[prop]; // Remember, this is a reference copy! + } +} + + +/** + * Fill an object with a possibly partially defined other object. + * + * Only copies values for the properties already present in a. * That means an object is not created on a property if only the b object has it. + * * @param {object} a * @param {object} b - * @param {boolean} [allowDeletion=false] + * @param {boolean} [allowDeletion=false] if true, delete properties in a that are explicitly set to null in b */ exports.fillIfDefined = function (a, b, allowDeletion = false) { + // NOTE: iteration of properties of a + // NOTE: prototype properties iterated over as well for (var prop in a) { if (b[prop] !== undefined) { - if (typeof b[prop] !== 'object') { - if ((b[prop] === undefined || b[prop] === null) && a[prop] !== undefined && allowDeletion === true) { - delete a[prop]; - } - else { - a[prop] = b[prop]; - } - } - else { + if (b[prop] === null || typeof b[prop] !== 'object') { // Note: typeof null === 'object' + copyOrDelete(a, b, prop, allowDeletion); + } else { if (typeof a[prop] === 'object') { exports.fillIfDefined(a[prop], b[prop], allowDeletion); } @@ -134,24 +159,6 @@ exports.fillIfDefined = function (a, b, allowDeletion = false) { }; - -/** - * Extend object a with the properties of object b or a series of objects - * Only properties with defined values are copied - * @param {Object} a - * @param {...Object} b - * @return {Object} a - */ -exports.protoExtend = function (a, b) { // eslint-disable-line no-unused-vars - for (var i = 1; i < arguments.length; i++) { - var other = arguments[i]; - for (var prop in other) { - a[prop] = other[prop]; - } - } - return a; -}; - /** * Extend object a with the properties of object b or a series of objects * Only properties with defined values are copied @@ -197,118 +204,112 @@ exports.selectiveExtend = function (props, a, b) { // eslint-disable-line no-un return a; }; + /** - * Extend object a with selected properties of object b or a series of objects - * Only properties with defined values are copied - * @param {Array.} props - * @param {Object} a - * @param {Object} b - * @param {boolean} [allowDeletion=false] - * @return {Object} a + * Extend object a with selected properties of object b. + * Only properties with defined values are copied. + * + * **Note:** Previous version of this routine implied that multiple source objects + * could be used; however, the implementation was **wrong**. + * Since multiple (>1) sources weren't used anywhere in the `vis.js` code, + * this has been removed + * + * @param {Array.} props names of first-level properties to copy over + * @param {object} a target object + * @param {object} b source object + * @param {boolean} [allowDeletion=false] if true, delete property in a if explicitly set to null in b + * @returns {Object} a */ exports.selectiveDeepExtend = function (props, a, b, allowDeletion = false) { // TODO: add support for Arrays to deepExtend if (Array.isArray(b)) { throw new TypeError('Arrays are not supported by deepExtend'); } - for (var i = 2; i < arguments.length; i++) { - var other = arguments[i]; - for (var p = 0; p < props.length; p++) { - var prop = props[p]; - if (other.hasOwnProperty(prop)) { - if (b[prop] && b[prop].constructor === Object) { - if (a[prop] === undefined) { - a[prop] = {}; - } - if (a[prop].constructor === Object) { - exports.deepExtend(a[prop], b[prop], false, allowDeletion); - } - else { - if ((b[prop] === null) && a[prop] !== undefined && allowDeletion === true) { - delete a[prop]; - } - else { - a[prop] = b[prop]; - } - } - } else if (Array.isArray(b[prop])) { - throw new TypeError('Arrays are not supported by deepExtend'); - } else { - if ((b[prop] === null) && a[prop] !== undefined && allowDeletion === true) { - delete a[prop]; - } - else { - a[prop] = b[prop]; - } - } + for (var p = 0; p < props.length; p++) { + var prop = props[p]; + if (b.hasOwnProperty(prop)) { + if (b[prop] && b[prop].constructor === Object) { + if (a[prop] === undefined) { + a[prop] = {}; + } + if (a[prop].constructor === Object) { + exports.deepExtend(a[prop], b[prop], false, allowDeletion); + } + else { + copyOrDelete(a, b, prop, allowDeletion); + } + } else if (Array.isArray(b[prop])) { + throw new TypeError('Arrays are not supported by deepExtend'); + } else { + copyOrDelete(a, b, prop, allowDeletion); } } } return a; }; + /** - * Extend object a with selected properties of object b or a series of objects + * Extend object `a` with properties of object `b`, ignoring properties which are explicitly specified + * to be excluded. + * + * The properties of `b` are considered for copying. + * Properties which are themselves objects are are also extended. * Only properties with defined values are copied - * @param {Array.} props - * @param {Object} a - * @param {Object} b - * @param {boolean} [allowDeletion=false] + * + * @param {Array.} propsToExclude names of properties which should *not* be copied + * @param {Object} a object to extend + * @param {Object} b object to take properties from for extension + * @param {boolean} [allowDeletion=false] if true, delete properties in a that are explicitly set to null in b * @return {Object} a */ -exports.selectiveNotDeepExtend = function (props, a, b, allowDeletion = false) { +exports.selectiveNotDeepExtend = function (propsToExclude, a, b, allowDeletion = false) { // TODO: add support for Arrays to deepExtend + // NOTE: array properties have an else-below; apparently, there is a problem here. if (Array.isArray(b)) { throw new TypeError('Arrays are not supported by deepExtend'); } + for (var prop in b) { - if (b.hasOwnProperty(prop)) { - if (props.indexOf(prop) == -1) { - if (b[prop] && b[prop].constructor === Object) { - if (a[prop] === undefined) { - a[prop] = {}; - } - if (a[prop].constructor === Object) { - exports.deepExtend(a[prop], b[prop]); - } - else { - if ((b[prop] === null) && a[prop] !== undefined && allowDeletion === true) { - delete a[prop]; - } - else { - a[prop] = b[prop]; - } - } - } else if (Array.isArray(b[prop])) { - a[prop] = []; - for (let i = 0; i < b[prop].length; i++) { - a[prop].push(b[prop][i]); - } - } else { - if ((b[prop] === null) && a[prop] !== undefined && allowDeletion === true) { - delete a[prop]; - } - else { - a[prop] = b[prop]; - } - } + if (!b.hasOwnProperty(prop)) continue; // Handle local properties only + if (propsToExclude.indexOf(prop) !== -1) continue; // In exclusion list, skip + + if (b[prop] && b[prop].constructor === Object) { + if (a[prop] === undefined) { + a[prop] = {}; + } + if (a[prop].constructor === Object) { + exports.deepExtend(a[prop], b[prop]); // NOTE: allowDeletion not propagated! + } + else { + copyOrDelete(a, b, prop, allowDeletion); + } + } else if (Array.isArray(b[prop])) { + a[prop] = []; + for (let i = 0; i < b[prop].length; i++) { + a[prop].push(b[prop][i]); } + } else { + copyOrDelete(a, b, prop, allowDeletion); } } + return a; }; + /** * Deep extend an object a with the properties of object b + * * @param {Object} a * @param {Object} b - * @param {boolean} [protoExtend] --> optional parameter. If true, the prototype values will also be extended. - * (ie. the options objects that inherit from others will also get the inherited options) - * @param {boolean} [allowDeletion] --> optional parameter. If true, the values of fields that are null will be deleted + * @param {boolean} [protoExtend=false] If true, the prototype values will also be extended. + * (ie. the options objects that inherit from others will also get the inherited options) + * @param {boolean} [allowDeletion=false] If true, the values of fields that are null will be deleted * @returns {Object} */ -exports.deepExtend = function (a, b, protoExtend, allowDeletion) { +exports.deepExtend = function (a, b, protoExtend=false, allowDeletion=false) { for (var prop in b) { if (b.hasOwnProperty(prop) || protoExtend === true) { if (b[prop] && b[prop].constructor === Object) { @@ -316,15 +317,10 @@ exports.deepExtend = function (a, b, protoExtend, allowDeletion) { a[prop] = {}; } if (a[prop].constructor === Object) { - exports.deepExtend(a[prop], b[prop], protoExtend); + exports.deepExtend(a[prop], b[prop], protoExtend); // NOTE: allowDeletion not propagated! } else { - if ((b[prop] === null) && a[prop] !== undefined && allowDeletion === true) { - delete a[prop]; - } - else { - a[prop] = b[prop]; - } + copyOrDelete(a, b, prop, allowDeletion); } } else if (Array.isArray(b[prop])) { a[prop] = []; @@ -332,18 +328,14 @@ exports.deepExtend = function (a, b, protoExtend, allowDeletion) { a[prop].push(b[prop][i]); } } else { - if ((b[prop] === null) && a[prop] !== undefined && allowDeletion === true) { - delete a[prop]; - } - else { - a[prop] = b[prop]; - } + copyOrDelete(a, b, prop, allowDeletion); } } } return a; }; + /** * Test whether all elements in two arrays are equal. * @param {Array} a diff --git a/test/Network.test.js b/test/Network.test.js index 2b450a15..8d6e4b6e 100644 --- a/test/Network.test.js +++ b/test/Network.test.js @@ -17,6 +17,7 @@ var Network = vis.network; var jsdom_global = require('jsdom-global'); var stdout = require('test-console').stdout; var Validator = require("./../lib/shared/Validator").default; +var {allOptions, configureOptions} = require('./../lib/network/options.js'); //var {printStyle} = require('./../lib/shared/Validator'); @@ -178,6 +179,49 @@ function assertNumEdges(network, expectedPresent, expectedVisible) { }; +/** + * Check if the font options haven't changed. + * + * This is to guard against future code changes; a lot of the code deals with particular properties of + * the font options. + * If any assertion fails here, all code in Network handling fonts should be checked. + */ +function checkFontProperties(fontItem, checkStrict = true) { + var knownProperties = [ + 'color', + 'size', + 'face', + 'background', + 'strokeWidth', + 'strokeColor', + 'align', + 'multi', + 'vadjust', + 'bold', + 'boldital', + 'ital', + 'mono', + ]; + + // All properties in fontItem should be known + for (var prop in fontItem) { + if (prop === '__type__') continue; // Skip special field in options definition + if (!fontItem.hasOwnProperty(prop)) continue; + assert(knownProperties.indexOf(prop) !== -1, "Unknown font option '" + prop + "'"); + } + + if (!checkStrict) return; + + // All known properties should be present + var keys = Object.keys(fontItem); + for (var n in knownProperties) { + var prop = knownProperties[n]; + assert(keys.indexOf(prop) !== -1, "Missing known font option '" + prop + "'"); + } +} + + + describe('Network', function () { before(function() { @@ -250,6 +294,13 @@ describe('Network', function () { describe('Node', function () { + it('has known font options', function () { + var network = createNetwork({}); + checkFontProperties(network.nodesHandler.defaultOptions.font); + checkFontProperties(allOptions.nodes.font); + checkFontProperties(configureOptions.nodes.font, false); + }); + /** * NOTE: choosify tests of Node and Edge are parallel @@ -293,6 +344,14 @@ describe('Node', function () { describe('Edge', function () { + it('has known font options', function () { + var network = createNetwork({}); + checkFontProperties(network.edgesHandler.defaultOptions.font); + checkFontProperties(allOptions.edges.font); + checkFontProperties(configureOptions.edges.font, false); + }); + + /** * NOTE: choosify tests of Node and Edge are parallel * TODO: consolidate this is necessary diff --git a/test/util.test.js b/test/util.test.js index 0c112b35..c5d7fc28 100644 --- a/test/util.test.js +++ b/test/util.test.js @@ -2,10 +2,345 @@ var assert = require('assert'); var util = require('../lib/util'); +describe('util', function () { + +/** + * Tests for copy and extend methods. + * + * Goal: to cover all possible paths within the tested method(s) + * + * + * **NOTES** + * + * - All these methods have the inherent flaw that it's possible to define properties + * on an object with value 'undefined'. e.g. in `node`: + * + * > a = { b:undefined } + * > a.hasOwnProperty('b') + * true + * + * The logic for handling this in the code is minimal and accidental. For the time being, + * this flaw is ignored. + */ +describe('extend routines', function () { + + /** + * Check if values have been copied over from b to a as intended + */ + function checkExtended(a, b, checkCopyTarget = false) { + var result = { + color: 'green', + sub: { + enabled: false, + sub2: { + font: 'awesome' + } + } + }; + + assert(a.color !== undefined && a.color === result.color); + assert(a.notInSource === true); + if (checkCopyTarget) { + assert(a.notInTarget === true); + } else { + assert(a.notInTarget === undefined); + } + + var sub = a.sub; + assert(sub !== undefined); + assert(sub.enabled !== undefined && sub.enabled === result.sub.enabled); + assert(sub.notInSource === true); + if (checkCopyTarget) { + assert(sub.notInTarget === true); + } else { + assert(sub.notInTarget === undefined); + } + + sub = a.sub.sub2; + assert(sub !== undefined); + assert(sub !== undefined && sub.font !== undefined && sub.font === result.sub.sub2.font); + assert(sub.notInSource === true); + assert(a.subNotInSource !== undefined); + if (checkCopyTarget) { + assert(a.subNotInTarget.enabled === true); + assert(sub.notInTarget === true); + } else { + assert(a.subNotInTarget === undefined); + assert(sub.notInTarget === undefined); + } + } + + + /** + * Spot check on values of a unchanged as intended + */ + function testAUnchanged(a) { + var sub = a.sub; + assert(sub !== undefined); + assert(sub.enabled !== undefined && sub.enabled === true); + assert(sub.notInSource === true); + assert(sub.notInTarget === undefined); + assert(sub.deleteThis === true); + + sub = a.sub.sub2; + assert(sub !== undefined); + assert(sub !== undefined && sub.font !== undefined && sub.font === 'arial'); + assert(sub.notInSource === true); + assert(sub.notInTarget === undefined); + + assert(a.subNotInSource !== undefined); + assert(a.subNotInTarget === undefined); + } + + + function initA() { + return { + color: 'red', + notInSource: true, + sub: { + enabled: true, + notInSource: true, + sub2: { + font: 'arial', + notInSource: true, + }, + deleteThis: true, + }, + subNotInSource: { + enabled: true, + }, + deleteThis: true, + subDeleteThis: { + enabled: true, + }, + }; + } + + + beforeEach(function() { + this.a = initA(); + + this.b = { + color: 'green', + notInTarget: true, + sub: { + enabled: false, + notInTarget: true, + sub2: { + font: 'awesome', + notInTarget: true, + }, + deleteThis: null, + }, + subNotInTarget: { + enabled: true, + }, + deleteThis: null, + subDeleteThis: null + }; + }); + + + it('performs fillIfDefined() as advertized', function () { + var a = this.a; + var b = this.b; + + util.fillIfDefined(a, b); + checkExtended(a, b); + + // NOTE: if allowDeletion === false, null values are copied over! + // This is due to existing logic; it might not be the intention and hence a bug + assert(a.sub.deleteThis === null); + assert(a.deleteThis === null); + assert(a.subDeleteThis === null); + }); + + + it('performs fillIfDefined() as advertized with deletion', function () { + var a = this.a; + var b = this.b; + + util.fillIfDefined(a, b, true); // thrid param: allowDeletion + checkExtended(a, b); + + // Following should be removed now + assert(a.sub.deleteThis === undefined); + assert(a.deleteThis === undefined); + assert(a.subDeleteThis === undefined); + }); + + + it('performs selectiveDeepExtend() as advertized', function () { + var a = this.a; + var b = this.b; + + // pedantic: copy nothing + util.selectiveDeepExtend([], a, b); + assert(a.color !== undefined && a.color === 'red'); + assert(a.notInSource === true); + assert(a.notInTarget === undefined); + + // pedantic: copy nonexistent property (nothing happens) + assert(b.iDontExist === undefined); + util.selectiveDeepExtend(['iDontExist'], a, b, true); + assert(a.iDontExist === undefined); + + // At this point nothing should have changed yet. + testAUnchanged(a); + + // Copy one property + util.selectiveDeepExtend(['color'], a, b); + assert(a.color !== undefined && a.color === 'green'); + + // Copy property Object + var sub = a.sub; + assert(sub.deleteThis === true); // pre + util.selectiveDeepExtend(['sub'], a, b); + assert(sub !== undefined); + assert(sub.enabled !== undefined && sub.enabled === false); + assert(sub.notInSource === true); + assert(sub.notInTarget === true); + assert(sub.deleteThis === null); + + + // Copy new Objects + assert(a.notInTarget === undefined); // pre + assert(a.subNotInTarget === undefined); // pre + util.selectiveDeepExtend(['notInTarget', 'subNotInTarget'], a, b); + assert(a.notInTarget === true); + assert(a.subNotInTarget.enabled === true); + + // Copy null objects + assert(a.deleteThis !== null); // pre + assert(a.subDeleteThis !== null); // pre + util.selectiveDeepExtend(['deleteThis', 'subDeleteThis'], a, b); + + // NOTE: if allowDeletion === false, null values are copied over! + // This is due to existing logic; it might not be the intention and hence a bug + assert(a.deleteThis === null); + assert(a.subDeleteThis === null); + }); + + + it('performs selectiveDeepExtend() as advertized with deletion', function () { + var a = this.a; + var b = this.b; + + // Only test expected differences here with test allowDeletion === false + + // Copy object property with properties to be deleted + var sub = a.sub; + assert(sub.deleteThis === true); // pre + util.selectiveDeepExtend(['sub'], a, b, true); + assert(sub.deleteThis === undefined); // should be deleted + + // Spot check on rest of properties in `a.sub` - there should have been copied + sub = a.sub; + assert(sub !== undefined); + assert(sub.enabled !== undefined && sub.enabled === false); + assert(sub.notInSource === true); + assert(sub.notInTarget === true); + + // Copy null objects + assert(a.deleteThis === true); // pre + assert(a.subDeleteThis !== undefined); // pre + assert(a.subDeleteThis.enabled === true); // pre + util.selectiveDeepExtend(['deleteThis', 'subDeleteThis'], a, b, true); + assert(a.deleteThis === undefined); // should be deleted + assert(a.subDeleteThis === undefined); // should be deleted + }); + + + it('performs selectiveNotDeepExtend() as advertized', function () { + var a = this.a; + var b = this.b; + + // Exclude all properties, nothing copied + util.selectiveNotDeepExtend(Object.keys(b), a, b); + testAUnchanged(a); + + // Exclude nothing, everything copied + util.selectiveNotDeepExtend([], a, b); + checkExtended(a, b, true); + + // Exclude some + a = initA(); + assert(a.notInTarget === undefined); // pre + assert(a.subNotInTarget === undefined); // pre + util.selectiveNotDeepExtend(['notInTarget', 'subNotInTarget'], a, b); + assert(a.notInTarget === undefined); // not copied + assert(a.subNotInTarget === undefined); // not copied + assert(a.sub.notInTarget === true); // copied! + }); + + + it('performs selectiveNotDeepExtend() as advertized with deletion', function () { + var a = this.a; + var b = this.b; + + // Exclude all properties, nothing copied + util.selectiveNotDeepExtend(Object.keys(b), a, b, true); + testAUnchanged(a); + + // Exclude nothing, everything copied and some deleted + util.selectiveNotDeepExtend([], a, b, true); + checkExtended(a, b, true); + + // Exclude some + a = initA(); + assert(a.notInTarget === undefined); // pre + assert(a.subNotInTarget === undefined); // pre + assert(a.deleteThis === true); // pre + assert(a.subDeleteThis !== undefined); // pre + assert(a.sub.deleteThis === true); // pre + assert(a.subDeleteThis.enabled === true); // pre + util.selectiveNotDeepExtend(['notInTarget', 'subNotInTarget'], a, b, true); + assert(a.deleteThis === undefined); // should be deleted + assert(a.sub.deleteThis !== undefined); // not deleted! Original logic, could be a bug + assert(a.subDeleteThis === undefined); // should be deleted + // Spot check: following should be same as allowDeletion === false + assert(a.notInTarget === undefined); // not copied + assert(a.subNotInTarget === undefined); // not copied + assert(a.sub.notInTarget === true); // copied! + }); + + + /** + * NOTE: parameter `protoExtend` not tested here! + */ + it('performs deepExtend() as advertized', function () { + var a = this.a; + var b = this.b; + + util.deepExtend(a, b); + checkExtended(a, b, true); + }); + + + /** + * NOTE: parameter `protoExtend` not tested here! + */ + it('performs deepExtend() as advertized with delete', function () { + var a = this.a; + var b = this.b; + + // Copy null objects + assert(a.deleteThis === true); // pre + assert(a.subDeleteThis !== undefined); // pre + assert(a.subDeleteThis.enabled === true); // pre + util.deepExtend(a, b, false, true); + checkExtended(a, b, true); // Normal copy should be good + assert(a.deleteThis === undefined); // should be deleted + assert(a.subDeleteThis === undefined); // should be deleted + assert(a.sub.deleteThis !== undefined); // not deleted!!! Original logic, could be a bug + }); +}); // extend routines + + // // The important thing with mergeOptions() is that 'enabled' is always set in target option. // -describe('util.mergeOptions', function () { +describe('mergeOptions', function () { it('handles good input without global options', function () { var options = { @@ -131,4 +466,6 @@ describe('util.mergeOptions', function () { util.mergeOptions(mergeTarget, options, 'alsoMissingEnabled', globalOptions); assert(mergeTarget.alsoMissingEnabled.enabled === true); }); -}); + +}); // mergeOptions +}); // util