From ceafd211fef82c73f17888599b588752d875967c Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Mon, 24 Jun 2019 11:39:41 +0200 Subject: [PATCH 01/16] Added context to `CKEditorError`, changed `isCKEditorError`()` method to `is()`. --- src/ckeditorerror.js | 24 +++++++++++++++--------- tests/ckeditorerror.js | 30 +++++++++++++++++++----------- 2 files changed, 34 insertions(+), 20 deletions(-) diff --git a/src/ckeditorerror.js b/src/ckeditorerror.js index 4f4fb11..46b388b 100644 --- a/src/ckeditorerror.js +++ b/src/ckeditorerror.js @@ -32,11 +32,13 @@ export default class CKEditorError extends Error { * @param {String} message The error message in an `error-name: Error message.` format. * During the minification process the "Error message" part will be removed to limit the code size * and a link to this error documentation will be added to the `message`. + * @param {Object} context A context of the error by which the {@link module:watchdog/watchdog~Watchdog watchdog} + * is able to determine which editor crashed. It should be an editor instance or a property connected to it. * @param {Object} [data] Additional data describing the error. A stringified version of this object * will be appended to the error message, so the data are quickly visible in the console. The original * data object will also be later available under the {@link #data} property. */ - constructor( message, data ) { + constructor( message, context, data ) { message = attachLinkToDocumentation( message ); if ( data ) { @@ -46,26 +48,30 @@ export default class CKEditorError extends Error { super( message ); /** - * @member {String} + * @type {String} */ this.name = 'CKEditorError'; + /** + * A context of the error by which the Watchdog is able to determine which editor crashed. + * + * @type {Object} + */ + this.context = context; + /** * The additional error data passed to the constructor. Undefined if none was passed. * - * @member {Object|undefined} + * @type {Object|undefined} */ this.data = data; } /** - * Checks if error is an instance of CKEditorError class. - * - * @param {Object} error Object to check. - * @returns {Boolean} + * Checks if the error is of the `CKEditorError` type. */ - static isCKEditorError( error ) { - return error instanceof CKEditorError; + is( type ) { + return type === 'CKEditorError'; } } diff --git a/tests/ckeditorerror.js b/tests/ckeditorerror.js index 73706af..f0111bf 100644 --- a/tests/ckeditorerror.js +++ b/tests/ckeditorerror.js @@ -7,20 +7,20 @@ import { default as CKEditorError, DOCUMENTATION_URL } from '../src/ckeditorerro describe( 'CKEditorError', () => { it( 'inherits from Error', () => { - const error = new CKEditorError( 'foo' ); + const error = new CKEditorError( 'foo', {} ); expect( error ).to.be.an.instanceOf( Error ); expect( error ).to.be.an.instanceOf( CKEditorError ); } ); it( 'sets the name', () => { - const error = new CKEditorError( 'foo' ); + const error = new CKEditorError( 'foo', {} ); expect( error ).to.have.property( 'name', 'CKEditorError' ); } ); it( 'sets the message', () => { - const error = new CKEditorError( 'foo' ); + const error = new CKEditorError( 'foo', {} ); expect( error ).to.have.property( 'message', 'foo' ); expect( error.data ).to.be.undefined; @@ -28,12 +28,20 @@ describe( 'CKEditorError', () => { it( 'sets the message and data', () => { const data = { bar: 1 }; - const error = new CKEditorError( 'foo', data ); + const error = new CKEditorError( 'foo', {}, data ); expect( error ).to.have.property( 'message', 'foo {"bar":1}' ); expect( error ).to.have.property( 'data', data ); } ); + it( 'sets the context of the error', () => { + const data = { bar: 1 }; + const editor = {}; + const error = new CKEditorError( 'foo', editor, data ); + + expect( error.context ).to.equal( editor ); + } ); + it( 'appends stringified data to the message', () => { class Foo { constructor() { @@ -46,14 +54,14 @@ describe( 'CKEditorError', () => { bom: new Foo(), bim: 10 }; - const error = new CKEditorError( 'foo', data ); + const error = new CKEditorError( 'foo', {}, data ); expect( error ).to.have.property( 'message', 'foo {"bar":"a","bom":{"x":1},"bim":10}' ); expect( error ).to.have.property( 'data', data ); } ); it( 'contains a link which leads to the documentation', () => { - const error = new CKEditorError( 'model-schema-no-item: Specified item cannot be found.' ); + const error = new CKEditorError( 'model-schema-no-item: Specified item cannot be found.', {} ); const errorMessage = 'model-schema-no-item: Specified item cannot be found. ' + `Read more: ${ DOCUMENTATION_URL }#error-model-schema-no-item\n`; @@ -62,7 +70,7 @@ describe( 'CKEditorError', () => { } ); it( 'link to documentation is added before the additional data message', () => { - const error = new CKEditorError( 'model-schema-no-item: Specified item cannot be found.', { foo: 1, bar: 2 } ); + const error = new CKEditorError( 'model-schema-no-item: Specified item cannot be found.', {}, { foo: 1, bar: 2 } ); const errorMessage = 'model-schema-no-item: Specified item cannot be found. ' + `Read more: ${ DOCUMENTATION_URL }#error-model-schema-no-item\n ` + @@ -71,13 +79,13 @@ describe( 'CKEditorError', () => { expect( error ).to.have.property( 'message', errorMessage ); } ); - describe( 'isCKEditorError', () => { + describe( 'is()', () => { it( 'checks if error is an instance of CKEditorError', () => { - const ckeditorError = new CKEditorError( 'foo' ); + const ckeditorError = new CKEditorError( 'foo', {} ); const regularError = new Error( 'foo' ); - expect( CKEditorError.isCKEditorError( ckeditorError ) ).to.be.true; - expect( CKEditorError.isCKEditorError( regularError ) ).to.be.false; + expect( ( !!ckeditorError.is && ckeditorError.is( 'CKEditorError' ) ) ).to.be.true; + expect( ( !!regularError.is && regularError.is( 'CKEditorError' ) ) ).to.be.false; } ); } ); } ); From efced2d8f3ade1a64c678636fe261ba72e4d36f4 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Mon, 24 Jun 2019 16:15:09 +0200 Subject: [PATCH 02/16] Added support for the null as a context. --- src/ckeditorerror.js | 7 ++++--- tests/ckeditorerror.js | 14 +++++++------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/ckeditorerror.js b/src/ckeditorerror.js index 46b388b..2b2382d 100644 --- a/src/ckeditorerror.js +++ b/src/ckeditorerror.js @@ -32,8 +32,9 @@ export default class CKEditorError extends Error { * @param {String} message The error message in an `error-name: Error message.` format. * During the minification process the "Error message" part will be removed to limit the code size * and a link to this error documentation will be added to the `message`. - * @param {Object} context A context of the error by which the {@link module:watchdog/watchdog~Watchdog watchdog} - * is able to determine which editor crashed. It should be an editor instance or a property connected to it. + * @param {Object|null} context A context of the error by which the {@link module:watchdog/watchdog~Watchdog watchdog} + * is able to determine which editor crashed. It should be an editor instance or a property connected to it. It can be also + * a `null` value if the editor should not be restarted in case of the error (e.g. during the editor initialization). * @param {Object} [data] Additional data describing the error. A stringified version of this object * will be appended to the error message, so the data are quickly visible in the console. The original * data object will also be later available under the {@link #data} property. @@ -55,7 +56,7 @@ export default class CKEditorError extends Error { /** * A context of the error by which the Watchdog is able to determine which editor crashed. * - * @type {Object} + * @type {Object|null} */ this.context = context; diff --git a/tests/ckeditorerror.js b/tests/ckeditorerror.js index f0111bf..c2eb63d 100644 --- a/tests/ckeditorerror.js +++ b/tests/ckeditorerror.js @@ -14,13 +14,13 @@ describe( 'CKEditorError', () => { } ); it( 'sets the name', () => { - const error = new CKEditorError( 'foo', {} ); + const error = new CKEditorError( 'foo', null ); expect( error ).to.have.property( 'name', 'CKEditorError' ); } ); it( 'sets the message', () => { - const error = new CKEditorError( 'foo', {} ); + const error = new CKEditorError( 'foo', null ); expect( error ).to.have.property( 'message', 'foo' ); expect( error.data ).to.be.undefined; @@ -28,7 +28,7 @@ describe( 'CKEditorError', () => { it( 'sets the message and data', () => { const data = { bar: 1 }; - const error = new CKEditorError( 'foo', {}, data ); + const error = new CKEditorError( 'foo', null, data ); expect( error ).to.have.property( 'message', 'foo {"bar":1}' ); expect( error ).to.have.property( 'data', data ); @@ -54,14 +54,14 @@ describe( 'CKEditorError', () => { bom: new Foo(), bim: 10 }; - const error = new CKEditorError( 'foo', {}, data ); + const error = new CKEditorError( 'foo', null, data ); expect( error ).to.have.property( 'message', 'foo {"bar":"a","bom":{"x":1},"bim":10}' ); expect( error ).to.have.property( 'data', data ); } ); it( 'contains a link which leads to the documentation', () => { - const error = new CKEditorError( 'model-schema-no-item: Specified item cannot be found.', {} ); + const error = new CKEditorError( 'model-schema-no-item: Specified item cannot be found.', null ); const errorMessage = 'model-schema-no-item: Specified item cannot be found. ' + `Read more: ${ DOCUMENTATION_URL }#error-model-schema-no-item\n`; @@ -70,7 +70,7 @@ describe( 'CKEditorError', () => { } ); it( 'link to documentation is added before the additional data message', () => { - const error = new CKEditorError( 'model-schema-no-item: Specified item cannot be found.', {}, { foo: 1, bar: 2 } ); + const error = new CKEditorError( 'model-schema-no-item: Specified item cannot be found.', null, { foo: 1, bar: 2 } ); const errorMessage = 'model-schema-no-item: Specified item cannot be found. ' + `Read more: ${ DOCUMENTATION_URL }#error-model-schema-no-item\n ` + @@ -81,7 +81,7 @@ describe( 'CKEditorError', () => { describe( 'is()', () => { it( 'checks if error is an instance of CKEditorError', () => { - const ckeditorError = new CKEditorError( 'foo', {} ); + const ckeditorError = new CKEditorError( 'foo', null ); const regularError = new Error( 'foo' ); expect( ( !!ckeditorError.is && ckeditorError.is( 'CKEditorError' ) ) ).to.be.true; From edc598ba4a132fb9b36c316e9b063e63c3997db7 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Mon, 24 Jun 2019 16:16:23 +0200 Subject: [PATCH 03/16] WIP - implementing `areConnectedThroughProperties` util. --- src/areconnectedthroughproperties.js | 91 ++++++++++++++++++++++++++++ 1 file changed, 91 insertions(+) create mode 100644 src/areconnectedthroughproperties.js diff --git a/src/areconnectedthroughproperties.js b/src/areconnectedthroughproperties.js new file mode 100644 index 0000000..23400b7 --- /dev/null +++ b/src/areconnectedthroughproperties.js @@ -0,0 +1,91 @@ +/** + * @license Copyright (c) 2003-2019, CKSource - Frederico Knabben. All rights reserved. + * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license + */ + +/** + * @module utils/arestructuresconnected + */ + +/* globals EventTarget */ + +/** + * Traverses both structures to find out whether there's some object that is shared between both structures. + * + * @param {*} str1 + * @param {*} str2 + */ +export default function areConnectedThroughProperties( str1, str2 ) { + if ( str1 === str2 && isObject( str1 ) ) { + return true; + } + + const subNodes1 = getSubNodes( str1 ); + const subNodes2 = getSubNodes( str2 ); + + for ( const node of subNodes1 ) { + if ( subNodes2.has( node ) ) { + return true; + } + } + + return false; +} + +// Traverses JS structure and stores all sub-nodes, including the head node. +// It walks into each iterable structures with the `try catch` block to omit errors that might be thrown during +// tree walking. All primitives, functions and built-ins are skipped. +function getSubNodes( head ) { + const nodes = [ head ]; + + // Nodes are stored to prevent infinite looping. + const subNodes = new Set(); + + while ( nodes.length > 0 ) { + const node = nodes.shift(); + + if ( subNodes.has( node ) || shouldNodeBeSkipped( node ) ) { + continue; + } + + subNodes.add( node ); + + // Handle arrays, maps, sets, custom collections that implements `[ Symbol.iterator ]()`, etc. + if ( node[ Symbol.iterator ] ) { + // The custom editor iterators might cause some problems if the editor is crashed. + try { + nodes.push( ...node ); + } catch ( err ) { + // eslint-disable-line no-empty + } + } else { + nodes.push( ...Object.values( node ) ); + } + } + + return subNodes; +} + +function shouldNodeBeSkipped( node ) { + const type = Object.prototype.toString.call( node ); + + return ( + type === '[object Number]' || + type === '[object Boolean]' || + type === '[object String]' || + type === '[object Symbol]' || + type === '[object Function]' || + type === '[object Date]' || + type === '[object RegExp]' || + + node === undefined || + node === null || + + // Skip native DOM objects, e.g. Window, nodes, events, etc. + node instanceof EventTarget + ); +} + +function isObject( structure ) { + return typeof structure === 'object' && structure !== null; +} From b0b71eb1e676ebe7290e0a877e2e69ea639dff60 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Tue, 25 Jun 2019 23:43:43 +0200 Subject: [PATCH 04/16] Added tests for the `areConnectedThroughProperties()` util. --- src/areconnectedthroughproperties.js | 5 +- tests/areconnectedthroughproperties.js | 214 +++++++++++++++++++++++++ tests/ckeditorerror.js | 2 +- 3 files changed, 218 insertions(+), 3 deletions(-) create mode 100644 tests/areconnectedthroughproperties.js diff --git a/src/areconnectedthroughproperties.js b/src/areconnectedthroughproperties.js index 23400b7..ffadda2 100644 --- a/src/areconnectedthroughproperties.js +++ b/src/areconnectedthroughproperties.js @@ -7,7 +7,7 @@ * @module utils/arestructuresconnected */ -/* globals EventTarget */ +/* globals EventTarget, Event */ /** * Traverses both structures to find out whether there's some object that is shared between both structures. @@ -82,7 +82,8 @@ function shouldNodeBeSkipped( node ) { node === null || // Skip native DOM objects, e.g. Window, nodes, events, etc. - node instanceof EventTarget + node instanceof EventTarget || + node instanceof Event ); } diff --git a/tests/areconnectedthroughproperties.js b/tests/areconnectedthroughproperties.js new file mode 100644 index 0000000..5beb2b4 --- /dev/null +++ b/tests/areconnectedthroughproperties.js @@ -0,0 +1,214 @@ +/** + * @license Copyright (c) 2003-2019, CKSource - Frederico Knabben. All rights reserved. + * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license + */ + +/* globals window, document, Event */ + +import areConnectedThroughProperties from '../src/areconnectedthroughproperties'; + +describe( 'areConnectedThroughProperties()', () => { + it( 'should return `false` if one of the value is primitive #1', () => { + const el1 = [ 'foo' ]; + const el2 = 'foo'; + + expect( areConnectedThroughProperties( el1, el2 ) ).to.be.false; + } ); + + it( 'should return `false` if one of the value is primitive #2', () => { + const el1 = 0; + const el2 = [ 0 ]; + + expect( areConnectedThroughProperties( el1, el2 ) ).to.be.false; + } ); + + it( 'should return `false` if both of the values are primitives', () => { + const el1 = null; + const el2 = null; + + expect( areConnectedThroughProperties( el1, el2 ) ).to.be.false; + } ); + + it( 'should return `false` if both values are plain objects', () => { + const el1 = {}; + const el2 = {}; + + expect( areConnectedThroughProperties( el1, el2 ) ).to.be.false; + } ); + + it( 'should return `true` if both objects references to the same object', () => { + const el1 = {}; + const el2 = el1; + + expect( areConnectedThroughProperties( el1, el2 ) ).to.be.true; + } ); + + it( 'should return `true` if both values share a common reference #1', () => { + const foo = {}; + const el1 = { foo }; + const el2 = { foo }; + + expect( areConnectedThroughProperties( el1, el2 ) ).to.be.true; + } ); + + it( 'should return `true` if both values share a common reference #2', () => { + const foo = []; + const el1 = [ foo ]; + const el2 = [ foo ]; + + expect( areConnectedThroughProperties( el1, el2 ) ).to.be.true; + } ); + + it( 'should return `true` if the first structure is deep inside the second structure', () => { + const el1 = {}; + + const el2 = { + foo: 1, + bar: [ 1, 2, 3, new Map( [ + [ {}, new Set( [ 1, 2, 3 ] ) ], + [ undefined, new Set( [ + Symbol( 'foo' ), + null, + { x: [ el1 ] } + ] ) ] + ] ) ] + }; + + expect( areConnectedThroughProperties( el1, el2 ) ).to.be.true; + } ); + + it( 'should return `true` if the second structure is deep inside the first structure', () => { + const el2 = {}; + + const el1 = { + foo: 1, + bar: [ 1, 2, 3, new Map( [ + [ {}, new Set( [ 1, 2, 3 ] ) ], + [ undefined, new Set( [ + Symbol( 'foo' ), + null, + { x: [ el2 ] } + ] ) ] + ] ) ] + }; + + expect( areConnectedThroughProperties( el1, el2 ) ).to.be.true; + } ); + + it( 'should return `true` if both structures have a common reference', () => { + const foo = {}; + + const el1 = { + foo: 1, + bar: [ 1, 2, 3, new Map( [ + [ {}, new Set( [ 1, 2, 3 ] ) ], + [ undefined, new Set( [ + Symbol( 'foo' ), + null, + { x: [ foo ] } + ] ) ] + ] ) ] + }; + + const el2 = { + foo: 1, + bar: [ 1, 2, 3, new Map( [ + [ {}, new Set( [ 1, 2, 3 ] ) ], + [ undefined, new Set( [ + Symbol( 'foo' ), + null, + { x: [ foo ] } + ] ) ] + ] ) ] + }; + + expect( areConnectedThroughProperties( el1, el2 ) ).to.be.true; + } ); + + it( 'should return `false` if the structures is not connected #1', () => { + const el1 = {}; + + const el2 = { + foo: 1, + bar: [ 1, 2, 3, new Map( [ + [ {}, new Set( [ 1, 2, 3 ] ) ], + [ undefined, new Set( [ + Symbol( 'foo' ), + null, + { x: [] } + ] ) ] + ] ) ] + }; + + expect( areConnectedThroughProperties( el1, el2 ) ).to.be.false; + } ); + + it( 'should return `false` if the structures is not connected #2', () => { + const el1 = { + foo: 1, + bar: [ 1, 2, 3, new Map( [ + [ {}, new Set( [ 1, 2, 3 ] ) ], + [ undefined, new Set( [ + Symbol( 'foo' ), + null, + { x: [] } + ] ) ] + ] ) ] + }; + + const el2 = { + foo: 1, + bar: [ 1, 2, 3, new Map( [ + [ {}, new Set( [ 1, 2, 3 ] ) ], + [ undefined, new Set( [ + Symbol( 'foo' ), + null, + { x: [] } + ] ) ] + ] ) ] + }; + + expect( areConnectedThroughProperties( el1, el2 ) ).to.be.false; + } ); + + it( 'should work well with nested objects #1', () => { + const el1 = {}; + el1.foo = el1; + + const el2 = {}; + el2.foo = el2; + + expect( areConnectedThroughProperties( el1, el2 ) ).to.be.false; + } ); + + it( 'should work well with nested objects #2', () => { + const el1 = {}; + el1.foo = el1; + + const el2 = {}; + el2.foo = { + foo: el2, + bar: el1 + }; + + expect( areConnectedThroughProperties( el1, el2 ) ).to.be.true; + } ); + + it( 'should skip DOM objects', () => { + const evt = new Event( 'click' ); + const el1 = { window, document, evt }; + const el2 = { window, document, evt }; + + expect( areConnectedThroughProperties( el1, el2 ) ).to.be.false; + } ); + + it( 'should skip date and regexp objects', () => { + const date = new Date(); + const regexp = /123/; + + const el1 = { date, regexp }; + const el2 = { date, regexp }; + + expect( areConnectedThroughProperties( el1, el2 ) ).to.be.false; + } ); +} ); diff --git a/tests/ckeditorerror.js b/tests/ckeditorerror.js index c2eb63d..9d56410 100644 --- a/tests/ckeditorerror.js +++ b/tests/ckeditorerror.js @@ -7,7 +7,7 @@ import { default as CKEditorError, DOCUMENTATION_URL } from '../src/ckeditorerro describe( 'CKEditorError', () => { it( 'inherits from Error', () => { - const error = new CKEditorError( 'foo', {} ); + const error = new CKEditorError( 'foo', null ); expect( error ).to.be.an.instanceOf( Error ); expect( error ).to.be.an.instanceOf( CKEditorError ); From 11ae39a255cdaae71fbd6ff74763caaf6daad8d8 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Tue, 25 Jun 2019 23:46:53 +0200 Subject: [PATCH 05/16] Fixed wording. --- tests/_utils/utils.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/_utils/utils.js b/tests/_utils/utils.js index b93ddfe..3a7aa49 100644 --- a/tests/_utils/utils.js +++ b/tests/_utils/utils.js @@ -48,7 +48,7 @@ const utils = { }, /** - * Checkes wether observable properties are properly bound to each other. + * Checks whether observable properties are properly bound to each other. * * Syntax given that observable `A` is bound to observables [`B`, `C`, ...]: * From 623a19dac548cc591f228452f18a3d366f886739 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Wed, 26 Jun 2019 17:26:04 +0200 Subject: [PATCH 06/16] Added `expectToThrowCKEditorError` and `assertCKEditorError` test utils. --- tests/_utils/utils.js | 164 ++++++++++++++++++++++++------------------ 1 file changed, 95 insertions(+), 69 deletions(-) diff --git a/tests/_utils/utils.js b/tests/_utils/utils.js index 3a7aa49..d2e855a 100644 --- a/tests/_utils/utils.js +++ b/tests/_utils/utils.js @@ -6,84 +6,110 @@ /* global console:false */ import EmitterMixin from '../../src/emittermixin'; +import CKEditorError from '../../src/ckeditorerror'; +import areConnectedThroughProperties from '../../src/areconnectedthroughproperties'; -const utils = { - /** - * Creates an instance inheriting from {@link utils.EmitterMixin} with one additional method `observe()`. - * It allows observing changes to attributes in objects being {@link utils.Observable observable}. - * - * The `observe()` method accepts: - * - * * `{String} observableName` – Identifier for the observable object. E.g. `"Editable"` when - * you observe one of editor's editables. This name will be displayed on the console. - * * `{utils.Observable observable} – The object to observe. - * * `{Array.} filterNames` – Array of propery names to be observed. - * - * Typical usage: - * - * const observer = utils.createObserver(); - * observer.observe( 'Editable', editor.editables.current ); - * - * // Stop listening (method from the EmitterMixin): - * observer.stopListening(); - * - * @returns {Emitter} The observer. - */ - createObserver() { - const observer = Object.create( EmitterMixin, { - observe: { - value: function observe( observableName, observable, filterNames ) { - observer.listenTo( observable, 'change', ( evt, propertyName, value, oldValue ) => { - if ( !filterNames || filterNames.includes( propertyName ) ) { - console.log( `[Change in ${ observableName }] ${ propertyName } = '${ value }' (was '${ oldValue }')` ); - } - } ); +/** + * Creates an instance inheriting from {@link utils.EmitterMixin} with one additional method `observe()`. + * It allows observing changes to attributes in objects being {@link utils.Observable observable}. + * + * The `observe()` method accepts: + * + * * `{String} observableName` – Identifier for the observable object. E.g. `"Editable"` when + * you observe one of editor's editables. This name will be displayed on the console. + * * `{utils.Observable observable} – The object to observe. + * * `{Array.} filterNames` – Array of propery names to be observed. + * + * Typical usage: + * + * const observer = utils.createObserver(); + * observer.observe( 'Editable', editor.editables.current ); + * + * // Stop listening (method from the EmitterMixin): + * observer.stopListening(); + * + * @returns {Emitter} The observer. + */ +export function createObserver() { + const observer = Object.create( EmitterMixin, { + observe: { + value: function observe( observableName, observable, filterNames ) { + observer.listenTo( observable, 'change', ( evt, propertyName, value, oldValue ) => { + if ( !filterNames || filterNames.includes( propertyName ) ) { + console.log( `[Change in ${ observableName }] ${ propertyName } = '${ value }' (was '${ oldValue }')` ); + } + } ); - return observer; - } + return observer; } - } ); + } + } ); - return observer; - }, + return observer; +} - /** - * Checks whether observable properties are properly bound to each other. - * - * Syntax given that observable `A` is bound to observables [`B`, `C`, ...]: - * - * assertBinding( A, - * { initial `A` attributes }, - * [ - * [ B, { new `B` attributes } ], - * [ C, { new `C` attributes } ], - * ... - * ], - * { `A` attributes after [`B`, 'C', ...] changed } - * ); - */ - assertBinding( observable, stateBefore, data, stateAfter ) { - let key, boundObservable, attrs; +/** + * Checks whether observable properties are properly bound to each other. + * + * Syntax given that observable `A` is bound to observables [`B`, `C`, ...]: + * + * assertBinding( A, + * { initial `A` attributes }, + * [ + * [ B, { new `B` attributes } ], + * [ C, { new `C` attributes } ], + * ... + * ], + * { `A` attributes after [`B`, 'C', ...] changed } + * ); + */ +export function assertBinding( observable, stateBefore, data, stateAfter ) { + let key, boundObservable, attrs; - for ( key in stateBefore ) { - expect( observable[ key ] ).to.be.equal( stateBefore[ key ] ); - } + for ( key in stateBefore ) { + expect( observable[ key ] ).to.be.equal( stateBefore[ key ] ); + } - // Change attributes of bound observables. - for ( [ boundObservable, attrs ] of data ) { - for ( key in attrs ) { - if ( !boundObservable.hasOwnProperty( key ) ) { - boundObservable.set( key, attrs[ key ] ); - } else { - boundObservable[ key ] = attrs[ key ]; - } + // Change attributes of bound observables. + for ( [ boundObservable, attrs ] of data ) { + for ( key in attrs ) { + if ( !boundObservable.hasOwnProperty( key ) ) { + boundObservable.set( key, attrs[ key ] ); + } else { + boundObservable[ key ] = attrs[ key ]; } } + } - for ( key in stateAfter ) { - expect( observable[ key ] ).to.be.equal( stateAfter[ key ] ); - } + for ( key in stateAfter ) { + expect( observable[ key ] ).to.be.equal( stateAfter[ key ] ); + } +} + +export function expectToThrowCKEditorError( fn, message, editorThatShouldBeFindableFromContext ) { + let err = null; + + try { + fn(); + } catch ( _err ) { + err = _err; + + assertCKEditorError( err, message, editorThatShouldBeFindableFromContext ); } -}; -export default utils; + expect( err ).to.not.equal( null, 'Function did not throw any error' ); +} + +export function assertCKEditorError( err, message, editorThatShouldBeFindableFromContext ) { + expect( err ).to.be.instanceOf( CKEditorError ); + expect( err.message ).to.match( message, 'Error message does not match the provided one.' ); + + if ( editorThatShouldBeFindableFromContext === null ) { + expect( err.context ).to.equal( null, 'Error context was expected to be `null`' ); + } else { + expect( + areConnectedThroughProperties( editorThatShouldBeFindableFromContext, err.context ), + 'Editor cannot be find from the error context' + ); + } +} From b78be089ca086849c4262245c8aa7e2656a681db Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Thu, 27 Jun 2019 15:06:44 +0200 Subject: [PATCH 07/16] Added utils for testing CKEditor errors. --- tests/_utils-tests/utils.js | 101 +++++++++++++++++++----------------- tests/_utils/utils.js | 2 +- 2 files changed, 54 insertions(+), 49 deletions(-) diff --git a/tests/_utils-tests/utils.js b/tests/_utils-tests/utils.js index 3c10ea5..183e566 100644 --- a/tests/_utils-tests/utils.js +++ b/tests/_utils-tests/utils.js @@ -3,76 +3,81 @@ * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license */ -import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; -import utilsTestUtils from '../../tests/_utils/utils'; -import ObesrvableMixin from '../../src/observablemixin'; +import * as testUtils from '../../tests/_utils/utils'; +import ObservableMixin from '../../src/observablemixin'; import EmitterMixin from '../../src/emittermixin'; -describe( 'utilsTestUtils.createObserver()', () => { - let observable, observable2, observer; +describe( 'utils - testUtils', () => { + afterEach( () => { + sinon.restore(); + } ); - testUtils.createSinonSandbox(); + describe( 'createObserver()', () => { + let observable, observable2, observer; - beforeEach( () => { - observer = utilsTestUtils.createObserver(); + testUtils.createSinonSandbox(); - observable = Object.create( ObesrvableMixin ); - observable.set( { foo: 0, bar: 0 } ); + beforeEach( () => { + observer = testUtils.createObserver(); - observable2 = Object.create( ObesrvableMixin ); - observable2.set( { foo: 0, bar: 0 } ); - } ); + observable = Object.create( ObservableMixin ); + observable.set( { foo: 0, bar: 0 } ); - it( 'should create an observer', () => { - function Emitter() {} - Emitter.prototype = EmitterMixin; + observable2 = Object.create( ObservableMixin ); + observable2.set( { foo: 0, bar: 0 } ); + } ); - expect( observer ).to.be.instanceof( Emitter ); - expect( observer.observe ).is.a( 'function' ); - expect( observer.stopListening ).is.a( 'function' ); - } ); + it( 'should create an observer', () => { + function Emitter() { } + Emitter.prototype = EmitterMixin; - describe( 'Observer', () => { - /* global console:false */ + expect( observer ).to.be.instanceof( Emitter ); + expect( observer.observe ).is.a( 'function' ); + expect( observer.stopListening ).is.a( 'function' ); + } ); - it( 'logs changes in the observable', () => { - const spy = testUtils.sinon.stub( console, 'log' ); + describe( 'Observer', () => { + /* global console:false */ - observer.observe( 'Some observable', observable ); - observer.observe( 'Some observable 2', observable2 ); + it( 'logs changes in the observable', () => { + const spy = sinon.stub( console, 'log' ); - observable.foo = 1; - expect( spy.callCount ).to.equal( 1 ); + observer.observe( 'Some observable', observable ); + observer.observe( 'Some observable 2', observable2 ); - observable.foo = 2; - observable2.bar = 3; - expect( spy.callCount ).to.equal( 3 ); - } ); + observable.foo = 1; + expect( spy.callCount ).to.equal( 1 ); - it( 'logs changes to specified properties', () => { - const spy = testUtils.sinon.stub( console, 'log' ); + observable.foo = 2; + observable2.bar = 3; + expect( spy.callCount ).to.equal( 3 ); + } ); - observer.observe( 'Some observable', observable, [ 'foo' ] ); + it( 'logs changes to specified properties', () => { + const spy = sinon.stub( console, 'log' ); - observable.foo = 1; - expect( spy.callCount ).to.equal( 1 ); + observer.observe( 'Some observable', observable, [ 'foo' ] ); - observable.bar = 1; - expect( spy.callCount ).to.equal( 1 ); - } ); + observable.foo = 1; + expect( spy.callCount ).to.equal( 1 ); + + observable.bar = 1; + expect( spy.callCount ).to.equal( 1 ); + } ); - it( 'stops listening when asked to do so', () => { - const spy = testUtils.sinon.stub( console, 'log' ); + it( 'stops listening when asked to do so', () => { + const spy = sinon.stub( console, 'log' ); - observer.observe( 'Some observable', observable ); + observer.observe( 'Some observable', observable ); - observable.foo = 1; - expect( spy.callCount ).to.equal( 1 ); + observable.foo = 1; + expect( spy.callCount ).to.equal( 1 ); - observer.stopListening(); + observer.stopListening(); - observable.foo = 2; - expect( spy.callCount ).to.equal( 1 ); + observable.foo = 2; + expect( spy.callCount ).to.equal( 1 ); + } ); } ); } ); } ); diff --git a/tests/_utils/utils.js b/tests/_utils/utils.js index d2e855a..84cb9ba 100644 --- a/tests/_utils/utils.js +++ b/tests/_utils/utils.js @@ -18,7 +18,7 @@ import areConnectedThroughProperties from '../../src/areconnectedthroughproperti * * `{String} observableName` – Identifier for the observable object. E.g. `"Editable"` when * you observe one of editor's editables. This name will be displayed on the console. * * `{utils.Observable observable} – The object to observe. - * * `{Array.} filterNames` – Array of propery names to be observed. + * * `{Array.} filterNames` – Array of property names to be observed. * * Typical usage: * From 0fb858230a1bb4f4ee544a3d85bf7d7481976a7a Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Fri, 28 Jun 2019 09:24:44 +0200 Subject: [PATCH 08/16] WIP - changing errors. --- src/focustracker.js | 2 +- tests/_utils-tests/utils.js | 5 +- tests/_utils/utils.js | 13 +++-- tests/collection.js | 42 +++++++-------- tests/focustracker.js | 5 +- tests/keyboard.js | 9 ++-- tests/observablemixin.js | 105 ++++++++++++++++++------------------ 7 files changed, 93 insertions(+), 88 deletions(-) diff --git a/src/focustracker.js b/src/focustracker.js index b058e5e..99fb6b1 100644 --- a/src/focustracker.js +++ b/src/focustracker.js @@ -75,7 +75,7 @@ export default class FocusTracker { */ add( element ) { if ( this._elements.has( element ) ) { - throw new CKEditorError( 'focusTracker-add-element-already-exist' ); + throw new CKEditorError( 'focusTracker-add-element-already-exist', this ); } this.listenTo( element, 'focus', () => this._focus( element ), { useCapture: true } ); diff --git a/tests/_utils-tests/utils.js b/tests/_utils-tests/utils.js index 183e566..ea193f6 100644 --- a/tests/_utils-tests/utils.js +++ b/tests/_utils-tests/utils.js @@ -3,9 +3,10 @@ * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license */ -import * as testUtils from '../../tests/_utils/utils'; +import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; import ObservableMixin from '../../src/observablemixin'; import EmitterMixin from '../../src/emittermixin'; +import { createObserver } from '../_utils/utils'; describe( 'utils - testUtils', () => { afterEach( () => { @@ -18,7 +19,7 @@ describe( 'utils - testUtils', () => { testUtils.createSinonSandbox(); beforeEach( () => { - observer = testUtils.createObserver(); + observer = createObserver(); observable = Object.create( ObservableMixin ); observable.set( { foo: 0, bar: 0 } ); diff --git a/tests/_utils/utils.js b/tests/_utils/utils.js index 84cb9ba..5a4cd50 100644 --- a/tests/_utils/utils.js +++ b/tests/_utils/utils.js @@ -86,7 +86,7 @@ export function assertBinding( observable, stateBefore, data, stateAfter ) { } } -export function expectToThrowCKEditorError( fn, message, editorThatShouldBeFindableFromContext ) { +export function expectToThrowCKEditorError( fn, message, editorThatShouldBeFindableFromContext, data ) { let err = null; try { @@ -94,22 +94,27 @@ export function expectToThrowCKEditorError( fn, message, editorThatShouldBeFinda } catch ( _err ) { err = _err; - assertCKEditorError( err, message, editorThatShouldBeFindableFromContext ); + assertCKEditorError( err, message, editorThatShouldBeFindableFromContext, data ); } expect( err ).to.not.equal( null, 'Function did not throw any error' ); } -export function assertCKEditorError( err, message, editorThatShouldBeFindableFromContext ) { +export function assertCKEditorError( err, message, editorThatShouldBeFindableFromContext, data ) { expect( err ).to.be.instanceOf( CKEditorError ); expect( err.message ).to.match( message, 'Error message does not match the provided one.' ); + // TODO: The `editorThatShouldBeFindableFromContext` is optional but should be required in the future. if ( editorThatShouldBeFindableFromContext === null ) { expect( err.context ).to.equal( null, 'Error context was expected to be `null`' ); - } else { + } else if ( editorThatShouldBeFindableFromContext !== undefined ) { expect( areConnectedThroughProperties( editorThatShouldBeFindableFromContext, err.context ), 'Editor cannot be find from the error context' ); } + + if ( data ) { + expect( err.data ).to.deep.equal( data ); + } } diff --git a/tests/collection.js b/tests/collection.js index 68bb7f9..5da9902 100644 --- a/tests/collection.js +++ b/tests/collection.js @@ -5,7 +5,7 @@ import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; import Collection from '../src/collection'; -import CKEditorError from '../src/ckeditorerror'; +import { expectToThrowCKEditorError } from '../tests/_utils/utils'; function getItem( id, idProperty ) { idProperty = idProperty || 'id'; @@ -165,17 +165,17 @@ describe( 'Collection', () => { collection.add( item1 ); - expect( () => { + expectToThrowCKEditorError( () => { collection.add( item2 ); - } ).to.throw( CKEditorError, /^collection-add-item-already-exists/ ); + }, /^collection-add-item-already-exists/ ); } ); it( 'should throw when item\'s id is not a string', () => { const item = { id: 1 }; - expect( () => { + expectToThrowCKEditorError( () => { collection.add( item ); - } ).to.throw( CKEditorError, /^collection-add-invalid-id/ ); + }, /^collection-add-invalid-id/ ); } ); it( @@ -271,11 +271,11 @@ describe( 'Collection', () => { collection.add( item1 ); - expect( () => { + expectToThrowCKEditorError( () => { collection.add( item2, -1 ); } ).to.throw( /^collection-add-item-invalid-index/ ); - expect( () => { + expectToThrowCKEditorError( () => { collection.add( item2, 2 ); } ).to.throw( /^collection-add-item-invalid-index/ ); @@ -315,9 +315,9 @@ describe( 'Collection', () => { } ); it( 'should throw if neither string or number given', () => { - expect( () => { + expectToThrowCKEditorError( () => { collection.get( true ); - } ).to.throw( CKEditorError, /^collection-get-invalid-arg/ ); + }, /^collection-get-invalid-arg/ ); } ); } ); @@ -484,9 +484,9 @@ describe( 'Collection', () => { it( 'should throw an error on invalid index', () => { collection.add( getItem( 'foo' ) ); - expect( () => { + expectToThrowCKEditorError( () => { collection.remove( 1 ); - } ).to.throw( CKEditorError, /^collection-remove-404/ ); + }, /^collection-remove-404/ ); expect( collection ).to.have.length( 1 ); } ); @@ -494,9 +494,9 @@ describe( 'Collection', () => { it( 'should throw an error on invalid id', () => { collection.add( getItem( 'foo' ) ); - expect( () => { + expectToThrowCKEditorError( () => { collection.remove( 'bar' ); - } ).to.throw( CKEditorError, /^collection-remove-404/ ); + }, /^collection-remove-404/ ); expect( collection ).to.have.length( 1 ); } ); @@ -504,9 +504,9 @@ describe( 'Collection', () => { it( 'should throw an error on invalid model', () => { collection.add( getItem( 'foo' ) ); - expect( () => { + expectToThrowCKEditorError( () => { collection.remove( getItem( 'bar' ) ); - } ).to.throw( CKEditorError, /^collection-remove-404/ ); + }, /^collection-remove-404/ ); expect( collection ).to.have.length( 1 ); } ); @@ -522,7 +522,7 @@ describe( 'Collection', () => { sinon.assert.calledWithExactly( spy, callback, ctx ); expect( ret ).to.deep.equal( [ 'foo' ], 'ret value was forwarded' ); - function callback() {} + function callback() { } } ); } ); @@ -538,7 +538,7 @@ describe( 'Collection', () => { sinon.assert.calledWithExactly( spy, callback, ctx ); expect( ret ).to.equal( needl, 'ret value was forwarded' ); - function callback() {} + function callback() { } } ); } ); @@ -555,7 +555,7 @@ describe( 'Collection', () => { sinon.assert.calledWithExactly( spy, callback, ctx ); expect( ret ).to.deep.equal( [ needl ], 'ret value was forwarded' ); - function callback() {} + function callback() { } } ); } ); @@ -607,9 +607,9 @@ describe( 'Collection', () => { it( 'throws when binding more than once', () => { collection.bindTo( {} ); - expect( () => { + expectToThrowCKEditorError( () => { collection.bindTo( {} ); - } ).to.throw( CKEditorError, /^collection-bind-to-rebind/ ); + }, /^collection-bind-to-rebind/ ); } ); it( 'provides "using()" and "as()" interfaces', () => { @@ -700,7 +700,7 @@ describe( 'Collection', () => { } ); it( 'does not chain', () => { - const returned = collection.bindTo( new Collection() ).using( () => {} ); + const returned = collection.bindTo( new Collection() ).using( () => { } ); expect( returned ).to.be.undefined; } ); diff --git a/tests/focustracker.js b/tests/focustracker.js index a8d8a6f..8a10fdc 100644 --- a/tests/focustracker.js +++ b/tests/focustracker.js @@ -9,6 +9,7 @@ import FocusTracker from '../src/focustracker'; import CKEditorError from '../src/ckeditorerror'; import global from '../src/dom/global'; import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; +import { expectToThrowCKEditorError } from './_utils/utils'; describe( 'FocusTracker', () => { let focusTracker, container, containerFirstInput, containerSecondInput; @@ -66,9 +67,9 @@ describe( 'FocusTracker', () => { it( 'should throw an error when element has been already added', () => { focusTracker.add( containerFirstInput ); - expect( () => { + expectToThrowCKEditorError( () => { focusTracker.add( containerFirstInput ); - } ).to.throw( CKEditorError, /focusTracker-add-element-already-exist/ ); + }, /focusTracker-add-element-already-exist/, null ); } ); describe( 'single element', () => { diff --git a/tests/keyboard.js b/tests/keyboard.js index 4aab017..af042ce 100644 --- a/tests/keyboard.js +++ b/tests/keyboard.js @@ -6,6 +6,7 @@ import env from '../src/env'; import { keyCodes, getCode, parseKeystroke, getEnvKeystrokeText } from '../src/keyboard'; import CKEditorError from '../src/ckeditorerror'; +import { expectToThrowCKEditorError } from './_utils/utils'; describe( 'Keyboard', () => { describe( 'keyCodes', () => { @@ -55,9 +56,9 @@ describe( 'Keyboard', () => { } ); it( 'throws when passed unknown key name', () => { - expect( () => { + expectToThrowCKEditorError( () => { getCode( 'foo' ); - } ).to.throw( CKEditorError, /^keyboard-unknown-key:/ ); + }, /^keyboard-unknown-key:/ ); } ); it( 'gets code of a keystroke info', () => { @@ -96,9 +97,9 @@ describe( 'Keyboard', () => { } ); it( 'throws on unknown name', () => { - expect( () => { + expectToThrowCKEditorError( () => { parseKeystroke( 'foo' ); - } ).to.throw( CKEditorError, /^keyboard-unknown-key:/ ); + }, /^keyboard-unknown-key:/ ); } ); } ); diff --git a/tests/observablemixin.js b/tests/observablemixin.js index 618f452..2d16744 100644 --- a/tests/observablemixin.js +++ b/tests/observablemixin.js @@ -4,15 +4,12 @@ */ import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; -import utilsTestUtils from '../tests/_utils/utils'; +import { assertBinding, expectToThrowCKEditorError } from '../tests/_utils/utils'; import ObservableMixin from '../src/observablemixin'; import EmitterMixin from '../src/emittermixin'; import EventInfo from '../src/eventinfo'; -import CKEditorError from '../src/ckeditorerror'; import mix from '../src/mix'; -const assertBinding = utilsTestUtils.assertBinding; - describe( 'ObservableMixin', () => { testUtils.createSinonSandbox(); @@ -222,9 +219,9 @@ describe( 'Observable', () => { it( 'should throw when overriding already existing property', () => { car.normalProperty = 1; - expect( () => { + expectToThrowCKEditorError( () => { car.set( 'normalProperty', 2 ); - } ).to.throw( CKEditorError, /^observable-set-cannot-override/ ); + }, /^observable-set-cannot-override/ ); expect( car ).to.have.property( 'normalProperty', 1 ); } ); @@ -236,9 +233,9 @@ describe( 'Observable', () => { car = new Car(); - expect( () => { + expectToThrowCKEditorError( () => { car.set( 'method', 2 ); - } ).to.throw( CKEditorError, /^observable-set-cannot-override/ ); + }, /^observable-set-cannot-override/ ); expect( car.method ).to.be.a( 'function' ); } ); @@ -274,30 +271,30 @@ describe( 'Observable', () => { } ); it( 'should throw when properties are not strings', () => { - expect( () => { + expectToThrowCKEditorError( () => { car.bind(); - } ).to.throw( CKEditorError, /observable-bind-wrong-properties/ ); + }, /observable-bind-wrong-properties/ ); - expect( () => { + expectToThrowCKEditorError( () => { car.bind( new Date() ); - } ).to.throw( CKEditorError, /observable-bind-wrong-properties/ ); + }, /observable-bind-wrong-properties/ ); - expect( () => { + expectToThrowCKEditorError( () => { car.bind( 'color', new Date() ); - } ).to.throw( CKEditorError, /observable-bind-wrong-properties/ ); + }, /observable-bind-wrong-properties/ ); } ); it( 'should throw when the same property is used than once', () => { - expect( () => { + expectToThrowCKEditorError( () => { car.bind( 'color', 'color' ); - } ).to.throw( CKEditorError, /observable-bind-duplicate-properties/ ); + }, /observable-bind-duplicate-properties/ ); } ); it( 'should throw when binding the same property more than once', () => { - expect( () => { + expectToThrowCKEditorError( () => { car.bind( 'color' ); car.bind( 'color' ); - } ).to.throw( CKEditorError, /observable-bind-rebind/ ); + }, /observable-bind-rebind/ ); } ); describe( 'to()', () => { @@ -308,11 +305,11 @@ describe( 'Observable', () => { } ); it( 'should throw when arguments are of invalid type - empty', () => { - expect( () => { + expectToThrowCKEditorError( () => { car = new Car(); car.bind( 'color' ).to(); - } ).to.throw( CKEditorError, /observable-bind-to-parse-error/ ); + }, /observable-bind-to-parse-error/ ); } ); it( 'should throw when binding multiple properties to multiple observables', () => { @@ -320,87 +317,87 @@ describe( 'Observable', () => { const car1 = new Car( { color: 'red', year: 1943 } ); const car2 = new Car( { color: 'yellow', year: 1932 } ); - expect( () => { + expectToThrowCKEditorError( () => { vehicle.bind( 'color', 'year' ).to( car1, 'color', car2, 'year' ); - } ).to.throw( CKEditorError, /observable-bind-to-no-callback/ ); + }, /observable-bind-to-no-callback/ ); - expect( () => { + expectToThrowCKEditorError( () => { vehicle = new Car(); vehicle.bind( 'color', 'year' ).to( car1, car2 ); - } ).to.throw( CKEditorError, /observable-bind-to-no-callback/ ); + }, /observable-bind-to-no-callback/ ); - expect( () => { + expectToThrowCKEditorError( () => { vehicle = new Car(); vehicle.bind( 'color', 'year' ).to( car1, car2, 'year' ); - } ).to.throw( CKEditorError, /observable-bind-to-no-callback/ ); + }, /observable-bind-to-no-callback/ ); - expect( () => { + expectToThrowCKEditorError( () => { vehicle = new Car(); vehicle.bind( 'color', 'year' ).to( car1, 'color', car2 ); - } ).to.throw( CKEditorError, /observable-bind-to-no-callback/ ); + }, /observable-bind-to-no-callback/ ); - expect( () => { + expectToThrowCKEditorError( () => { vehicle = new Car(); vehicle.bind( 'color', 'year', 'custom' ).to( car, car ); - } ).to.throw( CKEditorError, /observable-bind-to-no-callback/ ); + }, /observable-bind-to-no-callback/ ); } ); it( 'should throw when binding multiple properties but passed a callback', () => { let vehicle = new Car(); - expect( () => { + expectToThrowCKEditorError( () => { vehicle.bind( 'color', 'year' ).to( car, () => {} ); - } ).to.throw( CKEditorError, /observable-bind-to-extra-callback/ ); + }, /observable-bind-to-extra-callback/ ); - expect( () => { + expectToThrowCKEditorError( () => { vehicle = new Car(); vehicle.bind( 'color', 'year' ).to( car, car, () => {} ); - } ).to.throw( CKEditorError, /observable-bind-to-extra-callback/ ); + }, /observable-bind-to-extra-callback/ ); } ); it( 'should throw when binding a single property but multiple callbacks', () => { let vehicle = new Car(); - expect( () => { + expectToThrowCKEditorError( () => { vehicle.bind( 'color' ).to( car, () => {}, () => {} ); - } ).to.throw( CKEditorError, /observable-bind-to-parse-error/ ); + }, /observable-bind-to-parse-error/ ); - expect( () => { + expectToThrowCKEditorError( () => { vehicle = new Car(); vehicle.bind( 'color' ).to( car, car, () => {}, () => {} ); - } ).to.throw( CKEditorError, /observable-bind-to-parse-error/ ); + }, /observable-bind-to-parse-error/ ); } ); it( 'should throw when a number of properties does not match', () => { let vehicle = new Car(); - expect( () => { + expectToThrowCKEditorError( () => { vehicle.bind( 'color' ).to( car, 'color', 'year' ); - } ).to.throw( CKEditorError, /observable-bind-to-properties-length/ ); + }, /observable-bind-to-properties-length/ ); - expect( () => { + expectToThrowCKEditorError( () => { vehicle = new Car(); vehicle.bind( 'color', 'year' ).to( car, 'color' ); - } ).to.throw( CKEditorError, /observable-bind-to-properties-length/ ); + }, /observable-bind-to-properties-length/ ); - expect( () => { + expectToThrowCKEditorError( () => { vehicle = new Car(); vehicle.bind( 'color' ).to( car, 'color', 'year', () => {} ); - } ).to.throw( CKEditorError, /observable-bind-to-properties-length/ ); + }, /observable-bind-to-properties-length/ ); - expect( () => { + expectToThrowCKEditorError( () => { vehicle = new Car(); vehicle.bind( 'color' ).to( car, 'color', car, 'color', 'year', () => {} ); - } ).to.throw( CKEditorError, /observable-bind-to-properties-length/ ); + }, /observable-bind-to-properties-length/ ); } ); it( 'should work when properties don\'t exist in to() observable #1', () => { @@ -823,15 +820,15 @@ describe( 'Observable', () => { it( 'should throw when binding multiple properties', () => { let vehicle = new Car(); - expect( () => { + expectToThrowCKEditorError( () => { vehicle.bind( 'color', 'year' ).toMany( [ car ], 'foo', () => {} ); - } ).to.throw( CKEditorError, /observable-bind-to-many-not-one-binding/ ); + }, /observable-bind-to-many-not-one-binding/ ); - expect( () => { + expectToThrowCKEditorError( () => { vehicle = new Car(); vehicle.bind( 'color', 'year' ).to( car, car, () => {} ); - } ).to.throw( CKEditorError, /observable-bind-to-extra-callback/ ); + }, /observable-bind-to-extra-callback/ ); } ); it( 'binds observable property to collection property using callback', () => { @@ -880,9 +877,9 @@ describe( 'Observable', () => { } ); it( 'should throw when non-string property is passed', () => { - expect( () => { + expectToThrowCKEditorError( () => { car.unbind( new Date() ); - } ).to.throw( CKEditorError, /observable-unbind-wrong-properties/ ); + }, /observable-unbind-wrong-properties/ ); } ); it( 'should remove all bindings', () => { @@ -1054,9 +1051,9 @@ describe( 'Observable', () => { const foo = new Foo(); - expect( () => { + expectToThrowCKEditorError( () => { foo.decorate( 'method' ); - } ).to.throw( CKEditorError, /^observablemixin-cannot-decorate-undefined:/ ); + }, /^observablemixin-cannot-decorate-undefined:/ ); } ); } ); } ); From 7fecd710266e2ee1940afcd3da80a137eec902ab Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Mon, 1 Jul 2019 09:41:13 +0200 Subject: [PATCH 09/16] Fixed tests. Added missing API docs. --- tests/_utils/utils.js | 22 ++++++++++++++++++++++ tests/collection.js | 4 ++-- tests/focustracker.js | 3 +-- 3 files changed, 25 insertions(+), 4 deletions(-) diff --git a/tests/_utils/utils.js b/tests/_utils/utils.js index 5a4cd50..e98d85a 100644 --- a/tests/_utils/utils.js +++ b/tests/_utils/utils.js @@ -86,6 +86,16 @@ export function assertBinding( observable, stateBefore, data, stateAfter ) { } } +/** + * An assertion util to test whether the given function throws error that has correct message, + * data and whether the context of the error and the `editorThatShouldBeFindableFromContext` + * have common props (So the watchdog will be able to find the correct editor instance and restart it). + * + * @param {Function} fn Tested function that should throw a `CKEditorError`. + * @param {RegExp} message Expected message of the error. + * @param {*} editorThatShouldBeFindableFromContext An editor instance that should be findable from the error context. + * @param {Object} [data] Error data. + */ export function expectToThrowCKEditorError( fn, message, editorThatShouldBeFindableFromContext, data ) { let err = null; @@ -100,8 +110,20 @@ export function expectToThrowCKEditorError( fn, message, editorThatShouldBeFinda expect( err ).to.not.equal( null, 'Function did not throw any error' ); } +/** + * An assertion util to test whether a given error has correct message, data and whether the context of the + * error and the `editorThatShouldBeFindableFromContext` have common props (So the watchdog will be able to + * find the correct editor instance and restart it). + * + * @param {module:utils/ckeditorerror~CKEditorError} err The tested error. + * @param {RegExp} message Expected message of the error. + * @param {*} [editorThatShouldBeFindableFromContext] An editor instance that should be findable from the error context. + * @param {Object} [data] Error data. + */ export function assertCKEditorError( err, message, editorThatShouldBeFindableFromContext, data ) { expect( err ).to.be.instanceOf( CKEditorError ); + expect( message ).to.be.a( 'regexp', `Error message should be a string. Got ${ typeof message }.` ); + expect( err.message ).to.match( message, 'Error message does not match the provided one.' ); // TODO: The `editorThatShouldBeFindableFromContext` is optional but should be required in the future. diff --git a/tests/collection.js b/tests/collection.js index 5da9902..a3f215c 100644 --- a/tests/collection.js +++ b/tests/collection.js @@ -273,11 +273,11 @@ describe( 'Collection', () => { expectToThrowCKEditorError( () => { collection.add( item2, -1 ); - } ).to.throw( /^collection-add-item-invalid-index/ ); + }, /^collection-add-item-invalid-index/ ); expectToThrowCKEditorError( () => { collection.add( item2, 2 ); - } ).to.throw( /^collection-add-item-invalid-index/ ); + }, /^collection-add-item-invalid-index/ ); collection.add( item2, 1 ); collection.add( item3, 0 ); diff --git a/tests/focustracker.js b/tests/focustracker.js index 8a10fdc..fecc056 100644 --- a/tests/focustracker.js +++ b/tests/focustracker.js @@ -6,7 +6,6 @@ /* global document, Event */ import FocusTracker from '../src/focustracker'; -import CKEditorError from '../src/ckeditorerror'; import global from '../src/dom/global'; import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; import { expectToThrowCKEditorError } from './_utils/utils'; @@ -69,7 +68,7 @@ describe( 'FocusTracker', () => { expectToThrowCKEditorError( () => { focusTracker.add( containerFirstInput ); - }, /focusTracker-add-element-already-exist/, null ); + }, /focusTracker-add-element-already-exist/, focusTracker ); } ); describe( 'single element', () => { From 91a6000c8e9e410977a18db5e607e298aa3d7edc Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Mon, 1 Jul 2019 11:05:03 +0200 Subject: [PATCH 10/16] Made `assertCKEditorError` accepting strings and regexps as messages. --- tests/_utils/utils.js | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/tests/_utils/utils.js b/tests/_utils/utils.js index e98d85a..74bfec5 100644 --- a/tests/_utils/utils.js +++ b/tests/_utils/utils.js @@ -92,7 +92,7 @@ export function assertBinding( observable, stateBefore, data, stateAfter ) { * have common props (So the watchdog will be able to find the correct editor instance and restart it). * * @param {Function} fn Tested function that should throw a `CKEditorError`. - * @param {RegExp} message Expected message of the error. + * @param {RegExp|String} message Expected message of the error. * @param {*} editorThatShouldBeFindableFromContext An editor instance that should be findable from the error context. * @param {Object} [data] Error data. */ @@ -116,14 +116,17 @@ export function expectToThrowCKEditorError( fn, message, editorThatShouldBeFinda * find the correct editor instance and restart it). * * @param {module:utils/ckeditorerror~CKEditorError} err The tested error. - * @param {RegExp} message Expected message of the error. + * @param {RegExp|String} message Expected message of the error. * @param {*} [editorThatShouldBeFindableFromContext] An editor instance that should be findable from the error context. * @param {Object} [data] Error data. */ export function assertCKEditorError( err, message, editorThatShouldBeFindableFromContext, data ) { - expect( err ).to.be.instanceOf( CKEditorError ); - expect( message ).to.be.a( 'regexp', `Error message should be a string. Got ${ typeof message }.` ); + if ( typeof message === 'string' ) { + message = new RegExp( message ); + } + expect( message ).to.be.a( 'regexp', 'Error message should be a string or a regexp.' ); + expect( err ).to.be.instanceOf( CKEditorError ); expect( err.message ).to.match( message, 'Error message does not match the provided one.' ); // TODO: The `editorThatShouldBeFindableFromContext` is optional but should be required in the future. From f51c7c21ddeb166c285d7c91d753634edb026d9c Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Mon, 1 Jul 2019 11:35:59 +0200 Subject: [PATCH 11/16] Removed unused import. --- tests/keyboard.js | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/keyboard.js b/tests/keyboard.js index af042ce..6ff3c43 100644 --- a/tests/keyboard.js +++ b/tests/keyboard.js @@ -5,7 +5,6 @@ import env from '../src/env'; import { keyCodes, getCode, parseKeystroke, getEnvKeystrokeText } from '../src/keyboard'; -import CKEditorError from '../src/ckeditorerror'; import { expectToThrowCKEditorError } from './_utils/utils'; describe( 'Keyboard', () => { From fd783bf9c9d03b321cf0356ba78cd78f2cf6fdbe Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Mon, 1 Jul 2019 14:59:37 +0200 Subject: [PATCH 12/16] Added API comment. --- src/ckeditorerror.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/ckeditorerror.js b/src/ckeditorerror.js index 2b2382d..637644a 100644 --- a/src/ckeditorerror.js +++ b/src/ckeditorerror.js @@ -35,6 +35,8 @@ export default class CKEditorError extends Error { * @param {Object|null} context A context of the error by which the {@link module:watchdog/watchdog~Watchdog watchdog} * is able to determine which editor crashed. It should be an editor instance or a property connected to it. It can be also * a `null` value if the editor should not be restarted in case of the error (e.g. during the editor initialization). + * The error context should be checked using the `areConnectedThroughProperties( editor, context )` utility + * to check if the object works as the context. * @param {Object} [data] Additional data describing the error. A stringified version of this object * will be appended to the error message, so the data are quickly visible in the console. The original * data object will also be later available under the {@link #data} property. From 6a5d82b856aaa6318a20fd4dfc9b05492f2fabc3 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Mon, 1 Jul 2019 15:35:15 +0200 Subject: [PATCH 13/16] Fixed bug in the assertCKEditorError fn. --- tests/_utils/utils.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/_utils/utils.js b/tests/_utils/utils.js index 74bfec5..236cfe6 100644 --- a/tests/_utils/utils.js +++ b/tests/_utils/utils.js @@ -133,10 +133,8 @@ export function assertCKEditorError( err, message, editorThatShouldBeFindableFro if ( editorThatShouldBeFindableFromContext === null ) { expect( err.context ).to.equal( null, 'Error context was expected to be `null`' ); } else if ( editorThatShouldBeFindableFromContext !== undefined ) { - expect( - areConnectedThroughProperties( editorThatShouldBeFindableFromContext, err.context ), - 'Editor cannot be find from the error context' - ); + expect( areConnectedThroughProperties( editorThatShouldBeFindableFromContext, err.context ) ) + .to.equal( true, 'Editor cannot be find from the error context' ); } if ( data ) { From 657795ab14ede0f2fdaa564d8c126c0c6d2c2962 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Mon, 1 Jul 2019 15:36:50 +0200 Subject: [PATCH 14/16] Added missing error contexts. --- src/collection.js | 12 ++++++------ src/keyboard.js | 2 +- src/observablemixin.js | 26 +++++++++++++++----------- 3 files changed, 22 insertions(+), 18 deletions(-) diff --git a/src/collection.js b/src/collection.js index e0e3f78..bc22a90 100644 --- a/src/collection.js +++ b/src/collection.js @@ -148,7 +148,7 @@ export default class Collection { * * @error collection-add-invalid-id */ - throw new CKEditorError( 'collection-add-invalid-id' ); + throw new CKEditorError( 'collection-add-invalid-id', this ); } if ( this.get( itemId ) ) { @@ -157,7 +157,7 @@ export default class Collection { * * @error collection-add-item-already-exists */ - throw new CKEditorError( 'collection-add-item-already-exists' ); + throw new CKEditorError( 'collection-add-item-already-exists', this ); } } else { item[ idProperty ] = itemId = uid(); @@ -172,7 +172,7 @@ export default class Collection { * * @error collection-add-item-bad-index */ - throw new CKEditorError( 'collection-add-item-invalid-index' ); + throw new CKEditorError( 'collection-add-item-invalid-index', this ); } this._items.splice( index, 0, item ); @@ -203,7 +203,7 @@ export default class Collection { * * @error collection-get-invalid-arg */ - throw new CKEditorError( 'collection-get-invalid-arg: Index or id must be given.' ); + throw new CKEditorError( 'collection-get-invalid-arg: Index or id must be given.', this ); } return item || null; @@ -286,7 +286,7 @@ export default class Collection { * * @error collection-remove-404 */ - throw new CKEditorError( 'collection-remove-404: Item not found.' ); + throw new CKEditorError( 'collection-remove-404: Item not found.', this ); } this._items.splice( index, 1 ); @@ -459,7 +459,7 @@ export default class Collection { * * @error collection-bind-to-rebind */ - throw new CKEditorError( 'collection-bind-to-rebind: The collection cannot be bound more than once.' ); + throw new CKEditorError( 'collection-bind-to-rebind: The collection cannot be bound more than once.', this ); } this._bindToCollection = externalCollection; diff --git a/src/keyboard.js b/src/keyboard.js index 28c7d25..652a7f6 100644 --- a/src/keyboard.js +++ b/src/keyboard.js @@ -60,7 +60,7 @@ export function getCode( key ) { * @errror keyboard-unknown-key * @param {String} key */ - throw new CKEditorError( 'keyboard-unknown-key: Unknown key name.', { key } ); + throw new CKEditorError( 'keyboard-unknown-key: Unknown key name.', this, { key } ); } } else { keyCode = key.keyCode + diff --git a/src/observablemixin.js b/src/observablemixin.js index f43b47c..b474b9c 100644 --- a/src/observablemixin.js +++ b/src/observablemixin.js @@ -62,7 +62,7 @@ const ObservableMixin = { * * @error observable-set-cannot-override */ - throw new CKEditorError( 'observable-set-cannot-override: Cannot override an existing property.' ); + throw new CKEditorError( 'observable-set-cannot-override: Cannot override an existing property.', this ); } Object.defineProperty( this, name, { @@ -107,7 +107,7 @@ const ObservableMixin = { * * @error observable-bind-wrong-properties */ - throw new CKEditorError( 'observable-bind-wrong-properties: All properties must be strings.' ); + throw new CKEditorError( 'observable-bind-wrong-properties: All properties must be strings.', this ); } if ( ( new Set( bindProperties ) ).size !== bindProperties.length ) { @@ -116,7 +116,7 @@ const ObservableMixin = { * * @error observable-bind-duplicate-properties */ - throw new CKEditorError( 'observable-bind-duplicate-properties: Properties must be unique.' ); + throw new CKEditorError( 'observable-bind-duplicate-properties: Properties must be unique.', this ); } initObservable( this ); @@ -130,7 +130,7 @@ const ObservableMixin = { * * @error observable-bind-rebind */ - throw new CKEditorError( 'observable-bind-rebind: Cannot bind the same property more that once.' ); + throw new CKEditorError( 'observable-bind-rebind: Cannot bind the same property more that once.', this ); } } ); @@ -186,7 +186,7 @@ const ObservableMixin = { * * @error observable-unbind-wrong-properties */ - throw new CKEditorError( 'observable-unbind-wrong-properties: Properties must be strings.' ); + throw new CKEditorError( 'observable-unbind-wrong-properties: Properties must be strings.', this ); } unbindProperties.forEach( propertyName => { @@ -246,6 +246,7 @@ const ObservableMixin = { */ throw new CKEditorError( 'observablemixin-cannot-decorate-undefined: Cannot decorate an undefined method.', + this, { object: this, methodName } ); } @@ -380,7 +381,7 @@ function bindTo( ...args ) { * * @error observable-bind-no-callback */ - throw new CKEditorError( 'observable-bind-to-no-callback: Binding multiple observables only possible with callback.' ); + throw new CKEditorError( 'observable-bind-to-no-callback: Binding multiple observables only possible with callback.', this ); } // Eliminate A.bind( 'x', 'y' ).to( B, callback ) @@ -390,7 +391,10 @@ function bindTo( ...args ) { * * @error observable-bind-to-extra-callback */ - throw new CKEditorError( 'observable-bind-to-extra-callback: Cannot bind multiple properties and use a callback in one binding.' ); + throw new CKEditorError( + 'observable-bind-to-extra-callback: Cannot bind multiple properties and use a callback in one binding.', + this + ); } parsedArgs.to.forEach( to => { @@ -401,7 +405,7 @@ function bindTo( ...args ) { * * @error observable-bind-to-properties-length */ - throw new CKEditorError( 'observable-bind-to-properties-length: The number of properties must match.' ); + throw new CKEditorError( 'observable-bind-to-properties-length: The number of properties must match.', this ); } // When no to.properties specified, observing source properties instead i.e. @@ -442,7 +446,7 @@ function bindToMany( observables, attribute, callback ) { * * @error observable-bind-to-many-not-one-binding */ - throw new CKEditorError( 'observable-bind-to-many-not-one-binding: Cannot bind multiple properties with toMany().' ); + throw new CKEditorError( 'observable-bind-to-many-not-one-binding: Cannot bind multiple properties with toMany().', this ); } this.to( @@ -501,7 +505,7 @@ function parseBindToArgs( ...args ) { * * @error observable-bind-to-parse-error */ - throw new CKEditorError( 'observable-bind-to-parse-error: Invalid argument syntax in `to()`.' ); + throw new CKEditorError( 'observable-bind-to-parse-error: Invalid argument syntax in `to()`.', this ); } const parsed = { to: [] }; @@ -518,7 +522,7 @@ function parseBindToArgs( ...args ) { lastObservable = { observable: a, properties: [] }; parsed.to.push( lastObservable ); } else { - throw new CKEditorError( 'observable-bind-to-parse-error: Invalid argument syntax in `to()`.' ); + throw new CKEditorError( 'observable-bind-to-parse-error: Invalid argument syntax in `to()`.', this ); } } ); From 28a2d4e9f11f9aebe27eeea20ce4b121ffe23b74 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Tue, 2 Jul 2019 01:41:54 +0200 Subject: [PATCH 15/16] Fixed error contexts. --- src/keyboard.js | 5 ++++- src/observablemixin.js | 9 ++++++--- tests/keyboard.js | 4 ++-- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/keyboard.js b/src/keyboard.js index 652a7f6..5018582 100644 --- a/src/keyboard.js +++ b/src/keyboard.js @@ -60,7 +60,10 @@ export function getCode( key ) { * @errror keyboard-unknown-key * @param {String} key */ - throw new CKEditorError( 'keyboard-unknown-key: Unknown key name.', this, { key } ); + throw new CKEditorError( + 'keyboard-unknown-key: Unknown key name.', + null, { key } + ); } } else { keyCode = key.keyCode + diff --git a/src/observablemixin.js b/src/observablemixin.js index b474b9c..4a45619 100644 --- a/src/observablemixin.js +++ b/src/observablemixin.js @@ -381,7 +381,10 @@ function bindTo( ...args ) { * * @error observable-bind-no-callback */ - throw new CKEditorError( 'observable-bind-to-no-callback: Binding multiple observables only possible with callback.', this ); + throw new CKEditorError( + 'observable-bind-to-no-callback: Binding multiple observables only possible with callback.', + this + ); } // Eliminate A.bind( 'x', 'y' ).to( B, callback ) @@ -505,7 +508,7 @@ function parseBindToArgs( ...args ) { * * @error observable-bind-to-parse-error */ - throw new CKEditorError( 'observable-bind-to-parse-error: Invalid argument syntax in `to()`.', this ); + throw new CKEditorError( 'observable-bind-to-parse-error: Invalid argument syntax in `to()`.', null ); } const parsed = { to: [] }; @@ -522,7 +525,7 @@ function parseBindToArgs( ...args ) { lastObservable = { observable: a, properties: [] }; parsed.to.push( lastObservable ); } else { - throw new CKEditorError( 'observable-bind-to-parse-error: Invalid argument syntax in `to()`.', this ); + throw new CKEditorError( 'observable-bind-to-parse-error: Invalid argument syntax in `to()`.', null ); } } ); diff --git a/tests/keyboard.js b/tests/keyboard.js index 6ff3c43..3636b72 100644 --- a/tests/keyboard.js +++ b/tests/keyboard.js @@ -57,7 +57,7 @@ describe( 'Keyboard', () => { it( 'throws when passed unknown key name', () => { expectToThrowCKEditorError( () => { getCode( 'foo' ); - }, /^keyboard-unknown-key:/ ); + }, /^keyboard-unknown-key:/, null ); } ); it( 'gets code of a keystroke info', () => { @@ -98,7 +98,7 @@ describe( 'Keyboard', () => { it( 'throws on unknown name', () => { expectToThrowCKEditorError( () => { parseKeystroke( 'foo' ); - }, /^keyboard-unknown-key:/ ); + }, /^keyboard-unknown-key:/, null ); } ); } ); From d182bdbeb29c02fe2fdb80788a900caf9d0272e5 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Tue, 2 Jul 2019 02:14:11 +0200 Subject: [PATCH 16/16] Fixed param names. --- src/areconnectedthroughproperties.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/areconnectedthroughproperties.js b/src/areconnectedthroughproperties.js index ffadda2..0797402 100644 --- a/src/areconnectedthroughproperties.js +++ b/src/areconnectedthroughproperties.js @@ -10,18 +10,18 @@ /* globals EventTarget, Event */ /** - * Traverses both structures to find out whether there's some object that is shared between both structures. + * Traverses both structures to find out whether there is a reference that is shared between both structures. * - * @param {*} str1 - * @param {*} str2 + * @param {Object|Array} obj1 + * @param {Object|Array} obj2 */ -export default function areConnectedThroughProperties( str1, str2 ) { - if ( str1 === str2 && isObject( str1 ) ) { +export default function areConnectedThroughProperties( obj1, obj2 ) { + if ( obj1 === obj2 && isObject( obj1 ) ) { return true; } - const subNodes1 = getSubNodes( str1 ); - const subNodes2 = getSubNodes( str2 ); + const subNodes1 = getSubNodes( obj1 ); + const subNodes2 = getSubNodes( obj2 ); for ( const node of subNodes1 ) { if ( subNodes2.has( node ) ) {