★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/18076Nouvelle recherche | Portail wanayoo
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

test: add common.crashOnUnhandledRejection to addons/callback-scope #18076

Closed

Conversation

Copy link
Contributor

@shqld shqld commented Jan 10, 2018

Overview

This is my first commit to nodejs/node, which is from nodejs/code-and-learn#77. Thanks in advance. 🙇

I added common.crashOnUnhandledRejection to test/addons/callback-scope/test-resolve-async.js.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added addons test labels Jan 10, 2018
@hiroppy hiroppy added the code-and-learn label Jan 10, 2018
@hiroppy
Copy link
Member

@hiroppy hiroppy commented Jan 10, 2018

@@ -4,6 +4,8 @@ const common = require('../../common');
const assert = require('assert');
const { testResolveAsync } = require(`./build/${common.buildType}/binding`);

common.crashOnUnhandledRejection();
Copy link
Member

@lpinca lpinca Jan 10, 2018

Choose a reason for hiding this comment

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

I'm fine with this but there is an assertion below in the setTimeout() callback which should make this redundant.

Copy link
Contributor Author

@shqld shqld Jan 12, 2018

Choose a reason for hiding this comment

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

Should I make any changes to merge?

Copy link
Member

@joyeecheung joyeecheung Jan 12, 2018

Choose a reason for hiding this comment

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

@lpinca common.crashOnUnhandledRejection(); would display the rejection in the trace instead of a timeout though.

Copy link
Member

@lpinca lpinca Jan 12, 2018

Choose a reason for hiding this comment

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

Yes that's better. :shipit:

@joyeecheung joyeecheung added the author ready label Jan 12, 2018
@TimothyGu
Copy link
Member

@TimothyGu TimothyGu commented Jan 13, 2018

Landed in c84582c. 🎉!

@TimothyGu TimothyGu closed this Jan 13, 2018
@TimothyGu TimothyGu removed the author ready label Jan 13, 2018
TimothyGu pushed a commit that referenced this issue Jan 13, 2018
PR-URL: #18076
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
evanlucas pushed a commit that referenced this issue Jan 30, 2018
PR-URL: #18076
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
MylesBorins pushed a commit that referenced this issue Feb 27, 2018
PR-URL: #18076
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addons code-and-learn test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants