Improve & fix action and integration tests

This change fixes some issues with assertions not running, removes
unnecessary promise dances, and improves legibility and some code
patterns in the action and integration node tests mainly.

Detailed changes:

* actions.js
  * Fully migrate out of jQuery 1 promises (no done/fail)
  * Fix linkDwell action not returning the fetch action promise

* actions.test.js
  * Simplify setupWait for the tests
    * It always autoresolves immediately the wait call to ensure speedy tests
    * No waitDeferreds or waitPromises array coordination, rely on action
      returned promises instead
  * Get rid of that = this in favor of arrow functions
  * Rename generic "p" promises to meaningful names
  * Add assert.expect for more solid tests (so that we don't skip assertions in
    the future if we change them)
  * Fix some assertions that weren't being run because of the incorrect promise
    being returned (p.then, and then just returning p)
  * Get rid of $.when stubbing in favor of waiting for the promise returned from
    the action
    * Get rid of hacky setTimeout(..., 0) to run assertions after the promises

* integration.test.js
  * Get rid of wait(0) calls to hack around asynchronous actions
    * Use the action returned promises instead of the waitPromises/Deferreds
  * Remove unused "el" parameter being passed to this.abandon in several tests
  * Remove unnecessary test helper this.abandonPreview (it was the same as
    this.abandon)
  * Clarify a bit the last and more complex test with some comments and variable
    name changes
  * Get rid of that=this in favor of arrow functions

* container.test.js
  * Get rid of that=this in favor of arrow functions

* previewBehavior.test.js
  * Get rid of that=this in favor of arrow functions
  * Get rid of $.each in favor of .forEach

Bug: T170807
Change-Id: I06fafd92a1712f143018e2ddff24fadf1a6882b3
This commit is contained in:
joakin 2018-02-16 13:52:56 +01:00
parent 2df22ef2b2
commit 1fff0d2ea7
7 changed files with 135 additions and 209 deletions

Binary file not shown.

Binary file not shown.

View file

@ -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 ) );
}
} );
};

View file

@ -67,19 +67,10 @@ QUnit.test( '#boot', function ( assert ) {
* @param {Object} module
*/
function setupWait( module ) {
module.waitDeferreds = [];
module.waitPromises = [];
module.wait = module.sandbox.spy( function () {
var deferred = $.Deferred(),
promise = deferred.promise();
module.waitDeferreds.push( deferred );
module.waitPromises.push( promise );
return promise;
module.waitPromise = $.Deferred().resolve().promise();
module.wait = module.sandbox.stub( WaitModule, 'default' ).callsFake( function () {
return module.waitPromise;
} );
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 () {
return that.state;
};
this.getState = () => this.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,
event = {},
var 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 () {
return that.now;
} );
this.sandbox.stub( mw, 'now' ).callsFake( () => this.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
)( that.dispatch );
this.gateway, this.title, this.el, this.token
)( 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(),
whenSpy,
args,
result = {},
that = this;
var result = {},
fetched = this.fetch();
whenSpy = this.sandbox.stub( $, 'when' )
.returns( whenDeferred.promise() );
this.fetch();
assert.expect( 2 );
assert.strictEqual(
this.wait.getCall( 0 ).args[ 0 ],
350,
'It waits for FETCH_COMPLETE_TARGET_DELAY - FETCH_START_DELAY milliseconds.'
);
this.gatewayDeferred.resolve( result );
// ---
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 ],
{
type: 'FETCH_COMPLETE',
el: that.el,
result: result,
token: that.token
}
);
}, 0 );
return fetched.then( () => {
assert.deepEqual(
this.dispatch.getCall( 2 ).args[ 0 ],
{
type: 'FETCH_COMPLETE',
el: this.el,
result: result,
token: this.token
}
);
} );
} );
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 );
actions.abandon( this.el )( dispatch, getState );
abandoned = 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 ) {

View file

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

View file

@ -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 () {
that.waitDeferred = $.Deferred();
that.waitPromise = that.waitDeferred.promise();
that.wait.returns( that.waitPromise );
this.resetWait = () => {
this.waitDeferred = $.Deferred();
this.waitPromise = this.waitDeferred.promise();
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 ) {
return registerChangeListener( that.store, fn );
this.registerChangeListener = ( 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();
that.actions.linkDwell( title, el, ev, {
) => {
this.resetWait();
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 );
that.waitDeferred.resolve();
// Wait for the next tick to resolve pending callbacks. N.B. that the
// fetch action invokes wait twice.
return wait( 0 )
.then( function () {
that.waitDeferred.resolve();
return that.waitPromise;
} );
) => {
var dwelled = this.dwell( title, el, ev, fetchResponse, reject );
// Resolve the wait timeout for the linkDwell and the fetch action
this.waitDeferred.resolve();
return dwelled;
};
this.abandon = function () {
that.resetWait();
that.actions.abandon();
return that.waitPromise;
this.abandon = () => {
this.resetWait();
return this.actions.abandon();
};
this.abandonAndWait = function () {
that.abandon();
that.waitDeferred.resolve();
return wait( 0 ); // Wait for next tick to resolve pending callbacks
this.abandonAndWait = () => {
var abandoned = this.abandon();
this.waitDeferred.resolve();
return abandoned;
};
this.dwellAndPreviewDwell = function ( title, el, ev, res ) {
return that.dwellAndShowPreview( title, el, ev, res ).then( function () {
this.dwellAndPreviewDwell = ( title, el, ev, res ) => {
return this.dwellAndShowPreview( title, el, ev, res ).then( () => {
// Get out of the link, and before the delay ends...
var abandonPromise = that.abandon( el ),
abandonDeferred = that.waitDeferred;
var abandonPromise = this.abandon(),
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,
el = this.el;
var el = this.el;
return this.dwellAndShowPreview( this.title, el, 'event', 42 )
.then( function () {
return that.abandonAndWait( el );
} ).then( function () {
var state = that.store.getState();
.then( () => {
return this.abandonAndWait( el );
} ).then( () => {
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,
el = this.el;
var el = this.el;
return this.dwellAndPreviewDwell( this.title, el, 'event', 42 )
.then( function () {
var state = that.store.getState();
.then( () => {
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,
el = this.el;
var el = this.el;
// Dwell link, abandon it & hover preview
return this.dwellAndPreviewDwell( this.title, el, 'event', 42 )
.then( function () {
var abandonPreviewDeferred, dwellPromise, dwellDeferred;
.then( () => {
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 is triggered
dwellPromise = that.dwell( that.title, el, 'event2', 42 );
dwellDeferred = that.waitDeferred;
// Dwell back into the link, new event ('event2') is triggered
dwelled = this.dwell( this.title, el, 'event2', 42 );
dwellWaitDeferred = this.waitDeferred;
// Preview abandon happens next, before the fetch
abandonPreviewDeferred.resolve();
abandonWaitDeferred.resolve();
// Then fetch happens
dwellDeferred.resolve();
// Then dwell wait & fetch happens
dwellWaitDeferred.resolve();
return dwellPromise;
return $.when( abandonedPreview, dwelled );
} )
.then( function () {
var state = that.store.getState();
.then( () => {
var state = this.store.getState();
assert.equal( state.preview.activeLink, el );
} );
} );

View file

@ -16,8 +16,7 @@ QUnit.module( 'ext.popups.preview.settingsBehavior', {
} );
QUnit.test( 'it should set the settingsUrl on wgPopupsBetaFeature', function ( assert ) {
var that = this,
user = createStubUser( /* isAnon = */ false ),
var 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,