Merge "Improve & fix action and integration tests"

src/actions.js

@@ -142,7 +142,7 @@ export function fetch( gateway, title, el, token ) {
 					token: token
 				} );
 			} )
-			.fail( function ( data, result ) {
+			.catch( function ( data, result ) {
 				// All failures, except those due to being offline or network error,
 				// should present "There was an issue displaying this preview".
 				// e.g.:
@@ -213,7 +213,7 @@ export function linkDwell( title, el, event, gateway, generateToken ) {
 				var previewState = getState().preview;
 
 				if ( previewState.enabled && isNewInteraction() ) {
-					dispatch( fetch( gateway, title, el, token ) );
+					return dispatch( fetch( gateway, title, el, token ) );
 				}
 			} );
 	};

tests/node-qunit/actions.test.js

@@ -67,19 +67,10 @@ QUnit.test( '#boot', function ( assert ) {
 	* @param {Object} module
 	*/
 function setupWait( module ) {
-	module.waitDeferreds = [];
+	module.waitPromise = $.Deferred().resolve().promise();
-	module.waitPromises = [];
+	module.wait = module.sandbox.stub( WaitModule, 'default' ).callsFake( function () {
-
+		return module.waitPromise;
-	module.wait = module.sandbox.spy( function () {
-		var deferred = $.Deferred(),
-			promise = deferred.promise();
-
-		module.waitDeferreds.push( deferred );
-		module.waitPromises.push( promise );
-		return promise;
 	} );
-
-	module.sandbox.stub( WaitModule, 'default' ).callsFake( module.wait );
 }
 
 /**
@@ -95,14 +86,10 @@ function setupEl( module ) {
 
 QUnit.module( 'ext.popups/actions#linkDwell @integration', {
 	beforeEach: function () {
-		var that = this;
-
 		this.state = {
 			preview: {}
 		};
-		this.getState = function () {
+		this.getState = () => this.state;
-			return that.state;
-		};
 
 		// The worst-case implementation of mw.now.
 		mw.now = function () { return Date.now(); };
@@ -112,10 +99,12 @@ QUnit.module( 'ext.popups/actions#linkDwell @integration', {
 } );
 
 QUnit.test( '#linkDwell', function ( assert ) {
-	var p,
+	var linkDwelled,
 		event = {},
 		dispatch = this.sandbox.spy();
 
+	assert.expect( 2 );
+
 	this.sandbox.stub( mw, 'now' ).returns( new Date() );
 	this.sandbox.stub( actions, 'fetch' );
 
@@ -124,7 +113,7 @@ QUnit.test( '#linkDwell', function ( assert ) {
 		activeToken: generateToken()
 	};
 
-	p = actions.linkDwell(
+	linkDwelled = actions.linkDwell(
 		this.title, this.el, event, /* gateway = */ null, generateToken
 	)(
 		dispatch,
@@ -150,21 +139,22 @@ QUnit.test( '#linkDwell', function ( assert ) {
 
 	// ---
 
-	p.then( function () {
+	return linkDwelled.then( function () {
 		assert.strictEqual(
 			dispatch.callCount,
 			2,
 			'The fetch action is dispatched after FETCH_COMPLETE milliseconds.'
 		);
 	} );
-	return p;
 } );
 
 QUnit.test( '#linkDwell doesn\'t continue when previews are disabled', function ( assert ) {
-	var p,
+	var linkDwelled,
 		event = {},
 		dispatch = this.sandbox.spy();
 
+	assert.expect( 2 );
+
 	// Stub the state tree being updated by the LINK_DWELL action.
 	this.state.preview = {
 		enabled: false,
@@ -172,7 +162,7 @@ QUnit.test( '#linkDwell doesn\'t continue when previews are disabled', function
 		activeToken: generateToken()
 	};
 
-	p = actions.linkDwell(
+	linkDwelled = actions.linkDwell(
 		this.title, this.el, event, /* gateway = */ null, generateToken
 	)(
 		dispatch,
@@ -181,18 +171,18 @@ QUnit.test( '#linkDwell doesn\'t continue when previews are disabled', function
 
 	assert.strictEqual( dispatch.callCount, 1 );
 
-	p.then( function () {
+	return linkDwelled.then( function () {
 		assert.strictEqual( dispatch.callCount, 1 );
 	} );
-
-	return p;
 } );
 
 QUnit.test( '#linkDwell doesn\'t continue if the token has changed', function ( assert ) {
-	var p,
+	var linkDwelled,
 		event = {},
 		dispatch = this.sandbox.spy();
 
+	assert.expect( 1 );
+
 	// Stub the state tree being updated by a LINK_DWELL action.
 	this.state.preview = {
 		enabled: true,
@@ -200,7 +190,7 @@ QUnit.test( '#linkDwell doesn\'t continue if the token has changed', function (
 		activeToken: generateToken()
 	};
 
-	p = actions.linkDwell(
+	linkDwelled = actions.linkDwell(
 		this.title, this.el, event, /* gateway = */ null, generateToken
 	)(
 		dispatch,
@@ -219,24 +209,23 @@ QUnit.test( '#linkDwell doesn\'t continue if the token has changed', function (
 		activeToken: 'banana'
 	};
 
-	p.then( function () {
+	return linkDwelled.then( function () {
 		assert.strictEqual( dispatch.callCount, 1 );
 	} );
-
-	return p;
 } );
 
 QUnit.test( '#linkDwell dispatches the fetch action', function ( assert ) {
-	var p,
+	var event = {},
-		event = {},
 		dispatch = this.sandbox.spy();
 
+	assert.expect( 1 );
+
 	this.state.preview = {
 		enabled: true,
 		activeToken: generateToken()
 	};
 
-	p = actions.linkDwell(
+	return actions.linkDwell(
 		this.title, this.el, event, /* gateway = */ null, generateToken
 	)(
 		dispatch,
@@ -244,28 +233,22 @@ QUnit.test( '#linkDwell dispatches the fetch action', function ( assert ) {
 	).then( function () {
 		assert.strictEqual( dispatch.callCount, 2 );
 	} );
-
-	return p;
 } );
 
 QUnit.module( 'ext.popups/actions#fetch', {
 	beforeEach: function () {
-		var that = this;
-
-		// Setup the mw.now stub before actions is re-required in setupWait
 		this.now = 0;
 
-		this.sandbox.stub( mw, 'now' ).callsFake( function () {
+		this.sandbox.stub( mw, 'now' ).callsFake( () => this.now );
-			return that.now;
-		} );
 
 		setupWait( this );
 		setupEl( this );
 
 		this.gatewayDeferred = $.Deferred();
-		this.gatewayPromise = this.gatewayDeferred.promise();
 		this.gateway = {
-			getPageSummary: this.sandbox.stub().returns( this.gatewayPromise )
+			getPageSummary: this.sandbox.stub().returns(
+				this.gatewayDeferred.promise()
+			)
 		};
 
 		this.dispatch = this.sandbox.spy();
@@ -273,15 +256,17 @@ QUnit.module( 'ext.popups/actions#fetch', {
 		this.token = '1234567890';
 
 		// Sugar.
-		this.fetch = function () {
+		this.fetch = () => {
 			return actions.fetch(
-				that.gateway, that.title, that.el, that.token
+				this.gateway, this.title, this.el, this.token
-			)( that.dispatch );
+			)( this.dispatch );
 		};
 	}
 } );
 
 QUnit.test( 'it should fetch data from the gateway immediately', function ( assert ) {
+	assert.expect( 3 );
+
 	this.fetch();
 
 	assert.ok( this.gateway.getPageSummary.calledWith( 'Foo' ) );
@@ -301,19 +286,21 @@ QUnit.test( 'it should fetch data from the gateway immediately', function ( asse
 } );
 
 QUnit.test( 'it should dispatch the FETCH_END action when the API request ends', function ( assert ) {
-	var that = this;
+	var fetched;
 
-	this.fetch();
+	assert.expect( 1 );
+
+	fetched = this.fetch();
 
 	this.now += 115;
 	this.gatewayDeferred.resolve( {} );
 
-	return this.gatewayPromise.then( function () {
+	return fetched.then( () => {
 		assert.deepEqual(
-			that.dispatch.getCall( 1 ).args[ 0 ],
+			this.dispatch.getCall( 1 ).args[ 0 ],
 			{
 				type: 'FETCH_END',
-				el: that.el,
+				el: this.el,
 				timestamp: 115
 			}
 		);
@@ -321,112 +308,82 @@ QUnit.test( 'it should dispatch the FETCH_END action when the API request ends',
 } );
 
 QUnit.test( 'it should delay dispatching the FETCH_COMPLETE action', function ( assert ) {
-	var whenDeferred = $.Deferred(),
+	var result = {},
-		whenSpy,
+		fetched = this.fetch();
-		args,
-		result = {},
-		that = this;
 
-	whenSpy = this.sandbox.stub( $, 'when' )
+	assert.expect( 2 );
-		.returns( whenDeferred.promise() );
-
-	this.fetch();
 
 	assert.strictEqual(
 		this.wait.getCall( 0 ).args[ 0 ],
 		350,
 		'It waits for FETCH_COMPLETE_TARGET_DELAY - FETCH_START_DELAY milliseconds.'
 	);
+	this.gatewayDeferred.resolve( result );
 
-	// ---
+	return fetched.then( () => {
-	args = whenSpy.getCall( 0 ).args;
-
-	// This assertion is disabled due to $.Promise#then and #fail returning a new
-	// instance of $.Promise.
-	// assert.strictEqual( args[ 0 ], this.gatewayPromise );
-
-	assert.strictEqual( args[ 1 ], this.waitPromises[ 0 ] );
-
-	// ---
-	whenDeferred.resolve( result );
-
-	return whenDeferred.then( function () {
-
-		// Ensure the following assertions are made after all callbacks have been
-		// executed. Use setTimeout( _, 0 ) since it's not critical that these
-		// assertions are run before I/O is processed, i.e. we don't require
-		// process.nextTick.
-		setTimeout( function () {
 		assert.deepEqual(
-				that.dispatch.getCall( 1 ).args[ 0 ],
+			this.dispatch.getCall( 2 ).args[ 0 ],
 			{
 				type: 'FETCH_COMPLETE',
-					el: that.el,
+				el: this.el,
 				result: result,
-					token: that.token
+				token: this.token
 			}
 		);
-		}, 0 );
 	} );
 } );
 
 QUnit.test( 'it should dispatch the FETCH_FAILED action when the request fails', function ( assert ) {
-	var that = this;
+	var fetched = this.fetch();
 
 	assert.expect( 2 );
 
 	this.gatewayDeferred.reject( new Error( 'API req failed' ) );
 
-	const fetch = this.fetch().catch( function () {
+	this.now += 115;
+
+	return fetched.then( () => {
 		assert.equal(
-			that.dispatch.callCount, 3,
+			this.dispatch.callCount, 3,
 			'dispatch called thrice, START, FAILED, and COMPLETE'
 		);
 		assert.deepEqual(
-			that.dispatch.getCall( 1 ).args[ 0 ],
+			this.dispatch.getCall( 1 ).args[ 0 ],
 			{
 				type: 'FETCH_FAILED',
-				el: that.el
+				el: this.el
 			}
 		);
 	} );
-
-	this.now += 115;
-
-	return fetch;
 } );
 
 QUnit.test( 'it should dispatch the FETCH_FAILED action when the request fails even after the wait timeout', function ( assert ) {
-	var that = this;
+	var fetched = this.fetch();
 
 	assert.expect( 2 );
 
-	const fetch = this.fetch().catch( function () {
+	// After the wait interval happens, resolve the gateway request
+	return this.waitPromise.then( () => {
+		this.gatewayDeferred.reject( new Error( 'API req failed' ) );
+		return fetched;
+	} ).then( () => {
 		assert.equal(
-			that.dispatch.callCount, 3,
+			this.dispatch.callCount, 3,
 			'dispatch called thrice, START, FAILED, and COMPLETE'
 		);
 		assert.deepEqual(
-			that.dispatch.getCall( 1 ).args[ 0 ],
+			this.dispatch.getCall( 1 ).args[ 0 ],
 			{
 				type: 'FETCH_FAILED',
-				el: that.el
+				el: this.el
 			}
 		);
 	} );
-
-	// After the wait interval happens, resolve the gateway request
-	this.waitPromises[ 0 ].then( function () {
-		that.gatewayDeferred.reject( new Error( 'API req failed' ) );
-	} );
-
-	this.waitDeferreds[ 0 ].resolve();
-
-	return fetch;
 } );
 
 QUnit.module( 'ext.popups/actions#abandon', {
 	beforeEach: function () {
+		setupWait( this );
 		setupEl( this );
 	}
 } );
@@ -440,13 +397,13 @@ QUnit.test( 'it should dispatch start and end actions', function ( assert ) {
 					activeToken: token
 				}
 			};
-		}, p;
+		}, abandoned;
+
+	assert.expect( 3 );
 
 	this.sandbox.stub( mw, 'now' ).returns( new Date() );
 
-	setupWait( this );
+	abandoned = actions.abandon( this.el )( dispatch, getState );
-
-	actions.abandon( this.el )( dispatch, getState );
 
 	assert.ok( dispatch.calledWith( {
 		type: 'ABANDON_START',
@@ -461,7 +418,7 @@ QUnit.test( 'it should dispatch start and end actions', function ( assert ) {
 		'Have you spoken with #Design about changing this value?'
 	);
 
-	p = this.waitPromises[ 0 ].then( function () {
+	return abandoned.then( function () {
 		assert.ok(
 			dispatch.calledWith( {
 				type: 'ABANDON_END',
@@ -470,10 +427,6 @@ QUnit.test( 'it should dispatch start and end actions', function ( assert ) {
 			'ABANDON_* share the same token.'
 		);
 	} );
-
-	// After ABANDON_END_DELAY milliseconds...
-	this.waitDeferreds[ 0 ].resolve();
-	return p;
 } );
 
 QUnit.test( 'it shouldn\'t dispatch under certain conditions', function ( assert ) {

tests/node-qunit/container.test.js

@@ -14,8 +14,7 @@ QUnit.test( '#has', function ( assert ) {
 } );
 
 QUnit.test( '#get', function ( assert ) {
-	var service = {},
+	var service = {};
-		that = this;
 
 	this.factory.returns( service );
 
@@ -39,9 +38,7 @@ QUnit.test( '#get', function ( assert ) {
 	// ---
 
 	assert.throws(
-		function () {
+		() => { this.container.get( 'bar' ); },
-			that.container.get( 'bar' );
-		},
 		/The service "bar" hasn't been defined./
 	);
 } );

tests/node-qunit/integration.test.js

@@ -8,9 +8,6 @@ import registerChangeListener from '../../src/changeListener';
 
 var mw = mediaWiki,
 	$ = jQuery,
-	// Store the real wait to be actually used in tests
-	wait = WaitModule.default,
-
 	/**
 	* Whether Gateway#getPageSummary is resolved or rejected.
 	* @enum {number}
@@ -56,15 +53,13 @@ function constant( x ) { return function () { return x; }; }
 
 QUnit.module( 'ext.popups preview @integration', {
 	beforeEach: function () {
-		var that = this;
-
 		// The worst-case implementation of mw.now.
 		mw.now = function () { return Date.now(); };
 
-		this.resetWait = function () {
+		this.resetWait = () => {
-			that.waitDeferred = $.Deferred();
+			this.waitDeferred = $.Deferred();
-			that.waitPromise = that.waitDeferred.promise();
+			this.waitPromise = this.waitDeferred.promise();
-			that.wait.returns( that.waitPromise );
+			this.wait.returns( this.waitPromise );
 		};
 
 		this.wait = this.sandbox.stub( WaitModule, 'default' );
@@ -80,8 +75,8 @@ QUnit.module( 'ext.popups preview @integration', {
 			this.store.dispatch
 		);
 
-		this.registerChangeListener = function ( fn ) {
+		this.registerChangeListener = ( fn ) => {
-			return registerChangeListener( that.store, fn );
+			return registerChangeListener( this.store, fn );
 		};
 
 		this.title = stubs.createStubTitle( 1, 'Foo' );
@@ -102,71 +97,55 @@ QUnit.module( 'ext.popups preview @integration', {
 			{ get: identity }
 		);
 
-		this.dwell = function (
+		this.dwell = (
 			title, el, ev, fetchResponse, resolution = FETCH_RESOLUTION.RESOLVE
-		) {
+		) => {
-			that.resetWait();
+			this.resetWait();
-			that.actions.linkDwell( title, el, ev, {
+			return this.actions.linkDwell( title, el, ev, {
 				getPageSummary: function () {
 					var method = resolution === FETCH_RESOLUTION.RESOLVE ?
 						'resolve' : 'reject';
 					return $.Deferred()[ method ]( fetchResponse ).promise();
 				}
 			}, function () { return 'pagetoken'; } );
-			return that.waitPromise;
 		};
 
-		this.dwellAndShowPreview = function (
+		this.dwellAndShowPreview = (
 			title, el, ev, fetchResponse, reject = FETCH_RESOLUTION.RESOLVE
-		) {
+		) => {
-			that.dwell( title, el, ev, fetchResponse, reject );
+			var dwelled = this.dwell( title, el, ev, fetchResponse, reject );
-			that.waitDeferred.resolve();
+			// Resolve the wait timeout for the linkDwell and the fetch action
-
+			this.waitDeferred.resolve();
-			// Wait for the next tick to resolve pending callbacks. N.B. that the
+			return dwelled;
-			// fetch action invokes wait twice.
-			return wait( 0 )
-				.then( function () {
-					that.waitDeferred.resolve();
-
-					return that.waitPromise;
-				} );
 		};
 
-		this.abandon = function () {
+		this.abandon = () => {
-			that.resetWait();
+			this.resetWait();
-			that.actions.abandon();
+			return this.actions.abandon();
-			return that.waitPromise;
 		};
 
-		this.abandonAndWait = function () {
+		this.abandonAndWait = () => {
-			that.abandon();
+			var abandoned = this.abandon();
-			that.waitDeferred.resolve();
+			this.waitDeferred.resolve();
-			return wait( 0 ); // Wait for next tick to resolve pending callbacks
+			return abandoned;
 		};
 
-		this.dwellAndPreviewDwell = function ( title, el, ev, res ) {
+		this.dwellAndPreviewDwell = ( title, el, ev, res ) => {
-			return that.dwellAndShowPreview( title, el, ev, res ).then( function () {
+			return this.dwellAndShowPreview( title, el, ev, res ).then( () => {
 
 				// Get out of the link, and before the delay ends...
-				var abandonPromise = that.abandon( el ),
+				var abandonPromise = this.abandon(),
-					abandonDeferred = that.waitDeferred;
+					abandonWaitDeferred = this.waitDeferred;
 
 				// Dwell over the preview
-				that.actions.previewDwell( el );
+				this.actions.previewDwell( el );
 
 				// Then the abandon delay finishes
-				abandonDeferred.resolve();
+				abandonWaitDeferred.resolve();
 
 				return abandonPromise;
 			} );
 		};
-
-		this.abandonPreview = function () {
-			that.resetWait();
-			that.actions.abandon();
-
-			return that.waitPromise;
-		};
 	}
 } );
 
@@ -224,56 +203,54 @@ QUnit.test( 'in ACTIVE state, fetch fail switches it to DATA', function ( assert
 } );
 
 QUnit.test( 'in ACTIVE state, abandon start, and then end, switch it to INACTIVE', function ( assert ) {
-	var that = this,
+	var el = this.el;
-		el = this.el;
 
 	return this.dwellAndShowPreview( this.title, el, 'event', 42 )
-		.then( function () {
+		.then( () => {
-			return that.abandonAndWait( el );
+			return this.abandonAndWait( el );
-		} ).then( function () {
+		} ).then( () => {
-			var state = that.store.getState();
+			var state = this.store.getState();
 			assert.equal( state.preview.activeLink, undefined,
 				'After abandoning, preview is back to INACTIVE' );
 		} );
 } );
 
 QUnit.test( 'in ACTIVE state, abandon link, and then dwell preview, should keep it active after all delays', function ( assert ) {
-	var that = this,
+	var el = this.el;
-		el = this.el;
 
 	return this.dwellAndPreviewDwell( this.title, el, 'event', 42 )
-		.then( function () {
+		.then( () => {
-			var state = that.store.getState();
+			var state = this.store.getState();
 			assert.equal( state.preview.activeLink, el );
 		} );
 } );
 
 QUnit.test( 'in ACTIVE state, abandon link, hover preview, back to link, should keep it active after all delays', function ( assert ) {
-	var that = this,
+	var el = this.el;
-		el = this.el;
 
+	// Dwell link, abandon it & hover preview
 	return this.dwellAndPreviewDwell( this.title, el, 'event', 42 )
-		.then( function () {
+		.then( () => {
-			var abandonPreviewDeferred, dwellPromise, dwellDeferred;
+			var abandonedPreview, abandonWaitDeferred, dwelled, dwellWaitDeferred;
 
 			// Start abandoning the preview
-			that.abandonPreview( el );
+			abandonedPreview = this.abandon();
+			abandonWaitDeferred = this.waitDeferred;
 
-			abandonPreviewDeferred = that.waitDeferred;
+			// Dwell back into the link, new event ('event2') is triggered
-			// Dwell back into the link, new event is triggered
+			dwelled = this.dwell( this.title, el, 'event2', 42 );
-			dwellPromise = that.dwell( that.title, el, 'event2', 42 );
+			dwellWaitDeferred = this.waitDeferred;
-			dwellDeferred = that.waitDeferred;
 
 			// Preview abandon happens next, before the fetch
-			abandonPreviewDeferred.resolve();
+			abandonWaitDeferred.resolve();
 
-			// Then fetch happens
+			// Then dwell wait & fetch happens
-			dwellDeferred.resolve();
+			dwellWaitDeferred.resolve();
 
-			return dwellPromise;
+			return $.when( abandonedPreview, dwelled );
 		} )
-		.then( function () {
+		.then( () => {
-			var state = that.store.getState();
+			var state = this.store.getState();
 			assert.equal( state.preview.activeLink, el );
 		} );
 } );

tests/node-qunit/previewBehavior.test.js

@@ -16,8 +16,7 @@ QUnit.module( 'ext.popups.preview.settingsBehavior', {
 } );
 
 QUnit.test( 'it should set the settingsUrl on wgPopupsBetaFeature', function ( assert ) {
-	var that = this,
+	var user = createStubUser( /* isAnon = */ false ),
-		user = createStubUser( /* isAnon = */ false ),
 		actions = {},
 		cases;
 
@@ -26,12 +25,12 @@ QUnit.test( 'it should set the settingsUrl on wgPopupsBetaFeature', function ( a
 		[ false, 'Special:Preferences#mw-prefsection-rendering' ]
 	];
 
-	$.each( cases, function ( i, testCase ) {
+	cases.forEach( ( testCase ) => {
 		var behavior;
 
-		that.config.set( 'wgPopupsBetaFeature', testCase[ 0 ] );
+		this.config.set( 'wgPopupsBetaFeature', testCase[ 0 ] );
 
-		behavior = createPreviewBehavior( that.config, user, actions );
+		behavior = createPreviewBehavior( this.config, user, actions );
 
 		assert.deepEqual(
 			behavior.settingsUrl,