test(node): restore test-node as a real Node-runtime suite#173
Conversation
Re-establishes Node.js cross-runtime coverage that broke when the package was renamed to test-js (Oct 2025), leaving a stray Bun-only file run by nothing. - give test-node a package.json again (node --test, native TS strip) - rewrite index.test.ts to load the built dist bundles under Node's real CJS (require) and ESM (import) loaders, for both the main and currencies entry points — never the TS source - add a dedicated Node 24 CI job to build-test.yml (run via `node` directly, since bunfig `[run] bun=true` would otherwise re-route to Bun)
📝 WalkthroughWalkthroughA new Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Restores packages/test-node as an actual Node.js runtime test suite to validate that the built dist/ bundles execute correctly under Node’s CJS (require) and ESM (import) loaders, and wires it into CI.
Changes:
- Reintroduces
packages/test-node/package.jsonwith a Node-native test script. - Rewrites
packages/test-node/index.test.tsto validate loading builtdist/artifacts under Node (CJS + ESM), including the currencies subpath. - Adds a dedicated Node 24 CI job and expands workflow path filters to trigger it.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/test-node/package.json | Re-adds a minimal Node test package manifest and node --test script. |
| packages/test-node/index.test.ts | Implements Node-based tests that load built dist/ bundles via CJS/ESM. |
| bun.lock | Registers test-node as a Bun workspace package. |
| .github/workflows/build-test.yml | Adds a Node 24 job to build dist/ and run the Node runtime tests; updates path filters. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build-test.yml:
- Line 53: The GitHub Actions setup steps are using unpinned version tags which
creates a supply-chain security risk. Replace the version tag references for
both the oven-sh/setup-bun action (currently `@v2`) and the actions/setup-node
action (currently `@v6`) with pinned commit SHAs instead, following GitHub's
security best practice of pinning external actions to specific commits rather
than mutable version tags. Apply this change consistently across all occurrences
of these actions in the workflow file.
- Line 50: The `actions/checkout@v6` action reference needs to be pinned to a
specific commit SHA instead of using a version tag, and the
`persist-credentials` option must be explicitly set to `false` to mitigate
supply-chain attack risks and prevent credential exposure. Update the checkout
action invocation to include the commit SHA in the reference (e.g.,
`actions/checkout@<SHA>`) and add `persist-credentials: false` to the action
configuration. Apply this same security fix to all instances of
`actions/checkout` throughout the workflow file for consistency.
In `@packages/test-node/package.json`:
- Line 6: Add a trailing comma after the test script value in the package.json
file. The line with "test": "node --test ./*.test.ts" needs a comma appended to
the closing quote to comply with the ES5 trailing comma coding guideline used
across the project.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fcee138f-4e06-4bf4-b4d2-566ad8c98b71
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
.github/workflows/build-test.ymlpackages/test-node/index.test.tspackages/test-node/package.json
What
Restores
packages/test-nodeas a genuine Node.js test of the published bundles, replacing the stray Bun-only file that nothing ran.test-nodeapackage.jsonagain (node --test, Node's native TS type-stripping — no loader needed)index.test.tsto load the built dist under Node's real CJS (require) and ESM (import) loaders, for both entry points (.and./currencies) — never the.tssourcebuild-test.ymlWhy
test-nodewas a real Node-runtime package until the Oct-2025 rename totest-jsmoved itspackage.jsonaway, leaving a leftoverindex.test.tsthat importedbun:test+.tssource and was wired into no script or workflow. Bun-only CI doesn't validate that the shipped CJS/ESM bundles actually execute under Node — the runtime most consumers use. This restores that guarantee (and would catch packaging/interop regressions Bun papers over).Notes
node --testdirectly, not viabun run—bunfig.toml[run] bun = truere-routesnodeto Bun, which would defeat the purpose.packages/test-node/**to the workflow path filter so changes trigger CI.