FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: add unhandled rejection guard by babygoat · Pull Request #17275 · nodejs/node · GitHub

/ node Public

test: add unhandled rejection guard - #17275

Closed
babygoat wants to merge 1 commit into
nodejs:masterfrom
babygoat:my_first_node_pr
ConversationCommits (1)ChecksFiles changed
Closed

test: add unhandled rejection guard#17275
babygoat wants to merge 1 commit into
nodejs:masterfrom
babygoat:my_first_node_pr

Conversation

babygoat commented Nov 23, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 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 commented Nov 23, 2017

Copy link
Copy Markdown
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 reviewed Nov 23, 2017
View reviewed changes
Comment thread test/addons-napi/test_promise/test.js Outdated

MylesBorins Nov 23, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

babygoat Nov 24, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

addaleax Nov 26, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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 :)

babygoat Nov 27, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 approved these changes Nov 26, 2017
View reviewed changes

addaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM with or without Myles' suggestion

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 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 10, 2017

addaleax commented Dec 10, 2017

Copy link
Copy Markdown
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
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 commented Dec 12, 2017

Copy link
Copy Markdown
Member

Landed in ef49f55

BridgeAR closed this Dec 12, 2017

BridgeAR commented Dec 12, 2017

Copy link
Copy Markdown
Member

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

addaleax removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 13, 2017

babygoat commented Dec 14, 2017

Copy link
Copy Markdown
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
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 mentioned this pull request Jan 10, 2018
Merged
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 12, 2018
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
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
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 mentioned this pull request Apr 16, 2018
Merged
MylesBorins pushed a commit that referenced this pull request May 1, 2018
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 mentioned this pull request May 2, 2018
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

addaleax addaleax approved these changes
BridgeAR BridgeAR approved these changes
+1 more reviewer
MylesBorins MylesBorins left review comments
Reviewers whose approvals may not affect merge requirements

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.

5 participants

Back | FazBrowse Home | New Git URL