Conversation
|
90dd405 to
8dac805
Compare
176b5ed to
86b42d4
Compare
86b42d4 to
375259a
Compare
375259a to
8c4626c
Compare
| delimiters: ["", ""], | ||
| } | ||
| ), | ||
| // Use the babel plugin to transform require statements for addons before they get resolved |
There was a problem hiding this comment.
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 🤔
shirakaba
left a comment
There was a problem hiding this comment.
Not familiar enough to comment further, but some drive-by comments!
packages/node-tests/package.json
Outdated
| "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" |
There was a problem hiding this comment.
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!
| "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" |
There was a problem hiding this comment.
TIL - that's great, thanks for sharing 👍 I'm going to use that a lot going forward.
There was a problem hiding this comment.
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.
| @@ -0,0 +1 @@ | |||
| export const buildType = "Release"; | |||
There was a problem hiding this comment.
Should it be typed like this so that you get appropriate intellisense, or is it really only ever gonna be Release?
| export const buildType = "Release"; | |
| export const buildType: "Debug" | "Release" = "Release"; |
There was a problem hiding this comment.
For now, we're only testing release builds 🙂 I'll likely revisit that later 👍
| execSync("cmake-rn", { | ||
| cwd: projectDirectory, | ||
| stdio: "inherit", | ||
| }); |
There was a problem hiding this comment.
Would it make sense to do these as a Promise.all(), or not worth it?
There was a problem hiding this comment.
Very likely, once we have more it would be great to not serialize on these 👍
There was a problem hiding this comment.
I tried, but it seems to provoke a race-condition on Windows 🤔
There was a problem hiding this comment.
Huh, that's unexpected. I don't see any smoking gun at a glance (and I'm pretty sure cmd.exe supports && in commands the same as POSIX shells). The only thing that comes to mind is that, Windows holds some pretty aggressive file locks sometimes.
There was a problem hiding this comment.
I'll revisit once we start adding more of these tests 🤞
| 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; | ||
| }); | ||
| } |
There was a problem hiding this comment.
I like the lightweight assertions as usual 👍
apps/test-app/App.tsx
Outdated
| describe(suiteName, () => { | ||
| for (const [exampleName, requireTest] of Object.entries(examples)) { | ||
| it(exampleName, requireTest); | ||
| } | ||
| }); |
There was a problem hiding this comment.
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.
paradowstack
left a comment
There was a problem hiding this comment.
My eyes couldn’t spot anything - great progress!
This seemed to be causing issues on Windows
1fc3431 to
9bf0671
Compare
9bf0671 to
8d4c8f2
Compare
I want to start running the js-native-api tests (where we bundle in universal polyfills for the Node.js buildins).