-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Start fixing bugs discovered by Node.js's Node-API tests #14501
Conversation
Updated 8:03 PM PT - Feb 26th, 2025
❌ @190n, your commit b1f78cd has 2 failures in
🧪 try this PR locally: bunx bun-pr 14501 |
…eError exceptions
…nsure that the key is a name in napi_has_own_property
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Overview
This PR addresses bugs discovered by Node.js's Node-API tests by updating timeouts and adjusting test settings.
- Adds a new napiTimeout constant for Node-API tests.
- Updates the getTestTimeout function to return the napiTimeout for paths matching "napi".
Reviewed Changes
File | Description |
---|---|
scripts/runner.node.mjs | Introduces napiTimeout and updates test timeout logic. |
Copilot reviewed 341 out of 341 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
scripts/runner.node.mjs:51
- [nitpick] Consider adding an inline comment describing that this constant sets the timeout for Node-API tests and clarifying the units (milliseconds) for better readability.
const napiTimeout = 10 * 60_000;
scripts/runner.node.mjs:684
- [nitpick] Consider adding a comment to clarify that the regex match is intended to identify Node-API tests, ensuring that future maintainers understand the context of this branch in the timeout logic.
if (/napi/i.test(testPath)) {
What does this PR do?
TODO before merging:
remove dependency on JSC change formerged JSC change insteadnapi_get_property_names
(@heimskr)Fixes #14336
Fixes #15383
Fixes #15429
Fixes #17503
Tests in
js-native-api
How did you verify your code works?
Running Node's tests