Skip to content

Conversation

@legendecas
Copy link
Member

Any JavaScript values can be thrown and they have to be printed in node-report to prevent confusion.

This PR is trying to print primitive values' ToString value in the report as the javascriptStack.message.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Mar 31, 2021
@legendecas legendecas added the report Issues and PRs related to process.report. label Mar 31, 2021
@legendecas legendecas force-pushed the node-report/exception branch from 68d2012 to 8292057 Compare March 31, 2021 18:03
@mhdawson
Copy link
Member

@legendecas is there any related issue or additional context?

@legendecas
Copy link
Member Author

@mhdawson is there any related issue or additional context?

No, I didn't find any issues raised.

Please checkout the tests included in the PR. Basically the issue is that node-report is unable to print those thrown primitives as uncaught exceptions.

@legendecas legendecas force-pushed the node-report/exception branch 2 times, most recently from 86ae6c4 to 545cad3 Compare April 1, 2021 16:55
@nodejs-github-bot

This comment has been minimized.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson
Copy link
Member

mhdawson commented Apr 8, 2021

@gireeshpunathil what do you think?

@nodejs-github-bot

This comment has been minimized.

@legendecas legendecas force-pushed the node-report/exception branch from 545cad3 to 3504831 Compare April 19, 2021 15:42
@legendecas
Copy link
Member Author

Just pushed with rebase and a fix on crash with V8_ENABLE_CHECKS flag.

@nodejs-github-bot

This comment has been minimized.

@legendecas legendecas force-pushed the node-report/exception branch from 3504831 to be59b67 Compare April 19, 2021 16:37
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@legendecas
Copy link
Member Author

Looks like the CI failures are produced on pi2-docker, mostly "warning: failed to remove out/Release/.nfs00000000018426c4000015ac: Device or resource busy". And the history looks like the failure is happening to start from days ago.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

legendecas added a commit that referenced this pull request Apr 26, 2021
PR-URL: #38009
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@legendecas
Copy link
Member Author

Landed in 55745a1

@legendecas legendecas closed this Apr 26, 2021
@legendecas legendecas deleted the node-report/exception branch April 26, 2021 02:44
targos pushed a commit that referenced this pull request Apr 29, 2021
PR-URL: #38009
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@targos targos mentioned this pull request May 3, 2021
targos pushed a commit that referenced this pull request May 30, 2021
PR-URL: #38009
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. report Issues and PRs related to process.report.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants