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

Closed
wants to merge 1 commit into from

Conversation

babygoat
Copy link
Contributor

@babygoat babygoat commented Nov 23, 2017

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!

const rejectPromise = Promise.reject(-1);
assert.strictEqual(test_promise.isPromise(rejectPromise), true);
//Dummy rejection handler
rejectPromise.catch(() => {});
Copy link
Contributor

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

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

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

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Dec 12, 2017
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
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
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 12, 2018
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
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
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
MylesBorins pushed a commit that referenced this pull request May 1, 2018
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants