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 unhandled rejection guard #17275

Closed
wants to merge 1 commit into from
Closed

test: add unhandled rejection guard #17275

wants to merge 1 commit into from

Conversation

babygoat
Copy link
Contributor

@babygoat babygoat commented Nov 23, 2017 •
edited

Add an unhandled rejection function in
addons-napi/test_promise/test.js. Also, add a
rejection handler to catch the unhandled rejection
after introducing the guard and test the reason
code.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests. labels Nov 23, 2017
@babygoat
Copy link
Contributor Author

Hi @MylesBorins,
Sorry for not finishing the workshop task until now and thanks again for the great sharing yesterday. During the work, I found out there was an small issue in previous test file and I dealt with it as well in the change. I was wondering if I should also fire up an open issue and attached the issue ID in the commit message or just leave as it. Thanks for your consideration!

MylesBorins
MylesBorins reviewed Nov 23, 2017
View reviewed changes
test/addons-napi/test_promise/test.js Outdated
const rejectPromise = Promise.reject(-1);
assert.strictEqual(test_promise.isPromise(rejectPromise), true);
//Dummy rejection handler
rejectPromise.catch(() => {});
Copy link
Member

Choose a reason for hiding this comment

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

would it maybe make sense to test the error object in the catch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! I could make the change. Would it be a good start by testing the equality between the reason code input of static rejection and the error code inside the catch?

Copy link
Member

Choose a reason for hiding this comment

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

@babygoat I think there's no rejection error code here - the rejection value itself is -1, I think, so it's a number and can't have properties of its own. We can test that the value is -1 here, though :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @addaleax, thanks for your explanation! Yeah, you are right the rejection value is not an error object itself but a pure number (-1). But your suggestion is exactly the way I'm currently planning to do! Maybe I should provide the pseudo code for the implementation instead of only vague proposal. I rebase and upload the new commit for the further discussion

addaleax
addaleax approved these changes Nov 26, 2017
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM with or without Myles' suggestion

@babygoat
test: add unhandled rejection guard
9711075 
Add an unhandled rejection function in
addons-napi/test_promise/test.js. Also, add a
rejection handler to catch the unhandled rejection
after introducing the guard and test the reason
code.
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 10, 2017
@addaleax
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-commit/14722/

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Dec 12, 2017
@babygoat @BridgeAR
test: add unhandled rejection guard
ef49f55 
Add an unhandled rejection function in
addons-napi/test_promise/test.js. Also, add a
rejection handler to catch the unhandled rejection
after introducing the guard and test the reason
code.

PR-URL: nodejs#17275
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR
Copy link
Member

Landed in ef49f55

@BridgeAR BridgeAR closed this Dec 12, 2017
@BridgeAR
Copy link
Member

@babygoat thanks a lot for your contribution and congratulations on your first commit to Node.js! 🎉

@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 13, 2017
@babygoat
Copy link
Contributor Author

Thanks for your help, @BridgeAR ! Glad that I could help out and make it.

MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
@babygoat @MylesBorins
test: add unhandled rejection guard
5cd08d3 
Add an unhandled rejection function in
addons-napi/test_promise/test.js. Also, add a
rejection handler to catch the unhandled rejection
after introducing the guard and test the reason
code.

PR-URL: #17275
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
Merged
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 12, 2018
@babygoat
test: add unhandled rejection guard
e304c42 
Add an unhandled rejection function in
addons-napi/test_promise/test.js. Also, add a
rejection handler to catch the unhandled rejection
after introducing the guard and test the reason
code.

PR-URL: nodejs#17275
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 16, 2018
@babygoat
test: add unhandled rejection guard
a005c30 
Add an unhandled rejection function in
addons-napi/test_promise/test.js. Also, add a
rejection handler to catch the unhandled rejection
after introducing the guard and test the reason
code.

PR-URL: nodejs#17275
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@babygoat @MylesBorins
test: add unhandled rejection guard
86ddd03 
Add an unhandled rejection function in
addons-napi/test_promise/test.js. Also, add a
rejection handler to catch the unhandled rejection
after introducing the guard and test the reason
code.

Backport-PR-URL: #19447
PR-URL: #17275
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
Merged
MylesBorins pushed a commit that referenced this pull request May 1, 2018
@babygoat @MylesBorins
test: add unhandled rejection guard
077e187 
Add an unhandled rejection function in
addons-napi/test_promise/test.js. Also, add a
rejection handler to catch the unhandled rejection
after introducing the guard and test the reason
code.

Backport-PR-URL: #19265
PR-URL: #17275
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MylesBorins MylesBorins MylesBorins left review comments

@addaleax addaleax addaleax approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

Assignees
No one assigned
Labels
node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@babygoat @addaleax @BridgeAR @MylesBorins @nodejs-github-bot