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

Add test coverage for typed and range err #1280

Merged
merged 7 commits into from
Mar 17, 2023

Conversation

JckXia
Copy link
Member

@JckXia JckXia commented Feb 1, 2023

PR aims to close #981, #984 and #1271.

Adds test for TypeError and RangeError, as well as some validation logic for the napi_value object we pass to the base TypeError and RangeError ctor. The question is whether we want to crash the application when the napi_value passed isn't a js TypeError, or if we want to emit some type of warning instead.

@JckXia JckXia marked this pull request as draft February 1, 2023 22:48
@legendecas
Copy link
Member

legendecas commented Feb 3, 2023

I'd suggest avoiding type checks in type wrapper constructors for the following reasons:

  1. These are error classes and we should avoid throwing in their constructors as they are likely to be used in error-handling paths already.
  2. Calling into JavaScript can throw arbitrary errors too.
  3. We don't do type checks in other node-addon-api type wrapper constructors, and type check is not performance free.

Instead, I'd suggest adopting the approach introduced in #1281, allowing people to opt-in for the type check.

@JckXia
Copy link
Member Author

JckXia commented Feb 3, 2023

@legendecas Thank you for your explanation! Will update my changes.

@JckXia JckXia force-pushed the add-test-covg-for-typed-and-range-err branch from f4a835b to 2968cf0 Compare February 7, 2023 02:37
@JckXia JckXia changed the title [Draft] Add test covg for typed and range err Add test coverage for typed and range err Feb 7, 2023
@JckXia JckXia marked this pull request as ready for review February 7, 2023 02:37
@legendecas legendecas added the test label Feb 7, 2023
test/error.cc Outdated Show resolved Hide resolved
@JckXia JckXia merged commit e484327 into nodejs:main Mar 17, 2023
@JckXia JckXia deleted the add-test-covg-for-typed-and-range-err branch March 17, 2023 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Tests] Add test coverage document for typed error
2 participants