Skip to content

test: show actual assertion error message with value - #20956

Closed
thatshailesh wants to merge 3 commits into
nodejs:masterfrom
thatshailesh:master
Closed

test: show actual assertion error message with value#20956
thatshailesh wants to merge 3 commits into
nodejs:masterfrom
thatshailesh:master

Conversation

@thatshailesh

@thatshaileshthatshailesh commented May 25, 2018

Copy link
Copy Markdown
Contributor

on process exit if some assertion error occurs value of process._exiting
was hidden, this fix will show the actual error message with value

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

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label May 25, 2018

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.

Nit: I think the comment is not needed.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Sure, Actually this is my first PR, and not sure why windows test failed

I am a mac user, is there anything I can do to fix it?

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.

The failure is unrelated, don't worry about that.

@lpinca

Copy link
Copy Markdown
Member

@thatshailesh
thatshailesh requested a review from a team as a code ownerMay 25, 2018 07:45
@BridgeAR

Copy link
Copy Markdown
Member

@thatshailesh would you be so kind and rebase? Seems like you accidentally added the latest commit and that has to be backed out again.

@thatshailesh

Copy link
Copy Markdown
ContributorAuthor

@BridgeAR Sorry, I just did rebase, now only my commits are showing

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.

How about this?:

process.nextTick(common.mustNotCall('process is exiting, should not be called'));

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Done

@Trott

Copy link
Copy Markdown
Member

Looks like you may have accidentally included a package-lock.json update too? You can remove that change, I would think, or whoever is landing this can remove it. (The removed change will probably be the same change proposed in #20970.)

on process exit if some assertion error occurs value of `process._exiting`
was hidden, this fix will show the actual error message with value
@thatshailesh

Copy link
Copy Markdown
ContributorAuthor

@Trott ok I have removed it, thanks 👍

@BridgeAR

Copy link
Copy Markdown
Member

@BridgeARBridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 28, 2018
@apapirovski

apapirovski commented Jun 1, 2018

Copy link
Copy Markdown
Contributor

Landed in 8055bdb

Congrats on your first contribution @thatshailesh!

apapirovski pushed a commit that referenced this pull request Jun 1, 2018
On process exit if some assertion error occurs value of
`process._exiting` was hidden, this fix will show the actual
error message with value.
PR-URL: #20956
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@thatshailesh

Copy link
Copy Markdown
ContributorAuthor

@apapirovski@Trott Thanks for the help 👍

addaleax pushed a commit that referenced this pull request Jun 1, 2018
On process exit if some assertion error occurs value of
`process._exiting` was hidden, this fix will show the actual
error message with value.
PR-URL: #20956
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request Jun 6, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@thatshailesh@lpinca@BridgeAR@Trott@apapirovski@starkwang@trivikr@nodejs-github-bot