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’ll 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

Conversation

rwjblue
Copy link
Contributor

@rwjblue rwjblue commented Dec 18, 2017

Prior to this change, an unhandled exception in a promise that was not returned from the test callback would not cause test failure.

For example, the following (using default out of the box settings) results in test failures:

test('native promises cause an unhandled rejection', function(assert) {
  // imagine this was "deep" inside various method calls
  setTimeout(function() {
    throw new Error('whoops!');
  });

  // other things that we are trying to test
  assert.deepEqual(someMethod(), ['one', 'two']);
});

And this (prior to this PR) would result in a passing test suite:

test('native promises cause an unhandled rejection', function(assert) {
  // imagine this was "deep" inside various method calls
  Promise.resolve().then(function() {
    throw new Error('whoops!');
  });

  // other things that we are trying to test
  assert.deepEqual(someMethod(), ['one', 'two']);
});

The reason that what we are doing already isn't good enough is:

  • HTML Reporter: the default unhandledRejection handler does not dispatch to window.onerror at all... 😩
  • Node CLI: unhandled rejections in Node 6.6 and higher emit a console warning, but nothing else

@@ -0,0 +1,35 @@
// Currently (2017-12-18) only Chrome has implemented `unhandledRejection`
const IS_CHROME = !!window.chrome;
Copy link
Member

@Krinkle Krinkle Dec 18, 2017

Choose a reason for hiding this comment

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

'onrejectionhandled' in window might be easier to maintain as a feature test, or are there false positives for that? That way when other browsers land support, our CI will verify them automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome, thank you for that. Hard coding to Chrome definitely felt bad, but it was late and I ran out of ideas. 😝

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also just confirmed that Safari also supports the unhandled rejection events, thanks again for the pointer on the feature detect!

Brings unhandled rejections in line with unhandled exceptions.
@rwjblue rwjblue force-pushed the ensure-unhandled-rejections-fail branch from 6249803 to bc23a65 Compare December 18, 2017 17:43
@rwjblue
Copy link
Contributor Author

rwjblue commented Dec 18, 2017

I took some time to experiment with the implementation (by polyfilling in emberjs/ember-qunit#306), and it seems that just using QUnit.onError was quite the right implementation. QUnit.onError is closely tied to window.onerror semantics (returning true or false for bubbling, the specific argument signature for stack trace, etc). Instead of making it much more complicated I created a QUnit.onRejectionHandler method to specifically handle this case.

I also updated the tests to ensure that we are testing both inside and outside of a single test context, and modified to use a more appropriate feature detection (instead of only testing on Chrome). The new tests pass properly on both Chrome and Safari (which are the only two browsers which have implemented onunhandledrejection to my knowledge).

@rwjblue rwjblue force-pushed the ensure-unhandled-rejections-fail branch from 6d92642 to 93007ce Compare December 18, 2017 19:27
* Properly feature detect the browsers ability to report unhandled
  rejections
* Ensure unhandled rejections are tested for both inside and outside of
  a test
@rwjblue rwjblue force-pushed the ensure-unhandled-rejections-fail branch from 93007ce to bc32c4b Compare December 18, 2017 19:28
Unfortunately, we cannot use real errors here because the stack trace
representation changes across Node versions (and therefore the
expectations fail).
@rwjblue rwjblue force-pushed the ensure-unhandled-rejections-fail branch from bc32c4b to 6a6fae8 Compare December 18, 2017 19:34
@rwjblue
Copy link
Contributor Author

rwjblue commented Dec 18, 2017

Note: I'm happy to squash the commits down when we are ready to go on this one. It is easier to push new commits (e.g. maintainers have an easier time of reviewing the subset of the diff that is new).

Copy link
Contributor

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

Just left a small comment. I promise to give this a full review later 😄

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

@platinumazure platinumazure Dec 18, 2017

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)

@platinumazure
Copy link
Contributor

For what it's worth, I've opened eslint/eslint#9733 about the comment indent issue.

Copy link
Member

@trentmwillis trentmwillis left a comment

Choose a reason for hiding this comment

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

Overall, looks good to me. Just two minor comments.

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 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...

Copy link
Contributor

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@rwjblue rwjblue force-pushed the ensure-unhandled-rejections-fail branch from 97eb1c3 to 4b955d1 Compare December 19, 2017 17:02
@trentmwillis
Copy link
Member

Looks good to me. @Krinkle any other feedback before I merge?

@trentmwillis trentmwillis merged commit 81e0f69 into qunitjs:master Dec 20, 2017
@trentmwillis
Copy link
Member

Thanks @rwjblue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants