Browse Source

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
revert-3409-performance
wimrijnders 7 years ago
committed by Yotam Berkowitz
parent
commit
b0481d3220
3 changed files with 509 additions and 121 deletions
  1. +111
    -119
      lib/util.js
  2. +59
    -0
      test/Network.test.js
  3. +339
    -2
      test/util.test.js

+ 111
- 119
lib/util.js View File

@ -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.<string>} 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.<string>} 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.<string>} props
* @param {Object} a
* @param {Object} b
* @param {boolean} [allowDeletion=false]
*
* @param {Array.<string>} 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

+ 59
- 0
test/Network.test.js View File

@ -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

+ 339
- 2
test/util.test.js View File

@ -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

Loading…
Cancel
Save