Thiemo Mättig (WMDE) has uploaded a new change for review. ( https://gerrit.wikimedia.org/r/383383 )
Change subject: Remove QUnit.Parameterize dependency and make tests pass with QUnit 2 ...................................................................... Remove QUnit.Parameterize dependency and make tests pass with QUnit 2 This also fixes a series of related issues, like destroy() not properly calling destroy() on all child elements. This made tests fail on my local machine. I might split this patch if requested. Please tell me if I should do so. Bug: T177764 Change-Id: I44fc7d720ae3a2134d60702807915dccf0679812 --- M RELEASE-NOTES.md M ValueView.php M src/ExpertExtender/ExpertExtender.Preview.js M src/experts/GlobeCoordinateInput.js M src/experts/TimeInput.js M tests/lib/jquery/jquery.AnimationEvent.tests.js M tests/lib/jquery/jquery.focusAt.tests.js M tests/lib/resources.php M tests/src/experts/StringValue.tests.js M tests/src/jquery.valueview.ExpertStore.tests.js M tests/src/jquery.valueview.tests.MockViewState.tests.js M tests/src/jquery.valueview.tests.testExpert.js M tests/src/resources.php 13 files changed, 168 insertions(+), 162 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/data-values/value-view refs/changes/83/383383/1 diff --git a/RELEASE-NOTES.md b/RELEASE-NOTES.md index b160f6f..a08d3b2 100644 --- a/RELEASE-NOTES.md +++ b/RELEASE-NOTES.md @@ -1,5 +1,14 @@ # ValueView release notes +## 0.21.0 (2017-10-10) +* Removed `jQuery.valueview.ExpertExtender.CalendarHint`. +* Removed dependency on `globeCoordinate.Formatter`. +* Removed dependency on the [Parameterize](https://github.com/AStepaniuk/qunit-parameterize) QUnit plugin. +* Fixed `jQuery.valueview.ExpertExtender.Preview.destroy` failing when called twice. +* Fixed incomplete `jQuery.valueview.experts.GlobeCoordinateInput.destroy`. +* Fixed incomplete `jQuery.valueview.experts.TimeInput.destroy`. +* Made all tests compatible with QUnit 2. + ## 0.20.1 (2017-08-03) * Fixed `jQuery.ui.suggester` and `jQuery.animateWithEvent` tests. diff --git a/ValueView.php b/ValueView.php index 560354a..39d77a5 100644 --- a/ValueView.php +++ b/ValueView.php @@ -5,7 +5,7 @@ return 1; } -define( 'VALUEVIEW_VERSION', '0.20.1' ); +define( 'VALUEVIEW_VERSION', '0.21.0' ); // Include the composer autoloader if it is present. if ( is_readable( __DIR__ . '/vendor/autoload.php' ) ) { diff --git a/src/ExpertExtender/ExpertExtender.Preview.js b/src/ExpertExtender/ExpertExtender.Preview.js index cee1e49..d94a545 100644 --- a/src/ExpertExtender/ExpertExtender.Preview.js +++ b/src/ExpertExtender/ExpertExtender.Preview.js @@ -60,10 +60,12 @@ * Callback for the `destroy` `ExpertExtender` event. */ destroy: function() { - this._preview.destroy(); - this._preview.element.remove(); + if ( this._preview ) { + this._preview.destroy(); + this._preview.element.remove(); + this._preview = null; + } - this._preview = null; this._getUpstreamValue = null; }, diff --git a/src/experts/GlobeCoordinateInput.js b/src/experts/GlobeCoordinateInput.js index df61cf8..f091206 100644 --- a/src/experts/GlobeCoordinateInput.js +++ b/src/experts/GlobeCoordinateInput.js @@ -204,8 +204,15 @@ * @inheritdoc */ destroy: function() { - this.precisionRotator = null; - this.preview = null; + if ( this.precisionRotator ) { + this.precisionRotator.destroy(); + this.precisionRotator = null; + } + if ( this.preview ) { + this.preview.destroy(); + this.preview = null; + } + PARENT.prototype.destroy.call( this ); } } ); diff --git a/src/experts/TimeInput.js b/src/experts/TimeInput.js index aa80b33..8dbb99c 100644 --- a/src/experts/TimeInput.js +++ b/src/experts/TimeInput.js @@ -148,9 +148,18 @@ * @inheritdoc */ destroy: function() { - this.preview = null; - this.precisionRotator = null; - this.calendarRotator = null; + if ( this.calendarRotator ) { + this.calendarRotator.destroy(); + this.calendarRotator = null; + } + if ( this.precisionRotator ) { + this.precisionRotator.destroy(); + this.precisionRotator = null; + } + if ( this.preview ) { + this.preview.destroy(); + this.preview = null; + } PARENT.prototype.destroy.call( this ); // empties viewport }, diff --git a/tests/lib/jquery/jquery.AnimationEvent.tests.js b/tests/lib/jquery/jquery.AnimationEvent.tests.js index 8eaa60e..ae8daca 100644 --- a/tests/lib/jquery/jquery.AnimationEvent.tests.js +++ b/tests/lib/jquery/jquery.AnimationEvent.tests.js @@ -64,7 +64,7 @@ } ); QUnit.test( 'animationOptions()', function( assert ) { - assert.expect( 9 ); + assert.expect( AnimationEvent.ANIMATION_STEPS.length + 2 ); var event = AnimationEvent( 'animationpurpose' ); var predefined = { easing: 'swing', diff --git a/tests/lib/jquery/jquery.focusAt.tests.js b/tests/lib/jquery/jquery.focusAt.tests.js index f159e69..683de22 100644 --- a/tests/lib/jquery/jquery.focusAt.tests.js +++ b/tests/lib/jquery/jquery.focusAt.tests.js @@ -54,112 +54,118 @@ } ]; - var elemsCases = QUnit.cases( elemsCasesData ); + elemsCasesData.forEach( function ( params ) { + QUnit.test( 'Focusing with valid parameter', function( assert ) { + assert.expect( 10 ); + var $dom = getDomInsertionTestViewport(), + positions = [ 0, 1, 4, 9, 9999, 'start', 'end', -1, -3, -9999 ]; - elemsCases.test( 'Focusing with valid parameter', function( params, assert ) { - assert.expect( 10 ); - var $dom = getDomInsertionTestViewport(), - positions = [ 0, 1, 4, 9, 9999, 'start', 'end', -1, -3, -9999 ]; - - $.each( positions, function( i, pos ) { - // Put element in DOM, since Firefox expects this - $dom.append( params.elem ); - assert.ok( - params.elem.focusAt( pos ), - 'focusAt takes "' + pos + '" as a valid position for the element' - ); - params.elem.remove(); + $.each( positions, function( i, pos ) { + // Put element in DOM, since Firefox expects this + $dom.append( params.elem ); + assert.ok( + params.elem.focusAt( pos ), + 'focusAt takes "' + pos + '" as a valid position for the element' + ); + params.elem.remove(); + } ); } ); } ); - elemsCases.test( 'Focusing with invalid parameter', function( params, assert ) { - assert.expect( 5 ); - var positions = [ null, undefined, 'foo', [], {} ]; + elemsCasesData.forEach( function ( params ) { + QUnit.test( 'Focusing with invalid parameter', function( assert ) { + assert.expect( 5 ); + var positions = [ null, undefined, 'foo', [], {} ]; - $.each( positions, function( i, pos ) { - assert.throws( - function() { - params.elem.focusAt( pos ); - }, - 'focusAt does not take "' + pos + '" as a valid position for the element' - ); + $.each( positions, function( i, pos ) { + assert.throws( + function() { + params.elem.focusAt( pos ); + }, + 'focusAt does not take "' + pos + '" as a valid position for the element' + ); + } ); } ); } ); - elemsCases.test( 'Focusing element, not in DOM yet', function( params, assert ) { - assert.expect( 2 ); - var $dom = getDomInsertionTestViewport(), - elem = params.elem; + elemsCasesData.forEach( function ( params ) { + QUnit.test( 'Focusing element, not in DOM yet', function( assert ) { + assert.expect( 2 ); + var $dom = getDomInsertionTestViewport(), + elem = params.elem; - if ( !$dom.length ) { - throw new Error( 'Can only run this test on a HTML page with "body" tag in the browser.' ); - } + if ( !$dom.length ) { + throw new Error( 'Can only run this test on a HTML page with "body" tag in the browser.' ); + } - try { - assert.ok( - elem.focusAt( 0 ), - 'Can call focusAt on element not in DOM yet.' - ); - } catch ( e ) { - assert.expect( 1 ); - assert.ok( - e.name === 'NS_ERROR_FAILURE' && e.result === 0x80004005, - 'Unable to focus since browser requires element to be in the DOM.' - ); - return; - } + try { + assert.ok( + elem.focusAt( 0 ), + 'Can call focusAt on element not in DOM yet.' + ); + } catch ( e ) { + assert.expect( 1 ); + assert.ok( + e.name === 'NS_ERROR_FAILURE' && e.result === 0x80004005, + 'Unable to focus since browser requires element to be in the DOM.' + ); + return; + } - $( ':focus' ).blur(); - elem.appendTo( $dom ); + $( ':focus' ).blur(); + elem.appendTo( $dom ); - assert.equal( - $( ':focus' ).length, - 0, - 'After inserting focused element into DOM, the element is not focused since there is' + - 'no state tracking focus for those elements not in the DOM.' - ); - elem.remove(); - } ); - - elemsCases.test( 'Focusing element in DOM', function( params, assert ) { - var $dom = getDomInsertionTestViewport(), - elem = params.elem, - isOk; - - if ( !$dom.length ) { - throw new Error( 'Can only run this test on a HTML page with "body" tag in the browser.' ); - } - - $( ':focus' ).blur(); - elem.appendTo( $dom ); - - // Check if focussing actually works - elem.focus(); - if ( !elem.is( ':focus' ) ) { - assert.expect( 1 ); - assert.ok( 'Could not test because focussing does not work.' ); - return; - } - assert.expect( 3 ); - elem.blur(); - assert.ok( !elem.is( ':focus' ) ); - - assert.ok( - elem.focusAt( 0 ), - 'Can call focusAt on element in DOM' - ); - - if ( !params.focusable ) { assert.equal( $( ':focus' ).length, 0, - 'Element is a non-focusable element and no focus is active' + 'After inserting focused element into DOM, the element is not focused since there is' + + 'no state tracking focus for those elements not in the DOM.' ); - } else { - isOk = $( ':focus' ).filter( elem ).length; - assert.ok( isOk, 'Focused element has focus set.' ); - } - elem.remove(); + elem.remove(); + } ); + } ); + + elemsCasesData.forEach( function ( params ) { + QUnit.test( 'Focusing element in DOM', function( assert ) { + var $dom = getDomInsertionTestViewport(), + elem = params.elem, + isOk; + + if ( !$dom.length ) { + throw new Error( 'Can only run this test on a HTML page with "body" tag in the browser.' ); + } + + $( ':focus' ).blur(); + elem.appendTo( $dom ); + + // Check if focussing actually works + elem.focus(); + if ( !elem.is( ':focus' ) ) { + assert.expect( 1 ); + assert.ok( 'Could not test because focussing does not work.' ); + return; + } + assert.expect( 3 ); + elem.blur(); + assert.ok( !elem.is( ':focus' ) ); + + assert.ok( + elem.focusAt( 0 ), + 'Can call focusAt on element in DOM' + ); + + if ( !params.focusable ) { + assert.equal( + $( ':focus' ).length, + 0, + 'Element is a non-focusable element and no focus is active' + ); + } else { + isOk = $( ':focus' ).filter( elem ).length; + assert.ok( isOk, 'Focused element has focus set.' ); + } + elem.remove(); + } ); } ); }( jQuery, QUnit ) ); diff --git a/tests/lib/resources.php b/tests/lib/resources.php index 4646f58..62cec38 100644 --- a/tests/lib/resources.php +++ b/tests/lib/resources.php @@ -51,7 +51,6 @@ ), 'dependencies' => array( 'jquery.focusAt', - 'qunit.parameterize', ), ), diff --git a/tests/src/experts/StringValue.tests.js b/tests/src/experts/StringValue.tests.js index 2a6c8d7..3406888 100644 --- a/tests/src/experts/StringValue.tests.js +++ b/tests/src/experts/StringValue.tests.js @@ -2,7 +2,7 @@ * @licence GNU GPL v2+ * @author Daniel Werner < daniel.wer...@wikimedia.de > */ - ( function( QUnit, valueview ) { +( function( QUnit, valueview ) { 'use strict'; var testExpert = valueview.tests.testExpert; diff --git a/tests/src/jquery.valueview.ExpertStore.tests.js b/tests/src/jquery.valueview.ExpertStore.tests.js index 9440796..df9ecb7 100644 --- a/tests/src/jquery.valueview.ExpertStore.tests.js +++ b/tests/src/jquery.valueview.ExpertStore.tests.js @@ -267,13 +267,13 @@ } ); } - QUnit - .cases( expertStoreRegistrationTestCases ) - .test( + expertStoreRegistrationTestCases.forEach( function ( params ) { + QUnit.test( 'registerDataTypeExpert()/registerDataValueExpert() & getExpert()', - function( params, assert ) { + function( assert ) { expertStoreRegistrationTest( assert, params.register, params.expect ); } ); + } ); }( jQuery, dataValues, QUnit ) ); diff --git a/tests/src/jquery.valueview.tests.MockViewState.tests.js b/tests/src/jquery.valueview.tests.MockViewState.tests.js index 5ee2f48..e5b1e32 100644 --- a/tests/src/jquery.valueview.tests.MockViewState.tests.js +++ b/tests/src/jquery.valueview.tests.MockViewState.tests.js @@ -14,11 +14,12 @@ /** * Helper which returns a test function for a member of MockViewState. * + * @param {Object} params * @param {string} memberName * @return {Function} */ - function buildMemberTestFn( memberName ) { - return function( params, assert ) { + function buildMemberTestFn( params, memberName ) { + return function( assert ) { assert.expect( 1 ); var viewState = new MockViewState( params.constructorArg ); @@ -30,8 +31,7 @@ }; } - QUnit - .cases( [ + var testCases = [ { title: 'without constructor argument', constructorArg: undefined, @@ -79,8 +79,10 @@ optionFoo: true, optionBar: undefined } - ] ) - .test( 'constructor', function( params, assert ) { + ]; + + testCases.forEach( function ( params ) { + QUnit.test( 'constructor', function( assert ) { assert.expect( 2 ); var viewState = new MockViewState( params.constructorArg ); @@ -93,11 +95,15 @@ viewState instanceof ViewState, 'Constructed MockViewState is instanceof ViewState' ); - } ) - .test( 'isInEditMode', buildMemberTestFn( 'isInEditMode' ) ) - .test( 'isDisabled', buildMemberTestFn( 'isDisabled' ) ) - .test( 'value', buildMemberTestFn( 'value' ) ) - .test( 'option', function( params, assert ) { + } ); + + QUnit.test( 'isInEditMode', buildMemberTestFn( params, 'isInEditMode' ) ); + + QUnit.test( 'isDisabled', buildMemberTestFn( params, 'isDisabled' ) ); + + QUnit.test( 'value', buildMemberTestFn( params, 'value' ) ); + + QUnit.test( 'option', function( assert ) { assert.expect( 2 ); var viewState = new MockViewState( params.constructorArg ); @@ -112,8 +118,8 @@ params.optionBar, 'Option "bar" holds injected value' ); - } ); + } ); QUnit.test( 'Changing state after construction', function( assert ) { assert.expect( 2 ); diff --git a/tests/src/jquery.valueview.tests.testExpert.js b/tests/src/jquery.valueview.tests.testExpert.js index 19802f2..9ca3700 100644 --- a/tests/src/jquery.valueview.tests.testExpert.js +++ b/tests/src/jquery.valueview.tests.testExpert.js @@ -2,8 +2,8 @@ * @licence GNU GPL v2+ * @author Daniel Werner < daniel.wer...@wikimedia.de > */ - jQuery.valueview.tests = jQuery.valueview.tests || {}; - jQuery.valueview.tests.testExpert = ( function( $, QUnit, valueview, Notifier ) { +jQuery.valueview.tests = jQuery.valueview.tests || {}; +jQuery.valueview.tests.testExpert = ( function( $, QUnit, valueview, Notifier ) { 'use strict'; @@ -21,7 +21,6 @@ var Expert = testDefinition.expertConstructor; - // Used as source for expertProviders. function createExpertDefinitions() { return [ { @@ -48,48 +47,20 @@ ]; } - /** - * Returns an array of Functions. Each function returns an object with the following fields - * when executed: - * - expert: A valueview Expert instance. - * - constructorArgs: Arguments used to construct the expert given in the "expert" field. - * - * Each function has a "title" field which describes the expert instance mentioned above. - * - * @return {Function[]} - */ - function createExpertsProvider() { - return $.map( createExpertDefinitions(), function( definition ) { - // Provide a setup function for test case parameter creation instead of creating a case - // definition object directly. If that would be done later, the expert would already - // be created and, in some cases, create conflicts with other tests since some experts - // immediately instantiate certain widgets (e.g. inputextender). - var caseSetup = function() { + // We always have to destroy experts so all widgets used by them get destroyed as well in case + // they add something to the body. + function expertCasesTestAndCleanup( description, testFn ) { + createExpertDefinitions().forEach( function ( definition ) { + QUnit.test( description + ' (' + definition.title + ')', function( assert ) { var $viewPort = definition.args[0], viewState = definition.args[1], notifier = definition.args[2]; - var expert = new Expert( $viewPort, viewState, notifier, { messages: {} } ); - expert.init(); - - return { - expert: expert, - constructorArgs: definition.args - }; - }; - caseSetup.title = definition.title; - return caseSetup; - } ); - } - - var expertCases = QUnit.cases( createExpertsProvider ); - - // We always have to destroy experts so all widgets used by them get destroyed as well in case - // they add something to the body. - function expertCasesTestAndCleanup( description, testFn ) { - expertCases.test( description, function( args, assert ) { - testFn( args, assert ); - args.expert.destroy(); + definition.expert = new Expert( $viewPort, viewState, notifier, { messages: {} } ); + definition.expert.init(); + testFn( definition, assert ); + definition.expert.destroy(); + } ); } ); } diff --git a/tests/src/resources.php b/tests/src/resources.php index 2a3d3a2..b681d0a 100644 --- a/tests/src/resources.php +++ b/tests/src/resources.php @@ -26,7 +26,6 @@ 'dataValues.values', 'jquery.valueview.ExpertStore', 'jquery.valueview.tests.MockExpert', - 'qunit.parameterize', ), ), @@ -69,7 +68,6 @@ ), 'dependencies' => array( 'jquery.valueview.tests.MockViewState', - 'qunit.parameterize', ), ), @@ -80,7 +78,6 @@ 'dependencies' => array( 'jquery.valueview.Expert', 'jquery.valueview.tests.MockViewState', - 'qunit.parameterize', 'util.Notifier', ), ), -- To view, visit https://gerrit.wikimedia.org/r/383383 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I44fc7d720ae3a2134d60702807915dccf0679812 Gerrit-PatchSet: 1 Gerrit-Project: data-values/value-view Gerrit-Branch: master Gerrit-Owner: Thiemo Mättig (WMDE) <thiemo.maet...@wikimedia.de> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
