From 2b36e3f760bd333a58076b705c999fcd217346df Mon Sep 17 00:00:00 2001 From: step2yeung Date: Mon, 29 Apr 2019 09:49:15 -0700 Subject: [PATCH] Core: Advance the processQueue when errors are thrown in callbacks --- Gruntfile.js | 1 + reporter/html.js | 9 +- src/core.js | 5 +- src/core/logging.js | 83 ++++++++++----- src/core/processing-queue.js | 72 +++++++------ src/test.js | 120 ++++++++++++++-------- test/callbacks-rejected-promises.html | 13 +++ test/callbacks-rejected-promises.js | 71 +++++++++++++ test/cli/fixtures/expected/tap-outputs.js | 2 + 9 files changed, 269 insertions(+), 107 deletions(-) create mode 100644 test/callbacks-rejected-promises.html create mode 100644 test/callbacks-rejected-promises.js diff --git a/Gruntfile.js b/Gruntfile.js index d291ed890..8c5e6068e 100644 --- a/Gruntfile.js +++ b/Gruntfile.js @@ -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", diff --git a/reporter/html.js b/reporter/html.js index 5d8b7ffbd..d772b9e12 100644 --- a/reporter/html.js +++ b/reporter/html.js @@ -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 ) { return; } - testItem = id( "qunit-test-output-" + details.testId ); - removeClass( testItem, "running" ); if ( details.failed > 0 ) { diff --git a/src/core.js b/src/core.js index ec616c685..d9de29a0a 100644 --- a/src/core.js +++ b/src/core.js @@ -176,7 +176,10 @@ export function begin() { runLoggingCallbacks( "begin", { totalTests: Test.count, modules: modulesLog - } ).then( unblockAndAdvanceQueue ); + } ).then( unblockAndAdvanceQueue, function( err ) { + setTimeout( unblockAndAdvanceQueue ); + throw err; + } ); } else { unblockAndAdvanceQueue(); } diff --git a/src/core/logging.js b/src/core/logging.js index 154f89c97..f1e18a9bf 100644 --- a/src/core/logging.js +++ b/src/core/logging.js @@ -1,41 +1,73 @@ import config from "./config"; import { objectType } from "./utilities"; import Promise from "../promise"; +import { + setTimeout +} from "../globals"; -// 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." - ); +const callbackNames = [ + "begin", + "done", + "log", + "testStart", + "testDone", + "moduleStart", + "moduleDone" +]; + +function _promisfyCallbacksSequentially( callbacks, args ) { + return callbacks.reduce( ( promiseChain, callback ) => { + return promiseChain.then( + () => callback( args ), + ( err ) => { + setTimeout( callback( args ) ); + throw err; } + ); + }, Promise.resolve( [] ) ); +} - config.callbacks[ key ].push( callback ); - }; +function _registerLoggingCallback( key ) { + const loggingCallback = ( callback ) => { + if ( objectType( callback ) !== "function" ) { + throw new Error( + "QUnit logging methods require a callback function as their first parameters." + ); + } - return loggingCallback; - } + config.callbacks[ key ].push( callback ); + }; - for ( i = 0, l = callbackNames.length; i < l; i++ ) { - key = callbackNames[ i ]; + return loggingCallback; +} + +/** + * Register logging callbacks to qunit + * + * @export + * @param {Object} qunit + */ +export function registerLoggingCallbacks( qunit ) { + callbackNames.forEach( key => { // Initialize key collection of logging callback if ( objectType( config.callbacks[ key ] ) === "undefined" ) { config.callbacks[ key ] = []; } - - obj[ key ] = registerLoggingCallback( key ); - } + qunit[ key ] = _registerLoggingCallback( key ); + } ); } +/** + * execute all callbacks from the given key as a sequential list of promises + * + * @export + * @param {String} key + * @param {Object} args + * @returns {Promise} + */ export function runLoggingCallbacks( key, args ) { - var callbacks = config.callbacks[ key ]; + const callbacks = config.callbacks[ key ]; // Handling 'log' callbacks separately. Unlike the other callbacks, // the log callback is not controlled by the processing queue, @@ -46,10 +78,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 ); } diff --git a/src/core/processing-queue.js b/src/core/processing-queue.js index 6b88acdee..c850a04cd 100644 --- a/src/core/processing-queue.js +++ b/src/core/processing-queue.js @@ -61,13 +61,19 @@ 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 new Error( err ); + }; + + Promise.resolve( task() ).then( processNextTaskOrAdvance, throwAndAdvance ); } else { setTimeout( advance ); } @@ -153,20 +159,9 @@ function unitSamplerGenerator( seed ) { }; } -/** - * 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; - const passed = config.stats.all - config.stats.bad; - +// Throws corresponding error if no tests were executed +function throwErrorIfNoTestExecuted() { if ( config.stats.all === 0 ) { - if ( config.filter && config.filter.length ) { throw new Error( `No tests matched the filter "${config.filter}".` ); } @@ -182,29 +177,46 @@ function done() { if ( config.testId && config.testId.length ) { throw new Error( `No tests matched the testId "${config.testId}".` ); } - throw new Error( "No tests were run." ); + } +} + +// 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() { + ProcessingQueue.finished = true; + + try { + throwErrorIfNoTestExecuted(); + } catch ( err ) { + throw err; } emit( "runEnd", globalSuite.end( true ) ); runLoggingCallbacks( "done", { - passed, + passed: config.stats.all - config.stats.bad, 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 ); - } - } - } + runtime: now() - config.started + } ).then( cleanStorage, function( err ) { + cleanStorage(); + throw err; } ); } diff --git a/src/test.js b/src/test.js index f28b233d4..b41e3a430 100644 --- a/src/test.js +++ b/src/test.js @@ -113,24 +113,30 @@ function getNotStartedModules( startModule ) { Test.prototype = { before: function() { - var module = this.module, + const module = this.module, notStartedModules = getNotStartedModules( module ); // ensure the callbacks are executed serially for each module - var callbackPromises = notStartedModules.reduce( ( promiseChain, startModule ) => { - return promiseChain.then( () => { + const callbackPromises = notStartedModules.reduce( ( promiseChain, startModule ) => { + 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 ); }, Promise.resolve( [] ) ); - return callbackPromises.then( () => { + const testStartHandler = () => { config.current = this; - + const testStartResolvedHandler = () => { + if ( !config.pollution ) { + saveGlobal(); + } + }; this.testEnvironment = extend( {}, module.testEnvironment ); this.started = now(); @@ -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; } ); } ); }, @@ -163,16 +175,7 @@ Test.prototype = { try { runTest( this ); } catch ( e ) { - this.pushFailure( "Died on test #" + ( this.assertions.length + 1 ) + " " + - this.stack + ": " + ( e.message || e ), extractStacktrace( e, 0 ) ); - - // Else next test will carry the responsibility - saveGlobal(); - - // Restart the tests if they're blocking - if ( config.blocking ) { - internalRecover( this ); - } + testFailureHandler( e ); } function runTest( test ) { @@ -188,6 +191,19 @@ Test.prototype = { ); } } + + function testFailureHandler( e ) { + this.pushFailure( "Died on test #" + ( this.assertions.length + 1 ) + " " + + this.stack + ": " + ( e.message || e ), extractStacktrace( e, 0 ) ); + + // Else next test will carry the responsibility + saveGlobal(); + + // Restart the tests if they're blocking + if ( config.blocking ) { + internalRecover( this ); + } + } }, after: function() { @@ -319,23 +335,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 ]; @@ -353,8 +353,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 ) { @@ -652,7 +679,7 @@ function saveGlobal() { function checkPollution() { var newGlobals, deletedGlobals, - old = config.pollution; + old = config.pollution || []; saveGlobal(); @@ -773,8 +800,7 @@ function internalRecover( test ) { internalStart( test ); } -// Release a processing hold, scheduling a resumption attempt if no holds remain. -function internalStart( test ) { +function validateSemaphore( test ) { // If semaphore is non-numeric, throw error if ( isNaN( test.semaphore ) ) { @@ -784,12 +810,12 @@ function internalStart( test ) { "Invalid value on test.semaphore", sourceFromStacktrace( 2 ) ); - return; + return true; } // Don't start until equal number of stop-calls if ( test.semaphore > 0 ) { - return; + return true; } // Throw an Error if start is called more often than stop @@ -800,6 +826,14 @@ function internalStart( test ) { "Tried to restart test while already started (test's semaphore was 0 already)", sourceFromStacktrace( 2 ) ); + return true; + } + return false; +} + +// Release a processing hold, scheduling a resumption attempt if no holds remain. +function internalStart( test ) { + if ( validateSemaphore( test ) ) { return; } diff --git a/test/callbacks-rejected-promises.html b/test/callbacks-rejected-promises.html new file mode 100644 index 000000000..f18c75754 --- /dev/null +++ b/test/callbacks-rejected-promises.html @@ -0,0 +1,13 @@ + + + + + QUnit Callbacks Test Suite + + + + + +
+ + diff --git a/test/callbacks-rejected-promises.js b/test/callbacks-rejected-promises.js new file mode 100644 index 000000000..5839db4b8 --- /dev/null +++ b/test/callbacks-rejected-promises.js @@ -0,0 +1,71 @@ +var done = false; +var errorsThrown = []; + +QUnit.onUnhandledRejection = function( e ) { + errorsThrown.push( e.message ); +}; + +QUnit.begin( function() { + return new Promise( function( resolve, reject ) { + reject( new Error( "begin" ) ); + } ); +} ); + +QUnit.moduleStart( function() { + return new Promise( function( resolve, reject ) { + reject( new Error( "moduleStart" ) ); + } ); +} ); + +QUnit.testStart( function() { + return new Promise( function( resolve, reject ) { + reject( new Error( "testStart" ) ); + } ); +} ); + +// TODO: error here will give a infinite loop of global errors +// because onUnhandledRejection will add another test(), which +// will call testDone callbacks again, which throws another onUnhandledRejection +QUnit.testDone( function() { + return new Promise( function( resolve, reject ) { + reject( new Error( "testDone" ) ); + } ); +} ); + +QUnit.moduleDone( function() { + return new Promise( function( resolve, reject ) { + reject( new Error( "moduleDone" ) ); + } ); +} ); + +QUnit.done( function() { + return new Promise( function( resolve, reject ) { + reject( new Error( "done" ) ); + } ); +} ); + +QUnit.done( function() { + if ( done ) { + return; + } + + done = true; + + QUnit.test( "errorCaught should be true", function( assert ) { + assert.deepEqual( errorsThrown, [ + "begin", + "moduleStart", + "testStart", + "testDone", + "moduleDone", + "done", + "moduleStart" + ] ); + } ); +} ); + +QUnit.module( "module1", function() { + QUnit.test( "test pass", function( assert ) { + assert.ok( true ); + } ); +} ); diff --git a/test/cli/fixtures/expected/tap-outputs.js b/test/cli/fixtures/expected/tap-outputs.js index 7e4719512..ec2786639 100644 --- a/test/cli/fixtures/expected/tap-outputs.js +++ b/test/cli/fixtures/expected/tap-outputs.js @@ -153,6 +153,7 @@ not ok 1 global failure actual: {} expected: undefined stack: Error: No tests were run. + at throwErrorIfNoTestExecuted (.*) at done (.*) at advanceTestQueue (.*) at Object.advance (.*) @@ -196,6 +197,7 @@ not ok 1 global failure actual: {} expected: undefined stack: Error: No tests matched the filter "no matches". + at throwErrorIfNoTestExecuted (.*) at done (.*) at advanceTestQueue (.*) at Object.advance (.*)