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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

shqld
Copy link

@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 Issues and PRs related to native addons. test Issues and PRs related to the tests. labels Jan 10, 2018
@hiroppy hiroppy added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Jan 10, 2018
@hiroppy
Copy link
Member

hiroppy commented Jan 10, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request-lite/79/

lpinca
lpinca approved these changes Jan 10, 2018
View reviewed changes
test/addons/callback-scope/test-resolve-async.js
@@ -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 •
edited
Loading

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
Author

@shqld shqld Jan 12, 2018 •
edited
Loading

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

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

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 PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 12, 2018
@TimothyGu
Copy link
Member

Landed in c84582c. 🎉!

@TimothyGu TimothyGu closed this Jan 13, 2018
@TimothyGu TimothyGu removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 13, 2018
TimothyGu pushed a commit that referenced this pull request Jan 13, 2018
@TimothyGu
test: add common.crashOnUnhandledRejection to addons/callback-scope
c84582c 
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 pull request Jan 30, 2018
@evanlucas
test: add common.crashOnUnhandledRejection to addons/callback-scope
2aeb025 
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 evanlucas mentioned this pull request Jan 30, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Feb 27, 2018
@MylesBorins
test: add common.crashOnUnhandledRejection to addons/callback-scope
80973ec 
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
Reviewers

@lpinca lpinca lpinca approved these changes

@joyeecheung joyeecheung joyeecheung left review comments

@addaleax addaleax addaleax approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@cjihrig cjihrig cjihrig approved these changes

@hiroppy hiroppy hiroppy approved these changes

Assignees
No one assigned
Labels
addons Issues and PRs related to native addons. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@shqld @hiroppy @TimothyGu @addaleax @lpinca @cjihrig @joyeecheung @MylesBorins @nodejs-github-bot