test: derive process.versions expectations from scripts/build/deps - #29295
test: derive process.versions expectations from scripts/build/deps#29295Jarred-Sumner wants to merge 8 commits into
Conversation
The process.versions test hardcoded the expected commit for each vendored dependency, so every dep bump (including the automated update-*.yml workflows) required a matching edit here. Read the pinned commit out of scripts/build/deps/<name>.ts at test time instead. This keeps a single source of truth and still verifies the build correctly propagated each commit through depVersionsHeader.ts -> bun_dependency_versions.h -> process.versions.
|
Updated 11:18 PM PT - Aug 8th, 2026
❌ @robobun, your commit 45977f9 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 29295That installs a local version of the PR into your bun-29295 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe ChangesProcess versions validation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — good move to a single source of truth for dep commit hashes.
Extended reasoning...
Overview
This PR modifies a single test file, rewriting the process.versions test to dynamically read pinned commit hashes from scripts/build/deps/*.ts at test time rather than hardcoding them. The change eliminates a recurring maintenance burden where every dep bump required a parallel edit to this test.
Security Risks
None. This is a test-only change with no production code affected.
Level of Scrutiny
Low. The change is self-contained, clearly motivated, and mechanically straightforward. The regex used (/^const [A-Z_]+_COMMIT = "([0-9a-f]{40})";$/m) mirrors the same pattern already used by the update workflows, so the extraction logic is well-precedented. The ares → cares filename mapping is the only non-obvious detail and it is correct.
Other Factors
The only bug found is a nit: there are now two it("process.versions", ...) blocks in the file — the new async one at line 271 and a pre-existing synchronous one at line 1186 that checks hardcoded node/v8/napi/modules strings. The inline comment captures this. It is cosmetic and does not affect correctness. The overall improvement in maintainability clearly outweighs the minor naming issue.
| it("process.versions", async () => { | ||
| // Verifies process.versions reports the same commits pinned in | ||
| // scripts/build/deps/*.ts. Reading the source files at test time keeps a | ||
| // single source of truth so dep bumps don't require touching this test. | ||
| const depsDir = resolve(import.meta.dir, "../../../../scripts/build/deps"); |
There was a problem hiding this comment.
🟡 The file now has two it("process.versions", ...) blocks with identical names — the new one at line 271 (reads dep commit hashes from scripts/build/deps/*.ts) and a pre-existing one at line 1186 (checks hardcoded node/v8/napi/modules strings). Both run when filtering with -t "^process.versions$", producing duplicate names in test reports and making it harder to tell which assertion failed. Consider renaming the new block to "process.versions (dep commits)" and the old one to "process.versions (node metadata)" to distinguish the two concerns.
Extended reasoning...
What the bug is and how it manifests
The file test/js/node/process/process.test.js has two it() blocks both named "process.versions". The first (line 271, modified by this PR) is an async test that reads each pinned commit from scripts/build/deps/.ts and asserts it matches process.versions[key]. The second (line 1186, pre-existing) is a synchronous test that asserts hardcoded string values for process.versions.node, .v8, .napi, and .modules. Both tests are structurally valid and pass, but they share the same name.
The specific code path that triggers it
The PR description explicitly calls out: USE_SYSTEM_BUN=1 bun test ... -t "^process.versions$" — fails. That filter matches both blocks. Bun's test runner runs every it() whose name matches the regex, so both execute under the filter, and both appear in CI/terminal output with the identical label "process.versions".
Why existing code does not prevent it
Test frameworks (including Bun's) do not enforce unique it() names within a file or suite. The duplicate was present before this PR (confirmed in commit edde070); the PR modified the first block to switch from hardcoded hashes to dynamically derived ones, but kept the name "process.versions" unchanged.
Impact
When one of the two tests fails, the output shows a failure for "process.versions" without indicating which of the two distinct concerns failed. Anyone reading CI logs or filtering with -t would run both tests and see two results under the same label — making debugging needlessly confusing.
How to fix it
Rename one or both tests to reflect what they actually verify:
- Line 271: "process.versions (dep commits)"
- Line 1186: "process.versions (node metadata)"
Step-by-step proof
- Open test/js/node/process/process.test.js.
- Search for it("process.versions" — two matches appear: line 271 (the async dep-hash test added/modified by this PR) and line 1186 (the synchronous node/v8/napi/modules test).
- Run: bun test test/js/node/process/process.test.js -t "^process.versions$"
- Observe two test results both labeled "process.versions" in the output.
- Temporarily break the line-1186 assertion (e.g., change "24.3.0" to "0.0.0"); the failure message says "process.versions" with no indication it is the node-metadata check rather than the dep-commits check.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/node/process/process.test.js`:
- Around line 276-290: The deps object literal (symbol: deps) is missing its
closing brace and semicolon before the declaration of expectedVersions, causing
a syntax error; fix by adding the closing "};" to terminate the deps object
literal (immediately after the last dependency entry) so that const
expectedVersions = {} can follow correctly.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7aa17118-71db-4bf6-8a8c-6a13d471567a
📒 Files selected for processing (1)
test/js/node/process/process.test.js
942c656 on main bumped scripts/build/deps/mimalloc.ts to a29368e but left the hardcoded expectation in process.test.js at 9a5e1f5, so the process.versions test fails on every platform that does a fresh build. Unblocks CI here until #29295 (derive expectations from the dep files) lands and removes the hardcoding entirely.
# Conflicts: # test/js/node/process/process.test.js
|
Rebased this onto main (merge commit f4f505b) and repaired two issues that were keeping it red:
Verified on a fresh debug build: This approach also ends the recurring churn on dep auto-update PRs (see #31315, where the pinned test hash had to be re-applied three times because the update workflow force-pushes only the dep file). With this PR the update workflows need no changes, so I will close #31954 in favor of this. |
There was a problem hiding this comment.
LGTM — the syntax error and regex anchor are both fixed, and this is a test-only maintenance change.
Extended reasoning...
Overview
This PR touches a single test file, test/js/node/process/process.test.js, rewriting the process.versions test to derive expected dependency commit hashes by reading scripts/build/deps/<name>.ts at test time instead of hardcoding them. The goal is to eliminate the recurring need to hand-edit this test on every dep bump.
Changes since my last review
My previous review flagged a missing }; on the deps object literal that broke the whole file at parse time. The latest push (merge commit f4f505b) fixes that and additionally drops the $ anchor from the extraction regex so zlib.ts (whose pin line carries a trailing // 2.3.3 comment) matches. I verified all eleven referenced dep files exist and each contains a line matching /^const [A-Z_]+_COMMIT = "([0-9a-f]{40})";/m. The earlier nit about two it("process.versions") blocks sharing a name remains, but that duplication predates this PR and isn't a blocker.
Security risks
None. This is a test-only change that reads local repo files via Bun.file(...).text() with paths constructed from a fixed literal map — no user input, no network, no privileged operations.
Level of scrutiny
Low. Test-only, ~30 lines net, no production/runtime code touched. The worst-case failure mode is a CI test failure, which is self-evident and easily reverted.
Other factors
The maintainer (robobun) rebased, fixed the outstanding issues, and re-verified locally with both positive and negative checks. The bug-hunting system found nothing new in this revision. The path math (../../../../scripts/build/deps from test/js/node/process/) and the join/resolve imports check out.
# Conflicts: # test/js/node/process/process.test.js
There was a problem hiding this comment.
LGTM — the syntax error is fixed and the regex now handles trailing comments; the duplicate it("process.versions") name I mentioned earlier is pre-existing and not a blocker.
Extended reasoning...
Overview
This PR modifies a single test block in test/js/node/process/process.test.js: instead of hardcoding the expected commit hash for each vendored dependency, the test now reads each pinned commit from scripts/build/deps/<name>.ts at test time using a regex that matches const X_COMMIT = "<40-hex>";. This removes the need to update the test whenever a dep is bumped while still catching the real failure mode (build didn't propagate the pinned commit into process.versions).
Since my last review, robobun pushed a fix that (1) adds the missing }; closing the deps object literal (the 🔴 syntax error I flagged, now marked resolved) and (2) drops the $ anchor from the extraction regex so pins with trailing comments (e.g. zlib.ts) still match. I verified all 11 referenced dep files exist in scripts/build/deps/ and each contains a line matching the regex, including the trailing-comment cases.
Security risks
None. This is a test-only change that reads local repo files at test time. No production/runtime code, no network I/O, no user-controlled input.
Level of scrutiny
Low. Test infrastructure only; the change is mechanical (replace a hardcoded map with a file-read + regex loop), self-contained, and has been verified passing on a fresh debug build per the robobun follow-up. The worst-case failure mode is a false test failure on a future dep pin that doesn't match the regex, which would be caught immediately in CI.
Other factors
My earlier 🟡 note about two it("process.versions") blocks sharing the same name remains, but that duplication predates this PR (this PR only edits one of the two existing blocks) and is a cosmetic reporting concern, not a correctness issue. It shouldn't block merge. No new bugs were found by the bug-hunting system on the current revision.
# Conflicts: # test/js/node/process/process.test.js
There was a problem hiding this comment.
LGTM — the syntax error and regex-anchor issue flagged earlier are both fixed, and the extraction pattern matches all 11 referenced scripts/build/deps/*.ts pins (including zlib.ts with its trailing comment). The earlier note about the duplicate it("process.versions") name is pre-existing and non-blocking.
Extended reasoning...
Overview
Test-only change to test/js/node/process/process.test.js: the process.versions dep-commit test now reads each pinned commit from scripts/build/deps/<name>.ts at test time instead of hardcoding 11 hashes inline. No production code touched.
What changed since my last review
The June 7 push fixed both issues that were open: (1) the unterminated deps object literal that caused a file-wide SyntaxError is now closed with };, and (2) the extraction regex dropped its $ anchor so it matches pins with trailing comments (zlib.ts). I re-verified against the current scripts/build/deps/ — all 11 mapped files (boringssl, libarchive, mimalloc, picohttpparser, zlib, tinycc, lolhtml, cares, libdeflate, zstd, lshpack) contain a line matching ^const [A-Z_]+_COMMIT = "<40-hex>";, and the ../../../../scripts/build/deps path from test/js/node/process/ resolves to the repo root correctly.
Security risks
None. Reads local repo files at test time; no network, no untrusted input, no runtime code paths.
Level of scrutiny
Low. This is test infrastructure that eliminates a recurring source of dep-bump churn. The regex is defensive (asserts non-null with a labeled message so a future format change fails loudly rather than silently skipping a key), and the assertion loop is unchanged from before.
Other factors
The remaining 🟡 inline comment about two it("process.versions") blocks sharing a name is pre-existing (both blocks existed before this PR) and cosmetic — it doesn't affect correctness or CI, so it doesn't block approval.
# Conflicts: # test/js/node/process/process.test.js
There was a problem hiding this comment.
LGTM — the syntax error from the earlier revision is fixed and the regex now handles trailing comments (zlib.ts).
What was reviewed:
- Confirmed all 11 mapped dep files exist in
scripts/build/deps/and each carries a^const X_COMMIT = "<40-hex>";line the regex matches (including zlib/libwebp-style trailing-comment pins). - Verified the
../../../../scripts/build/depspath resolves correctly fromtest/js/node/process/and thatjoin/resolveare already imported. - The duplicate
it("process.versions", ...)name I flagged earlier is pre-existing and cosmetic; not blocking.
Extended reasoning...
Overview
Test-only change to test/js/node/process/process.test.js: the process.versions test previously hardcoded 11 vendored-dependency commit hashes, requiring a matching edit on every dep bump. It now reads each pinned commit from scripts/build/deps/<name>.ts at test time via Bun.file(...).text() and a ^const [A-Z_]+_COMMIT = "(<40-hex>)"; regex, then asserts process.versions[key] matches. The usockets === uwebsockets and sqlite checks are unchanged context.
Prior review status
I previously flagged two issues on this PR: a 🔴 missing }; on the deps object literal (parse-time SyntaxError) and a 🟡 duplicate it("process.versions") name. The rebase on 2026-06-07 fixed the syntax error and additionally dropped the $ anchor from the extraction regex (with an explanatory comment) so zlib.ts's trailing // 2.3.3 comment doesn't defeat the match. The current diff reflects both fixes. The duplicate-name nit was not addressed, but it predates this PR (the second block already existed with that name) and is purely a test-report readability concern — not worth blocking on.
Security risks
None. This is a test file reading build-script sources from the local checkout; no untrusted input, no network, no production code touched.
Level of scrutiny
Low. Test-only refactor that replaces a hardcoded lookup table with a derived one. I verified against the current source tree that every entry in the deps map (boringssl, libarchive, mimalloc, picohttpparser, zlib, tinycc, lolhtml, cares, libdeflate, zstd, lshpack) has a corresponding .ts file containing a line matching the regex, so the extraction loop cannot vacuously pass on an empty or mis-shaped file — the expect(match).not.toBeNull() guard would fire first.
Other factors
The change was re-verified by robobun after the rebase (bun bd test ... -t "process.versions" passes; tampering a hash in lshpack.ts produces the expected failure diff). The motivation — eliminating recurring churn on automated dep-update PRs — is sound and matches the "one source of truth" guidance in REVIEW.md.
# Conflicts: # test/js/node/process/process.test.js
There was a problem hiding this comment.
LGTM — the earlier syntax error (unclosed deps object) is fixed and the regex now handles trailing comments on pin lines.
What was reviewed:
- Verified all 11 referenced
scripts/build/deps/*.tsfiles exist and match/^const [A-Z_]+_COMMIT = "([0-9a-f]{40})";/m(includingzlib.tswith its// 2.3.3trailing comment). - Confirmed
resolve/joinare already imported and the../../../../scripts/build/depspath resolves to repo root correctly fromtest/js/node/process/. - The pre-existing duplicate
it("process.versions")name I flagged earlier is unchanged, but it predates this PR and is a cosmetic nit — not blocking.
Extended reasoning...
Overview
This PR touches a single test file, test/js/node/process/process.test.js, rewriting the process.versions test body to read expected dependency commit hashes from scripts/build/deps/<name>.ts at test time instead of hardcoding them. Same 11 keys covered, same downstream assertions (toHaveProperty + toBe), plus the unchanged usockets/uwebsockets/sqlite checks. No production code, no new files.
Security risks
None. Test-only change reading local repo files with a fixed relative path. No network, no secrets, no user input.
Level of scrutiny
Low. This is a maintenance refactor of a metadata-consistency test. The failure mode it guards (build not propagating pinned dep commits into process.versions) is preserved — the test still compares the built binary against the source tree. What it gives up (a second copy of the hashes as an independent check) is exactly the maintenance burden the PR is designed to eliminate, and the maintainer comment confirms this churn was recurring on auto-update PRs.
Other factors
- My prior 🔴 finding (missing
};closing thedepsliteral → parse error) is resolved in the current diff — the closing brace is present as a context line, and the inline thread is marked resolved. - The regex was adjusted per the rebase comment to drop the
$anchor; I confirmed against the currentscripts/build/deps/tree that every referenced file matches, includingzlib.tswhich carries a trailing// 2.3.3comment. - My prior 🟡 nit (two
it("process.versions")blocks with identical names) was not addressed, but it is pre-existing — this PR only rewrote the body of the first block, it did not introduce the duplicate. It was phrased as a suggestion and is out of scope for a hardcoded-hash-removal change. - The CI failures on the latest build are an unrelated flaky worker stress test and a binary-size delta measured against a stale canary — a test-only diff cannot affect binary size.
- The rebase/repair was done by robobun (maintainer bot) with explicit verification on a fresh debug build including a negative check, which further reduces risk.
# Conflicts: # test/js/node/process/process.test.js
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
LGTM — the earlier syntax error and $-anchor regex issue are both fixed.
What was reviewed:
- Confirmed all 11 referenced files exist in
scripts/build/deps/and each matches^const [A-Z_]+_COMMIT = "([0-9a-f]{40})";(includingzlib.tswith its trailing// 2.3.3comment). - Verified
resolve(import.meta.dir, "../../../../scripts/build/deps")lands at the repo root fromtest/js/node/process/. - The pre-existing duplicate
it("process.versions")name I flagged earlier is unchanged, but it predates this PR and is cosmetic only.
Extended reasoning...
Overview
This PR touches a single test file, test/js/node/process/process.test.js, rewriting the process.versions test to derive expected dep commit hashes at runtime by reading scripts/build/deps/<name>.ts and matching ^const [A-Z_]+_COMMIT = "<40-hex>";, instead of hardcoding eleven commit hashes inline. No production code is touched.
Security risks
None. The change reads local repo files at test time and compares strings against process.versions. No network, no credentials, no user-controlled input.
Level of scrutiny
Low. This is a test-maintenance refactor whose purpose is to eliminate the recurring need to hand-edit hardcoded hashes on every dep bump. The failure mode it guards (build not propagating the source-tree commit into process.versions) is preserved — the test still asserts exact equality against the pinned commit, just sourced from the same file the build reads.
Verification
I grepped scripts/build/deps/ and confirmed each of the 11 mapped files (boringssl, libarchive, mimalloc, picohttpparser, zlib, tinycc, lolhtml, cares, libdeflate, zstd, lshpack) contains exactly one const *_COMMIT = "<40-hex>"; line matching the regex. The dropped $ anchor is necessary and sufficient for zlib.ts ("; // 2.3.3), libwebp.ts, etc. The ares → cares filename mapping is correct. The relative path from test/js/node/process/ up four levels to scripts/build/deps resolves correctly.
Other factors
Two prior review findings have been addressed: the missing }; closing the deps literal (fixed — now present in the diff) and the over-strict $ regex anchor (dropped, with a comment explaining why). The robobun follow-up confirms both fixes and re-verified the test on a fresh debug build plus a negative check with a tampered hash. My earlier 🟡 note about the duplicate it("process.versions") name remains, but that duplication predates this PR (both blocks existed before) and is a cosmetic reporting nit, not a correctness issue — not worth blocking on. The bug-hunting system found nothing this run.
2 participants
What does this PR do?
The
process.versionstest intest/js/node/process/process.test.jshardcoded the expected commit hash for every vendored dependency. Each dep bump — including the automatedupdate-*.ymlworkflows fixed in #29289 — required a matching edit here, and forgetting it broke CI.This rewrites the test to read each pinned commit out of
scripts/build/deps/<name>.tsat test time using the same^const X_COMMIT = "([0-9a-f]{40})";$pattern the workflows use. Single source of truth: bumping a dep no longer requires touching this test, and it still catches the real failure mode (build didn't propagate the source-tree commit throughdepVersionsHeader.ts→bun_dependency_versions.h→process.versions).How did you verify your code works?
bun bd test test/js/node/process/process.test.js— 101 pass, 0 failUSE_SYSTEM_BUN=1 bun test ... -t "^process.versions$"— fails (system bun has older libarchive than source tree)ZSTD_COMMITinscripts/build/deps/zstd.tsto a dummy hash → test fails with clear expected/received diff