Skip to content
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.

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

Already on GitHub? Sign in to your account

Ensure unhandled rejections cause test failure. #1241

Merged
merged 8 commits into from Dec 20, 2017
5 changes: 5 additions & 0 deletions bin/run.js
Expand Up @@ -33,6 +33,11 @@ function run( args, options ) {
}
} );

// Listen for unhandled rejections, and call QUnit.onUnhandledRejection
process.on( "unhandledRejection", function( reason ) {
QUnit.onUnhandledRejection( reason );
} );

const files = utils.getFilesFromArgs( args );

QUnit = requireQUnit();
Expand Down
4 changes: 4 additions & 0 deletions reporter/html.js
Expand Up @@ -980,4 +980,8 @@ export function escapeText( s ) {
return ret;
};

// Listen for unhandled rejections, and call QUnit.onUnhandledRejection
window.addEventListener( "unhandledrejection", function( event ) {
QUnit.onUnhandledRejection( event.reason );
} );
}() );
5 changes: 4 additions & 1 deletion src/core.js
Expand Up @@ -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 = {};
Expand Down Expand Up @@ -251,7 +252,9 @@ extend( QUnit, {
return sourceFromStacktrace( offset );
},

onError
onError,

onUnhandledRejection
} );

QUnit.pushFailure = pushFailure;
Expand Down
25 changes: 25 additions & 0 deletions src/core/on-unhandled-rejection.js
@@ -0,0 +1,25 @@
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 ) {
Copy link
Member

Choose a reason for hiding this comment

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

Is reason just an Error instance?

Copy link
Contributor Author

@rwjblue rwjblue Dec 19, 2017

Choose a reason for hiding this comment

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

No, reason is whatever the rejection value is. An Error instance is almost certainly the most common thing that reason would be, but it could also be a failed XHR, a string, undefined, null, etc. The value here is either whatever the argument to reject() or the argument tothrow is.

See small demo JSBin with the following content:

window.addEventListener('unhandledrejection', function(event) {
  console.log('reason:', event.reason);
})

Promise.reject();
Promise.reject(null);
Promise.reject("asdf");
Promise.resolve().then(() => {
  throw new Error('boom');
})

Copy link
Member

Choose a reason for hiding this comment

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

Ah okay, makes sense. Early-morning me couldn't quite make that connection.

const resultInfo = {
result: false,
message: reason.message || "error",
actual: reason,
source: reason.stack || sourceFromStacktrace( 3 )
};

const currentTest = config.current;
if ( currentTest ) {
currentTest.assert.pushResult( resultInfo );
} else {
test( "global failure", extend( function( assert ) {
assert.pushResult( resultInfo );
}, { validTest: true } ) );
}
}

36 changes: 35 additions & 1 deletion 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":
Expand Down Expand Up @@ -89,6 +91,38 @@ 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: {
"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 /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",
"stack": "Error: outside of a test context\\n at Object.<anonymous> (/some/path/wherever/unhandled-rejection.js:20:18)"
}
expected: undefined
stack: Error: outside of a test context
at Object.<anonymous> (/some/path/wherever/unhandled-rejection.js:20:18)
...
1..2
# pass 0
# skip 0
# todo 0
# fail 2
`
};
34 changes: 34 additions & 0 deletions test/cli/fixtures/unhandled-rejection.js
@@ -0,0 +1,34 @@
"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 ) {
Copy link
Member

Choose a reason for hiding this comment

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

Correct me if I'm wrong, but can't this just be Promise.resolve().then( ... )? If so, can you change it here and below as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, you are absolutely correct. I have been working too much with Ember's internal Ember.RSVP.Promise which is configured to resolve in an "odd" way, sorry about that!

Updated to be much more idiomatic...

setTimeout( 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!
at /some/path/wherever/unhandled-rejection.js:13:11`
};
} );

// prevent test from exiting before unhandled rejection fires
setTimeout( done, 10 );
} );

// 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.<anonymous> (/some/path/wherever/unhandled-rejection.js:20:18)`
} );
} );
16 changes: 16 additions & 0 deletions test/cli/main.js
Expand Up @@ -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'";
Expand Down
1 change: 1 addition & 0 deletions test/index.html
Expand Up @@ -19,6 +19,7 @@
<script src="setTimeout.js"></script>
<script src="reporter-html/reporter-html.js"></script>
<script src="reporter-html/diff.js"></script>
<script src="reporter-html/unhandled-rejection.js"></script>
<script src="onerror/inside-test.js"></script>
<script src="onerror/outside-test.js"></script>
</head>
Expand Down
78 changes: 78 additions & 0 deletions test/reporter-html/unhandled-rejection.js
@@ -0,0 +1,78 @@
// Detect if the current browser supports `onunhandledrejection` (avoiding
// errors for browsers without the capability)
var HAS_UNHANDLED_REJECTION_HANDLER = "onunhandledrejection" in window;

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 ) {

// Inverts the result so we can test failing assertions
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious to know why this comment is de-indented from the line below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a mistake, though I'm surprised that the otherwise aggressive indentation linting rules didn't catch it...

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries. The indent rule is slightly lenient about comments because in general it is hard to tell what lines of code a comment might be associated with, so ESLint generally allows comments to align with the next line of code, the previous line of code, or with the "correct" indentation (i.e., the indentation we would expect a token to actually be at if this were a token instead of a comment).

My guess is the indent rule is allowing this location due to aligning with line 9. That seems weird since line 10 is whitespace-only, so I wonder if we could improve the rule to not use the previous-line heuristic if the previous token is actually 2 lines above the comment instead of 1 (and similarly, not use the next-line heuristic if the next token is actually 2 lines below the comment instead of 1).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally makes sense! (also cleaned up the indentation)

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 ) {
setTimeout( resolve );
} )
.then( function() {
throw new Error( "Error thrown in non-returned promise!" );
} );

// 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"
} );
} );
}