Skip to content

src: ERR_ILLEGAL_CONSTRUCTOR thrown from c++ had the wrong constructor#58364

Open
nektro wants to merge 1 commit intonodejs:mainfrom
nektro:nektro-patch-53012
Open

src: ERR_ILLEGAL_CONSTRUCTOR thrown from c++ had the wrong constructor#58364
nektro wants to merge 1 commit intonodejs:mainfrom
nektro:nektro-patch-53012

Conversation

@nektro
Copy link
Copy Markdown
Contributor

@nektro nektro commented May 16, 2025

this brings it into sync with the definition of this error in lib/internal/errors.js.

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/sqlite

@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 May 16, 2025
@codecov
Copy link
Copy Markdown

codecov Bot commented May 16, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.19%. Comparing base (a436f7b) to head (ad03a4a).
⚠️ Report is 2288 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #58364      +/-   ##
==========================================
- Coverage   90.24%   90.19%   -0.05%     
==========================================
  Files         633      635       +2     
  Lines      186859   187171     +312     
  Branches    36685    36750      +65     
==========================================
+ Hits       168622   168811     +189     
- Misses      11036    11124      +88     
- Partials     7201     7236      +35     
Files with missing lines Coverage Δ
src/node_errors.h 87.50% <ø> (ø)

... and 52 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

I'm fine with this but keep in mind that changing the error type is semver-major

@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 17, 2025
@aduh95
Copy link
Copy Markdown
Contributor

aduh95 commented May 19, 2025

The commit message should start with a verb in infinitive form, e.g. src: make `ERR_ILLEGAL_CONSTRUCTOR` inherit `TypeError` .

@nektro nektro force-pushed the nektro-patch-53012 branch from 89c79b5 to ad03a4a Compare May 21, 2025 23:36
@nektro
Copy link
Copy Markdown
Contributor Author

nektro commented Sep 6, 2025

bump

Comment on lines +26 to +27
assert(cp.stderr.includes(`TypeError: Illegal constructor\n`));
assert(cp.stderr.includes(`code: 'ERR_ILLEGAL_CONSTRUCTOR'\n`));
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.

Can you use assert.match so in case of failure we get a more informative error message?

Suggested change
assert(cp.stderr.includes(`TypeError: Illegal constructor\n`));
assert(cp.stderr.includes(`code: 'ERR_ILLEGAL_CONSTRUCTOR'\n`));
assert.match(cp.stderr, /TypeError: Illegal constructor\n(.*\n)*\s*code: 'ERR_ILLEGAL_CONSTRUCTOR'\n/);

name: 'TypeError',
code: 'ERR_ILLEGAL_CONSTRUCTOR',
message: /Illegal constructor/,
message: 'Illegal constructor',
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.

We don't need to test the message

Suggested change
message: 'Illegal constructor',

@avivkeller
Copy link
Copy Markdown
Member

@nektro Please rebase :-)

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. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants