Skip to content

Core: ensure process-queue advances with rejected promises #1391

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Core: ensure process-queue advances with rejected promises #1391

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Gruntfile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ module.exports = function( grunt ) {
"test/reorderError2.html",
"test/callbacks.html",
"test/callbacks-promises.html",
"test/callbacks-rejected-promises.html",
"test/events.html",
"test/events-in-test.html",
"test/logs.html",
Expand Down
9 changes: 4 additions & 5 deletions reporter/html.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -858,16 +858,15 @@ export function escapeText( s ) {
} );

QUnit.testDone( function( details ) {
var testTitle, time, testItem, assertList, status,
var testTitle, time, assertList, status,
good, bad, testCounts, skipped, sourceName,
tests = id( "qunit-tests" );
tests = id( "qunit-tests" ),
testItem = id( "qunit-test-output-" + details.testId );

if ( !tests ) {
if ( !tests || !testItem ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've extracted and applied this, with credit, via #1562 (released since then).

return;
}

testItem = id( "qunit-test-output-" + details.testId );

removeClass( testItem, "running" );

if ( details.failed > 0 ) {
Expand Down
5 changes: 4 additions & 1 deletion src/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,10 @@ export function begin() {
runLoggingCallbacks( "begin", {
totalTests: Test.count,
modules: modulesLog
} ).then( unblockAndAdvanceQueue );
} ).then( unblockAndAdvanceQueue, function( err ) {
setTimeout( unblockAndAdvanceQueue );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added tests for this scenario in #1634, and addressed the bug this fixes (#1446) via a different approach in #1638. In a nutshell, we handle this via the unhandledrejection event in the browser and Node.js, same as before, but previously that didn't work right due to it creating a fake test which we no longer do.

throw err;
} );
} else {
unblockAndAdvanceQueue();
}
Expand Down
54 changes: 32 additions & 22 deletions src/core/logging.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,27 +1,42 @@
import config from "./config";
import { objectType } from "./utilities";
import Promise from "../promise";
import {
setTimeout
} from "../globals";

function _promisfyCallbacksSequentially( callbacks, args ) {
return callbacks.reduce( ( promiseChain, callback ) => {
const executeCallback = () => callback( args );
return promiseChain.then(
executeCallback,
( err ) => {
setTimeout( executeCallback );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use of setTimeout must be conditional for non-browser environments such as SpiderMonkey (see other references). An alternate approach here can be to use Promise.resolve() as fallback for setTimeout, or to avoid this alltogether by using Promise.finally() for the executeCallback, which naturally forwards the original rejection/error.

throw err;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect that this particular thenable chain might be better without a local error handler as otherwise it means that the last error will be reported instead of the first one. I general QUnit's test logic is modelled to continue after assertion failures, but for uncaught exceptions I think we generally want to stop after the first for any given sequence.

}
);
}, Promise.resolve( [] ) );
}

function _registerLoggingCallback( key ) {
const loggingCallback = ( callback ) => {
if ( objectType( callback ) !== "function" ) {
throw new Error(
"QUnit logging methods require a callback function as their first parameters."
);
}

config.callbacks[ key ].push( callback );
};

return loggingCallback;
}

// Register logging callbacks
export function registerLoggingCallbacks( obj ) {
var i, l, key,
callbackNames = [ "begin", "done", "log", "testStart", "testDone",
"moduleStart", "moduleDone" ];

function registerLoggingCallback( key ) {
var loggingCallback = function( callback ) {
if ( objectType( callback ) !== "function" ) {
throw new Error(
"QUnit logging methods require a callback function as their first parameters."
);
}

config.callbacks[ key ].push( callback );
};

return loggingCallback;
}

for ( i = 0, l = callbackNames.length; i < l; i++ ) {
key = callbackNames[ i ];

Expand All @@ -30,7 +45,7 @@ export function registerLoggingCallbacks( obj ) {
config.callbacks[ key ] = [];
}

obj[ key ] = registerLoggingCallback( key );
obj[ key ] = _registerLoggingCallback( key );
}
}

Expand All @@ -46,10 +61,5 @@ export function runLoggingCallbacks( key, args ) {
return;
}

// ensure that each callback is executed serially
return callbacks.reduce( ( promiseChain, callback ) => {
return promiseChain.then( () => {
return Promise.resolve( callback( args ) );
} );
}, Promise.resolve( [] ) );
return _promisfyCallbacksSequentially( callbacks, args );
}
45 changes: 29 additions & 16 deletions src/core/processing-queue.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,13 +61,23 @@ function processTaskQueue( start ) {

if ( !defined.setTimeout || config.updateRate <= 0 || elapsedTime < config.updateRate ) {
const task = taskQueue.shift();
Promise.resolve( task() ).then( function() {
const processNextTaskOrAdvance = () => {
if ( !taskQueue.length ) {
advance();
} else {
processTaskQueue( start );
}
} );
};
const throwAndAdvance = ( err ) => {
setTimeout( advance );
throw err;
};

// Without throwAndAdvance, qunit does not continue advanceing the processing queue if
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

advanceing -> advancing

// task() throws an error
Promise.resolve( task() )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If task() throws a synchronous error, the catch bellow which would invoke throwAndAdvance won't be invoked. I'm guessing this isn't intentional, and regardless if task throws synchronously or asynchronously, we would expect the catchAndAdance to be invoked.

The following would rectify that issue.

new Promise(resolve => resolve(task()).then(processNextTaskOrAdance).catch(throwAndAdvance)

.then( processNextTaskOrAdvance, throwAndAdvance )
.catch( throwAndAdvance );
Copy link
Contributor

@stefanpenner stefanpenner May 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to have throwAndAdvance both in the then's rejection handler, and in the subsequent catch?

As implemented if the throwAndAdvance on line 79 is invoked, it will also cause the throwAndAdvance on line 80 to be invoked.

I believe the intention here may be:

then(processNextTaskOrAdvance).catch(throwAndAdvance)

} else {
setTimeout( advance );
}
Expand Down Expand Up @@ -153,13 +163,25 @@ function unitSamplerGenerator( seed ) {
};
}

// Clear own storage items when tests completes
function cleanStorage() {
const storage = config.storage;
if ( storage && config.stats.bad === 0 ) {
for ( let i = storage.length - 1; i >= 0; i-- ) {
const key = storage.key( i );

if ( key.indexOf( "qunit-test-" ) === 0 ) {
storage.removeItem( key );
}
}
}
}

/**
* This function is called when the ProcessingQueue is done processing all
* items. It handles emitting the final run events.
*/
function done() {
const storage = config.storage;

ProcessingQueue.finished = true;

const runtime = now() - config.started;
Expand Down Expand Up @@ -193,18 +215,9 @@ function done() {
failed: config.stats.bad,
total: config.stats.all,
runtime
} ).then( () => {

// Clear own storage items if all tests passed
if ( storage && config.stats.bad === 0 ) {
for ( let i = storage.length - 1; i >= 0; i-- ) {
const key = storage.key( i );

if ( key.indexOf( "qunit-test-" ) === 0 ) {
storage.removeItem( key );
}
}
}
} ).then( cleanStorage, function( err ) {
cleanStorage();
throw err;
} );
}

Expand Down
76 changes: 49 additions & 27 deletions src/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,19 +118,25 @@ Test.prototype = {

// ensure the callbacks are executed serially for each module
var callbackPromises = notStartedModules.reduce( ( promiseChain, startModule ) => {
return promiseChain.then( () => {
const moduleStartCallback = () => {
startModule.stats = { all: 0, bad: 0, started: now() };
emit( "suiteStart", startModule.suiteReport.start( true ) );
return runLoggingCallbacks( "moduleStart", {
name: startModule.name,
tests: startModule.tests
} );
} );
};

return promiseChain.then( moduleStartCallback, moduleStartCallback );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this will swallow any errors at this point - is that the desired behavior here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikrostew I assuming you are talking about const callbackPromises = notStartedModules.reduce(...); ?

if any of the promise from callbackPromises reject.. the whole promise should reject.. which should be handled on line 154:

return callbackPromises.then( testStartHandler, ( err ) => {
		return Promise.reject( err ).catch( ( err ) => {
			setTimeout( testStartHandler );
			throw err;
		} );
	} );

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was specifically looking at the line

return promiseChain.then( moduleStartCallback, moduleStartCallback );

If something in promiseChain resolves, then it calls the first function argument moduleStartCallback, no problem. But it looks like if it rejects (because of some error) it will call the second function argument, which is also moduleStartCallback, which doesn't do anything with the error that it receives. So that rejection will stop there, so that callbackPromises won't reject.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read this again, this shouldn't be a problem, but I can understand why it looks like the error is swallowed.
For a bit more context.. const callbackPromises = notStartedModules.reduce(...); ensures the callbacks from notStartedModules are promisfied sequentually, so each callback that was made into a promise, an awaited for until the next callback is executed and made into a promise.
so You will get an chain of promises. In that chain, if there is one rejected promise, then the whole promise chain is rejected.

return promiseChain.then( moduleStartCallback, moduleStartCallback ); in this case, only ensures the next callback is promisfied even if the previous promise rejects. But as stated above, lien 154 will ensure the error is thrown.

I hope that makes more sense.. I think I should likely find a better way to code the explanation above up

}, Promise.resolve( [] ) );

return callbackPromises.then( () => {
const testStartHandler = () => {
config.current = this;

const testStartResolvedHandler = () => {
if ( !config.pollution ) {
saveGlobal();
}
};
this.testEnvironment = extend( {}, module.testEnvironment );

this.started = now();
Expand All @@ -140,10 +146,16 @@ Test.prototype = {
module: module.name,
testId: this.testId,
previousFailure: this.previousFailure
} ).then( () => {
if ( !config.pollution ) {
saveGlobal();
}
} ).then( testStartResolvedHandler, function( err ) {
setTimeout( testStartResolvedHandler );
throw err;
} );
};

return callbackPromises.then( testStartHandler, ( err ) => {
return Promise.reject( err ).catch( ( err ) => {
setTimeout( testStartHandler );
throw err;
} );
} );
},
Expand Down Expand Up @@ -319,23 +331,7 @@ Test.prototype = {
emit( "testEnd", this.testReport.end( true ) );
this.testReport.slimAssertions();

return runLoggingCallbacks( "testDone", {
name: testName,
module: moduleName,
skipped: skipped,
todo: todo,
failed: bad,
passed: this.assertions.length - bad,
total: this.assertions.length,
runtime: skipped ? 0 : this.runtime,

// HTML Reporter use
assertions: this.assertions,
testId: this.testId,

// Source of Test
source: this.stack
} ).then( function() {
const testDoneResolvedHandler = function() {
if ( module.testsRun === numberOfTests( module ) ) {
const completedModules = [ module ];

Expand All @@ -353,8 +349,35 @@ Test.prototype = {
} );
}, Promise.resolve( [] ) );
}
} ).then( function() {
};

return runLoggingCallbacks( "testDone", {
name: testName,
module: moduleName,
skipped: skipped,
todo: todo,
failed: bad,
passed: this.assertions.length - bad,
total: this.assertions.length,
runtime: skipped ? 0 : this.runtime,

// HTML Reporter use
assertions: this.assertions,
testId: this.testId,

// Source of Test
source: this.stack
} ).then(
testDoneResolvedHandler,
function( err ) {
setTimeout( testDoneResolvedHandler );
throw err;
}
).then( function() {
config.current = undefined;
}, function( err ) {
config.current = undefined;
throw err;
} );

function logSuiteEnd( module ) {
Expand Down Expand Up @@ -660,7 +683,6 @@ function checkPollution() {
if ( newGlobals.length > 0 ) {
pushFailure( "Introduced global variable(s): " + newGlobals.join( ", " ) );
}

deletedGlobals = diff( old, config.pollution );
if ( deletedGlobals.length > 0 ) {
pushFailure( "Deleted global variable(s): " + deletedGlobals.join( ", " ) );
Expand Down
13 changes: 13 additions & 0 deletions test/callbacks-rejected-promises.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="UTF-8">
<title>QUnit Callbacks Test Suite</title>
<link rel="stylesheet" href="../dist/qunit.css">
<script src="../dist/qunit.js"></script>
<script src="callbacks-rejected-promises.js"></script>
</head>
<body>
<div id="qunit"></div>
</body>
</html>
Loading