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

src: fix winapi_strerror error string #55207

Conversation

huseyinacacak-janea
Copy link
Contributor

This fixes the bug in printing the error messages when an exception is thrown on Windows.

Fixes: #23191

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Oct 1, 2024
@@ -0,0 +1,43 @@
// Copyright Joyent, Inc. and other Node contributors.
Copy link
Member

Choose a reason for hiding this comment

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

We don't add the copyright header in new tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Fixed it.

@huseyinacacak-janea huseyinacacak-janea force-pushed the huseyin-6791-truncated-error-messages branch from 95c8b00 to 565a983 Compare October 1, 2024 07:33
Copy link

codecov bot commented Oct 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.41%. Comparing base (d2a4f92) to head (b1f1213).
Report is 294 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #55207      +/-   ##
==========================================
+ Coverage   88.07%   88.41%   +0.34%     
==========================================
  Files         652      652              
  Lines      183546   186838    +3292     
  Branches    35859    36060     +201     
==========================================
+ Hits       161649   165198    +3549     
+ Misses      15147    14891     -256     
+ Partials     6750     6749       -1     
Files with missing lines Coverage Δ
src/api/exceptions.cc 94.18% <ø> (ø)

... and 232 files with indirect coverage changes

Comment on lines 20 to 22
if (common.isWindows) {
testDebugPrint().then(common.mustCall());
}
Copy link
Member

Choose a reason for hiding this comment

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

Using common.skip is preferred

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. Fixed it.

@huseyinacacak-janea huseyinacacak-janea force-pushed the huseyin-6791-truncated-error-messages branch from 565a983 to d8c6d70 Compare October 2, 2024 06:58
@StefanStojanovic StefanStojanovic added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 4, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 4, 2024
@nodejs-github-bot
Copy link
Collaborator

@lpinca lpinca added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Oct 11, 2024
@lpinca
Copy link
Member

lpinca commented Oct 11, 2024

Can you use a more descriptive name for the test file? We renamed many test-xxx-gh-xxx.js in the past.


const cp = child.spawn('pwd');

cp.on('exit', common.mustCall(function(code) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cp.on('exit', common.mustCall(function(code) {
cp.on('exit', common.mustCall(function() {

Or add an assertion for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. Fixed it.

@huseyinacacak-janea huseyinacacak-janea force-pushed the huseyin-6791-truncated-error-messages branch from d8c6d70 to b1f1213 Compare October 14, 2024 07:57
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 14, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 14, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 15, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 15, 2024
@nodejs-github-bot nodejs-github-bot merged commit 51d8146 into nodejs:main Oct 15, 2024
59 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 51d8146

aduh95 pushed a commit that referenced this pull request Oct 19, 2024
Fixes: #23191
PR-URL: #55207
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@aduh95 aduh95 mentioned this pull request Oct 24, 2024
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
Fixes: nodejs#23191
PR-URL: nodejs#55207
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
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++. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Truncated error messages produced by WinapiErrnoException
6 participants