Skip to content

Add a node-tests package #147

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Add a node-tests package #147

wants to merge 12 commits into from

Conversation

kraenhansen
Copy link
Collaborator

I want to start running the js-native-api tests (where we bundle in universal polyfills for the Node.js buildins).

@kraenhansen kraenhansen self-assigned this Jun 29, 2025
Copy link

changeset-bot bot commented Jun 29, 2025

⚠️ No Changeset found

Latest commit: 77d261b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@kraenhansen kraenhansen force-pushed the main branch 4 times, most recently from 90dd405 to 8dac805 Compare July 7, 2025 12:30
@kraenhansen kraenhansen added Apple 🍎 Anything related to the Apple platform (iOS, macOS, Cocoapods, Xcode, XCFrameworks, etc.) Android 🤖 Anything related to the Android platform (Gradle, NDK, Android SDK) CI Continuous integration labels Jul 24, 2025
@kraenhansen kraenhansen marked this pull request as ready for review July 24, 2025 13:07
delimiters: ["", ""],
}
),
// Use the babel plugin to transform require statements for addons before they get resolved
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thinking more about it, we might actually be able to do without this if we simply externalize this and replace the __require on that instead 🤔

Copy link
Collaborator

@shirakaba shirakaba left a comment

Choose a reason for hiding this comment

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

Not familiar enough to comment further, but some drive-by comments!

"gyp-to-cmake": "gyp-to-cmake ./tests",
"build-tests": "tsx scripts/build-tests.mts",
"bundle": "rolldown -c rolldown.config.mts",
"bootstrap": "npm run copy-tests && npm run gyp-to-cmake && npm run build-tests && npm run bundle"
Copy link
Collaborator

Choose a reason for hiding this comment

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

As long as you don't need pre<script> and post<script>, we can use node --run to avoid the overhead of spinning up npm each time. Should save a good number of milliseconds!

Suggested change
"bootstrap": "npm run copy-tests && npm run gyp-to-cmake && npm run build-tests && npm run bundle"
"bootstrap": "node --run copy-tests && node --run gyp-to-cmake && node --run build-tests && node --run bundle"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TIL - that's great, thanks for sharing 👍 I'm going to use that a lot going forward.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yagiz made it in response to Bun showing off about their fast script-running. At the time, he estimated npm run as adding an additional 100-200ms of overhead compared to just running the script in Node.js.

https://x.com/yagiznizipli/status/1700901259463577956

More recently, he's said that node --run is 10 times faster as it takes only around 25 ms to run.

https://x.com/yagiznizipli/status/1779200364153065836

@@ -0,0 +1 @@
export const buildType = "Release";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should it be typed like this so that you get appropriate intellisense, or is it really only ever gonna be Release?

Suggested change
export const buildType = "Release";
export const buildType: "Debug" | "Release" = "Release";

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For now, we're only testing release builds 🙂 I'll likely revisit that later 👍

Comment on lines 11 to 14
execSync("cmake-rn", {
cwd: projectDirectory,
stdio: "inherit",
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make sense to do these as a Promise.all(), or not worth it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Very likely, once we have more it would be great to not serialize on these 👍

Comment on lines +11 to +24
function readGypTargetNames(gypFilePath: string): string[] {
const contents = JSON.parse(fs.readFileSync(gypFilePath, "utf-8")) as unknown;
assert(
typeof contents === "object" && contents !== null,
"Expected gyp file to contain a valid JSON object"
);
assert("targets" in contents, "Expected targets in gyp file");
const { targets } = contents;
assert(Array.isArray(targets), "Expected targets to be an array");
return targets.map(({ target_name }) => {
assert(
typeof target_name === "string",
"Expected target_name to be a string"
);
return target_name;
});
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the lightweight assertions as usual 👍

Comment on lines +54 to +58
describe(suiteName, () => {
for (const [exampleName, requireTest] of Object.entries(examples)) {
it(exampleName, requireTest);
}
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure whether our test runner supports it, but wondering whether we could use describe.each / test.each for better test readouts. I haven't tried before, so no worries if it's not worth it in this case.

Copy link
Contributor

@paradowstack paradowstack left a comment

Choose a reason for hiding this comment

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

My eyes couldn’t spot anything - great progress!

This seemed to be causing issues on Windows
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android 🤖 Anything related to the Android platform (Gradle, NDK, Android SDK) Apple 🍎 Anything related to the Apple platform (iOS, macOS, Cocoapods, Xcode, XCFrameworks, etc.) CI Continuous integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants