From 336b32e53438466c381d46ba64d777c29d30c42f Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Mon, 18 Dec 2017 01:07:57 -0500 Subject: [PATCH 1/8] CLI: Ensure an unhandled rejection results in a failed test. --- bin/run.js | 5 +++++ reporter/html.js | 1 - test/cli/fixtures/expected/tap-outputs.js | 18 +++++++++++++++++- test/cli/fixtures/unhandled-rejection.js | 19 +++++++++++++++++++ test/cli/main.js | 16 ++++++++++++++++ 5 files changed, 57 insertions(+), 2 deletions(-) create mode 100644 test/cli/fixtures/unhandled-rejection.js diff --git a/bin/run.js b/bin/run.js index 6a627d653..9a0dcc4fb 100644 --- a/bin/run.js +++ b/bin/run.js @@ -33,6 +33,11 @@ function run( args, options ) { } } ); + // Listen for unhandled rejections, and call QUnit.onError. + process.on( "unhandledRejection", function( reason ) { + QUnit.onError( reason ); + } ); + const files = utils.getFilesFromArgs( args ); QUnit = requireQUnit(); diff --git a/reporter/html.js b/reporter/html.js index aeeb8a0a4..facea8789 100644 --- a/reporter/html.js +++ b/reporter/html.js @@ -979,5 +979,4 @@ export function escapeText( s ) { return ret; }; - }() ); diff --git a/test/cli/fixtures/expected/tap-outputs.js b/test/cli/fixtures/expected/tap-outputs.js index 435ca6a94..88069b6a8 100644 --- a/test/cli/fixtures/expected/tap-outputs.js +++ b/test/cli/fixtures/expected/tap-outputs.js @@ -89,6 +89,22 @@ Available custom reporters from dependencies are: npm-reporter /* eslint-disable max-len */ "qunit hanging-test": `Error: Process exited before tests finished running Last test to run (hanging) has an async hold. Ensure all assert.async() callbacks are invoked and Promises resolve. You should also set a standard timeout via QUnit.config.testTimeout. -` +`, /* eslint-enable max-len */ + "qunit unhandled-rejection.js": +`TAP version 13 +not ok 1 Unhandled Rejections > test passes just fine, but has a rejected promise + --- + message: "Error thrown in non-returned promise!" + severity: failed + actual: null + expected: undefined + stack: undefined:undefined + ... +1..1 +# pass 0 +# skip 0 +# todo 0 +# fail 1 +` }; diff --git a/test/cli/fixtures/unhandled-rejection.js b/test/cli/fixtures/unhandled-rejection.js new file mode 100644 index 000000000..8236e3971 --- /dev/null +++ b/test/cli/fixtures/unhandled-rejection.js @@ -0,0 +1,19 @@ +"use strict"; + +QUnit.module( "Unhandled Rejections", function() { + QUnit.test( "test passes just fine, but has a rejected promise", function( assert ) { + assert.ok( true ); + + const done = assert.async(); + + new Promise( function( resolve ) { + setTimeout( resolve ); + } ) + .then( function() { + throw new Error( "Error thrown in non-returned promise!" ); + } ); + + // prevent test from exiting before unhandled rejection fires + setTimeout( done, 10 ); + } ); +} ); diff --git a/test/cli/main.js b/test/cli/main.js index 394dd1ee0..6e153199f 100644 --- a/test/cli/main.js +++ b/test/cli/main.js @@ -105,6 +105,22 @@ QUnit.module( "CLI Main", function() { } } ) ); + QUnit.test( "unhandled rejections fail tests", co.wrap( function* ( assert ) { + const command = "qunit unhandled-rejection.js"; + + try { + const result = yield execute( command ); + assert.pushResult( { + result: false, + actual: result.stdout + } ); + } catch ( e ) { + assert.equal( e.code, 1 ); + assert.equal( e.stderr, "" ); + assert.equal( e.stdout, expectedOutput[ command ] ); + } + } ) ); + QUnit.module( "filter", function() { QUnit.test( "can properly filter tests", co.wrap( function* ( assert ) { const command = "qunit --filter 'single' test single.js 'glob/**/*-test.js'"; From bc23a658a5dd3b228f4fc73427d76af534fc3785 Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Mon, 18 Dec 2017 01:11:50 -0500 Subject: [PATCH 2/8] HTML Reporter: Ensure unhandled rejection fails. Brings unhandled rejections in line with unhandled exceptions. --- reporter/html.js | 5 ++++ test/index.html | 1 + test/reporter-html/unhandled-rejection.js | 36 +++++++++++++++++++++++ 3 files changed, 42 insertions(+) create mode 100644 test/reporter-html/unhandled-rejection.js diff --git a/reporter/html.js b/reporter/html.js index facea8789..237b70eab 100644 --- a/reporter/html.js +++ b/reporter/html.js @@ -979,4 +979,9 @@ export function escapeText( s ) { return ret; }; + + // Listen for unhandled rejections, and call QUnit.onError. + window.addEventListener( "unhandledrejection", function( event ) { + QUnit.onError( event.reason ); + } ); }() ); diff --git a/test/index.html b/test/index.html index 64b4024b2..36bb9f3da 100644 --- a/test/index.html +++ b/test/index.html @@ -19,6 +19,7 @@ + diff --git a/test/reporter-html/unhandled-rejection.js b/test/reporter-html/unhandled-rejection.js new file mode 100644 index 000000000..8624c1a98 --- /dev/null +++ b/test/reporter-html/unhandled-rejection.js @@ -0,0 +1,36 @@ +// Detect if the current browser supports `onunhandledrejection` (avoiding +// errors for browsers without the capability) +const HAS_UNHANDLED_REJECTION_HANDLER = "onunhandledrejection" in window; + +QUnit.module( "Unhandled Rejections", function( hooks ) { + var originalQUnitOnError; + + hooks.beforeEach( function() { + originalQUnitOnError = QUnit.onError; + } ); + + hooks.afterEach( function() { + QUnit.onError = originalQUnitOnError; + } ); + + if ( HAS_UNHANDLED_REJECTION_HANDLER ) { + QUnit.test( "test passes just fine, but has a rejected promise", function( assert ) { + + QUnit.onError = function() { + assert.ok( true, "QUnit.onError was called" ); + }; + + const done = assert.async(); + + new Promise( function( resolve ) { + setTimeout( resolve ); + } ) + .then( function() { + throw new Error( "Error thrown in non-returned promise!" ); + } ); + + // prevent test from exiting before unhandled rejection fires + setTimeout( done, 10 ); + } ); + } +} ); From 42eebf7a0bbf0e84628d1cda6f787964ca9520ad Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Mon, 18 Dec 2017 13:37:53 -0500 Subject: [PATCH 3/8] Core: Add QUnit.onUnhandledRejection. --- bin/run.js | 4 ++-- reporter/html.js | 4 ++-- src/core.js | 5 ++++- src/core/on-unhandled-rejection.js | 24 ++++++++++++++++++++++++ 4 files changed, 32 insertions(+), 5 deletions(-) create mode 100644 src/core/on-unhandled-rejection.js diff --git a/bin/run.js b/bin/run.js index 9a0dcc4fb..6125225da 100644 --- a/bin/run.js +++ b/bin/run.js @@ -33,9 +33,9 @@ function run( args, options ) { } } ); - // Listen for unhandled rejections, and call QUnit.onError. + // Listen for unhandled rejections, and call QUnit.onUnhandledRejection process.on( "unhandledRejection", function( reason ) { - QUnit.onError( reason ); + QUnit.onUnhandledRejection( reason ); } ); const files = utils.getFilesFromArgs( args ); diff --git a/reporter/html.js b/reporter/html.js index 237b70eab..ecc3277c0 100644 --- a/reporter/html.js +++ b/reporter/html.js @@ -980,8 +980,8 @@ export function escapeText( s ) { return ret; }; - // Listen for unhandled rejections, and call QUnit.onError. + // Listen for unhandled rejections, and call QUnit.onUnhandledRejection window.addEventListener( "unhandledrejection", function( event ) { - QUnit.onError( event.reason ); + QUnit.onUnhandledRejection( event.reason ); } ); }() ); diff --git a/src/core.js b/src/core.js index b4c76ab71..7dd2c11b9 100644 --- a/src/core.js +++ b/src/core.js @@ -16,6 +16,7 @@ import SuiteReport from "./reports/suite"; import { on, emit } from "./events"; import onError from "./core/onerror"; +import onUnhandledRejection from "./core/on-unhandled-rejection"; let focused = false; const QUnit = {}; @@ -251,7 +252,9 @@ extend( QUnit, { return sourceFromStacktrace( offset ); }, - onError + onError, + + onUnhandledRejection } ); QUnit.pushFailure = pushFailure; diff --git a/src/core/on-unhandled-rejection.js b/src/core/on-unhandled-rejection.js new file mode 100644 index 000000000..2b39d5d39 --- /dev/null +++ b/src/core/on-unhandled-rejection.js @@ -0,0 +1,24 @@ +import { test } from "../test"; + +import config from "./config"; +import { extend } from "./utilities"; + +// Handle an unhandled rejection +export default function onUnhandledRejection( reason ) { + const resultInfo = { + result: false, + message: reason.message || "error", + actual: reason, + source: reason.stack + }; + + const currentTest = config.current; + if ( currentTest ) { + currentTest.assert.pushResult( resultInfo ); + } else { + test( "global failure", extend( function( assert ) { + assert.pushResult( resultInfo ); + }, { validTest: true } ) ); + } +} + From f3e30f43a077f597b8510332a0c8618ac38f7057 Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Mon, 18 Dec 2017 13:38:55 -0500 Subject: [PATCH 4/8] Tests: Update tests for unhandled rejection scenarios. * Properly feature detect the browsers ability to report unhandled rejections * Ensure unhandled rejections are tested for both inside and outside of a test --- test/cli/fixtures/expected/tap-outputs.js | 20 ++++-- test/cli/fixtures/unhandled-rejection.js | 2 + test/reporter-html/unhandled-rejection.js | 78 +++++++++++++++++------ 3 files changed, 78 insertions(+), 22 deletions(-) diff --git a/test/cli/fixtures/expected/tap-outputs.js b/test/cli/fixtures/expected/tap-outputs.js index 88069b6a8..f0ad51c9b 100644 --- a/test/cli/fixtures/expected/tap-outputs.js +++ b/test/cli/fixtures/expected/tap-outputs.js @@ -97,14 +97,26 @@ not ok 1 Unhandled Rejections > test passes just fine, but has a rejected promis --- message: "Error thrown in non-returned promise!" severity: failed - actual: null + actual: {} expected: undefined - stack: undefined:undefined + stack: Error: Error thrown in non-returned promise! + at /Users/rjackson/src/open-source/qunit/test/cli/fixtures/unhandled-rejection.js:13:11 + at ... -1..1 +not ok 2 global failure + --- + message: "outside of a test context" + severity: failed + actual: { + "message": "outside of a test context" +} + expected: undefined + stack: at runTest (/Users/rjackson/src/open-source/qunit/dist/qunit.js:1478:30) + ... +1..2 # pass 0 # skip 0 # todo 0 -# fail 1 +# fail 2 ` }; diff --git a/test/cli/fixtures/unhandled-rejection.js b/test/cli/fixtures/unhandled-rejection.js index 8236e3971..9f3eea81c 100644 --- a/test/cli/fixtures/unhandled-rejection.js +++ b/test/cli/fixtures/unhandled-rejection.js @@ -16,4 +16,6 @@ QUnit.module( "Unhandled Rejections", function() { // prevent test from exiting before unhandled rejection fires setTimeout( done, 10 ); } ); + + Promise.reject( { message: "outside of a test context" } ); } ); diff --git a/test/reporter-html/unhandled-rejection.js b/test/reporter-html/unhandled-rejection.js index 8624c1a98..2a5644f1e 100644 --- a/test/reporter-html/unhandled-rejection.js +++ b/test/reporter-html/unhandled-rejection.js @@ -1,25 +1,20 @@ // Detect if the current browser supports `onunhandledrejection` (avoiding // errors for browsers without the capability) -const HAS_UNHANDLED_REJECTION_HANDLER = "onunhandledrejection" in window; +var HAS_UNHANDLED_REJECTION_HANDLER = "onunhandledrejection" in window; -QUnit.module( "Unhandled Rejections", function( hooks ) { - var originalQUnitOnError; +if ( HAS_UNHANDLED_REJECTION_HANDLER ) { + QUnit.module( "Unhandled Rejections inside test context", function( hooks ) { + hooks.beforeEach( function( assert ) { + var originalPushResult = assert.pushResult; + assert.pushResult = function( resultInfo ) { - hooks.beforeEach( function() { - originalQUnitOnError = QUnit.onError; - } ); - - hooks.afterEach( function() { - QUnit.onError = originalQUnitOnError; - } ); - - if ( HAS_UNHANDLED_REJECTION_HANDLER ) { - QUnit.test( "test passes just fine, but has a rejected promise", function( assert ) { - - QUnit.onError = function() { - assert.ok( true, "QUnit.onError was called" ); + // Inverts the result so we can test failing assertions + resultInfo.result = !resultInfo.result; + originalPushResult( resultInfo ); }; + } ); + QUnit.test( "test passes just fine, but has a rejected promise", function( assert ) { const done = assert.async(); new Promise( function( resolve ) { @@ -32,5 +27,52 @@ QUnit.module( "Unhandled Rejections", function( hooks ) { // prevent test from exiting before unhandled rejection fires setTimeout( done, 10 ); } ); - } -} ); + + } ); + + QUnit.module( "Unhandled Rejections outside test context", function( hooks ) { + var originalPushResult; + + hooks.beforeEach( function( assert ) { + + // Duck-punch pushResult so we can check test name and assert args. + originalPushResult = assert.pushResult; + + assert.pushResult = function( resultInfo ) { + + // Restore pushResult for this assert object, to allow following assertions. + this.pushResult = originalPushResult; + + this.strictEqual( this.test.testName, "global failure", "Test is appropriately named" ); + + this.deepEqual( + resultInfo, + { + message: "Error message", + source: "filePath.js:1", + result: false, + actual: { + message: "Error message", + fileName: "filePath.js", + lineNumber: 1, + stack: "filePath.js:1" + } + }, + "Expected assert.pushResult to be called with correct args" + ); + }; + } ); + + hooks.afterEach( function() { + QUnit.config.current.pushResult = originalPushResult; + } ); + + // Actual test (outside QUnit.test context) + QUnit.onUnhandledRejection( { + message: "Error message", + fileName: "filePath.js", + lineNumber: 1, + stack: "filePath.js:1" + } ); + } ); +} From 9a77ca430b231eee7c59c43458eaed64ff356ec8 Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Mon, 18 Dec 2017 14:23:43 -0500 Subject: [PATCH 5/8] Core: Capture the proper stack trace in onUnhandledRejection. --- src/core/on-unhandled-rejection.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/core/on-unhandled-rejection.js b/src/core/on-unhandled-rejection.js index 2b39d5d39..458ca4f79 100644 --- a/src/core/on-unhandled-rejection.js +++ b/src/core/on-unhandled-rejection.js @@ -2,6 +2,7 @@ import { test } from "../test"; import config from "./config"; import { extend } from "./utilities"; +import { sourceFromStacktrace } from "./stacktrace"; // Handle an unhandled rejection export default function onUnhandledRejection( reason ) { @@ -9,7 +10,7 @@ export default function onUnhandledRejection( reason ) { result: false, message: reason.message || "error", actual: reason, - source: reason.stack + source: reason.stack || sourceFromStacktrace( 3 ) }; const currentTest = config.current; From 6a6fae88f2940f678bffa0fdc065fd5c9de9c9e7 Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Mon, 18 Dec 2017 14:24:18 -0500 Subject: [PATCH 6/8] Tests: Update CLI test and test fixture to remove stacktrace ambiguity. Unfortunately, we cannot use real errors here because the stack trace representation changes across Node versions (and therefore the expectations fail). --- test/cli/fixtures/expected/tap-outputs.js | 16 +++++++++++----- test/cli/fixtures/unhandled-rejection.js | 17 +++++++++++++++-- 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/test/cli/fixtures/expected/tap-outputs.js b/test/cli/fixtures/expected/tap-outputs.js index f0ad51c9b..ff3c5cd7e 100644 --- a/test/cli/fixtures/expected/tap-outputs.js +++ b/test/cli/fixtures/expected/tap-outputs.js @@ -1,3 +1,5 @@ +"use strict"; + // Expected outputs from the TapReporter for the commands run in CLI tests module.exports = { "qunit": @@ -97,21 +99,25 @@ not ok 1 Unhandled Rejections > test passes just fine, but has a rejected promis --- message: "Error thrown in non-returned promise!" severity: failed - actual: {} + actual: { + "message": "Error thrown in non-returned promise!", + "stack": "Error: Error thrown in non-returned promise!\\n at /some/path/wherever/unhandled-rejection.js:13:11" +} expected: undefined stack: Error: Error thrown in non-returned promise! - at /Users/rjackson/src/open-source/qunit/test/cli/fixtures/unhandled-rejection.js:13:11 - at + at /some/path/wherever/unhandled-rejection.js:13:11 ... not ok 2 global failure --- message: "outside of a test context" severity: failed actual: { - "message": "outside of a test context" + "message": "outside of a test context", + "stack": "Error: outside of a test context\\n at Object. (/some/path/wherever/unhandled-rejection.js:20:18)" } expected: undefined - stack: at runTest (/Users/rjackson/src/open-source/qunit/dist/qunit.js:1478:30) + stack: Error: outside of a test context + at Object. (/some/path/wherever/unhandled-rejection.js:20:18) ... 1..2 # pass 0 diff --git a/test/cli/fixtures/unhandled-rejection.js b/test/cli/fixtures/unhandled-rejection.js index 9f3eea81c..82d3b6eed 100644 --- a/test/cli/fixtures/unhandled-rejection.js +++ b/test/cli/fixtures/unhandled-rejection.js @@ -10,12 +10,25 @@ QUnit.module( "Unhandled Rejections", function() { setTimeout( resolve ); } ) .then( function() { - throw new Error( "Error thrown in non-returned promise!" ); + + // throwing a non-Error here because stack trace representation + // across Node versions is not stable (they continue to get better) + throw { + message: "Error thrown in non-returned promise!", + stack: `Error: Error thrown in non-returned promise! + at /some/path/wherever/unhandled-rejection.js:13:11` + }; } ); // prevent test from exiting before unhandled rejection fires setTimeout( done, 10 ); } ); - Promise.reject( { message: "outside of a test context" } ); + // rejecting with a non-Error here because stack trace representation + // across Node versions is not stable (they continue to get better) + Promise.reject( { + message: "outside of a test context", + stack: `Error: outside of a test context + at Object. (/some/path/wherever/unhandled-rejection.js:20:18)` + } ); } ); From 455ac83f2a8924f1193a4f625ac10483d50ef2b8 Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Mon, 18 Dec 2017 15:47:58 -0500 Subject: [PATCH 7/8] Tests: Fixup comment indentation. --- test/reporter-html/unhandled-rejection.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/reporter-html/unhandled-rejection.js b/test/reporter-html/unhandled-rejection.js index 2a5644f1e..55c3c66e4 100644 --- a/test/reporter-html/unhandled-rejection.js +++ b/test/reporter-html/unhandled-rejection.js @@ -8,7 +8,7 @@ if ( HAS_UNHANDLED_REJECTION_HANDLER ) { var originalPushResult = assert.pushResult; assert.pushResult = function( resultInfo ) { - // Inverts the result so we can test failing assertions + // Inverts the result so we can test failing assertions resultInfo.result = !resultInfo.result; originalPushResult( resultInfo ); }; From 4b955d1afcdba2a320f7f8c44b56de934763bf8a Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Tue, 19 Dec 2017 11:36:40 -0500 Subject: [PATCH 8/8] Tests: Address feedback, use more idiomatic promise chaining. --- test/cli/fixtures/unhandled-rejection.js | 19 ++++++++----------- test/reporter-html/unhandled-rejection.js | 9 +++------ 2 files changed, 11 insertions(+), 17 deletions(-) diff --git a/test/cli/fixtures/unhandled-rejection.js b/test/cli/fixtures/unhandled-rejection.js index 82d3b6eed..4846a8925 100644 --- a/test/cli/fixtures/unhandled-rejection.js +++ b/test/cli/fixtures/unhandled-rejection.js @@ -6,19 +6,16 @@ QUnit.module( "Unhandled Rejections", function() { const done = assert.async(); - new Promise( function( resolve ) { - setTimeout( resolve ); - } ) - .then( function() { + Promise.resolve().then( function() { - // throwing a non-Error here because stack trace representation - // across Node versions is not stable (they continue to get better) - throw { - message: "Error thrown in non-returned promise!", - stack: `Error: Error thrown in non-returned promise! + // throwing a non-Error here because stack trace representation + // across Node versions is not stable (they continue to get better) + throw { + message: "Error thrown in non-returned promise!", + stack: `Error: Error thrown in non-returned promise! at /some/path/wherever/unhandled-rejection.js:13:11` - }; - } ); + }; + } ); // prevent test from exiting before unhandled rejection fires setTimeout( done, 10 ); diff --git a/test/reporter-html/unhandled-rejection.js b/test/reporter-html/unhandled-rejection.js index 55c3c66e4..5205e2e8e 100644 --- a/test/reporter-html/unhandled-rejection.js +++ b/test/reporter-html/unhandled-rejection.js @@ -17,12 +17,9 @@ if ( HAS_UNHANDLED_REJECTION_HANDLER ) { QUnit.test( "test passes just fine, but has a rejected promise", function( assert ) { const done = assert.async(); - new Promise( function( resolve ) { - setTimeout( resolve ); - } ) - .then( function() { - throw new Error( "Error thrown in non-returned promise!" ); - } ); + Promise.resolve().then( function() { + throw new Error( "Error thrown in non-returned promise!" ); + } ); // prevent test from exiting before unhandled rejection fires setTimeout( done, 10 );