test(node): run portable API loader coverage - #3989
Conversation
|
Warning Review limit reached
Next review available in: 13 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
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 |
📦 Client bundle boundary
A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e54342252
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A Windows checkout with core.autocrlf=true ends the first line in CRLF, so an exact LF comparison read the header as absent. The heuristic then saw the guarded Deno references in loader.test.ts and dropped the whole file from the Node and Bun suites, removing the portable coverage this change adds. Move the check into hasRuntimeGuardedDenoHeader, which compares the first line with its trailing carriage return stripped, and use it from both the runner filter and the repository contract test.
main moved the lockfile-failure case onto withMockFetch while this branch changed the same assertion to match both the Deno and the Node wording. Kept both: the mock fetch wrapper from main, the captured error and the portable assertMatch from this branch.
#3989 moved the Deno-dependence predicate into `tests/test-file-utils.mjs` as `isDenoDependentTestSource` and gave it an opt-out: a file whose first line is `// @veryfront-test runtime-guarded-deno` stays eligible for the Node and Bun suites. `src/routing/api/module-loader/loader.test.ts` uses it. This branch had already lifted the old inline predicate into the planner, so a plain merge kept the pre-#3989 copy and the Node suite dropped that file again: 1267 files on main, 1266 after the merge, with no runner reporting anything but a pass. The planner now calls the shared predicate instead of carrying its own. The parity test could not catch it, because its legacy helper carried a third copy of the same rule and agreed with the stale planner. It calls the shared predicate now too, and a new case asserts the Node plan contains the module-loader file, which fails without this change. `tests/runtime-test-filters.test.ts` keeps guarding the predicate itself. Verified: every suite selects the same files as the command it replaced, measured on this tree. unit:parallel 1941, unit:cwd 7, unit:cwd-exclusion 2 (1941+7+2 = 1950 = coverage:unit), integration:legacy-tests-root 180, integration:cli 24, runtime:node 1267, runtime:bun 1394. Node and Bun match origin/main file for file. Conflicts resolved: `tests/node/run-tests.mjs` and `tests/bun/run-tests.mjs` keep the planner bridges (main's dead local predicates are gone with the selection code that used them); `tests/runtime-test-filters.test.ts` keeps both import sides.
2 participants
Summary
Red and green
The regression test initially failed because
loader.test.tswas classified as Deno-only. The first real Node run then surfaced four runtime-specific cases, which are now explicitly guarded instead of excluding the whole file.The targeted Node run visibly executes 63 loader tests: 52 pass, 11 explicit Deno-only skips, 0 failures. The preparation test exercises route source hashing, so an unsupported primitive in that path fails the Node job.
Verification
deno task test:node(all shards, zero failures)bun test tests/test-file-utils.test.mjs(39 pass)deno task build:npmbash scripts/test/npm-install-smoke.shdeno task lintdeno task typecheckRefs veryfront/veryfront-issue-inbox#730
Review follow-up
.gitattributesin this repo, so a Windows clone withcore.autocrlf=trueread the header as absent and dropped the whole file from the Node and Bun suites, which is the exact exclusion this change exists to remove. The check moved into a sharedhasRuntimeGuardedDenoHeaderthat strips a trailing carriage return, andtests/runtime-test-filters.test.tsnow calls the same helper instead of repeating the comparison.origin/main. The one conflict was the lockfile-failure case: main moved it ontowithMockFetchwhile this branch changed the same assertion to match both the Deno and the Node wording. Kept both.Uint8Array.prototype.toHex()call in the module-loader hashing path (toHexinsrc/utils/hash-utils.ts, the [BUG] Every API route fails to load on stock Node — module loader calls Deno-native Uint8Array.prototype.toHex() #3968 defect) makes the targeted Node run fail withTypeError: (intermediate value).toHex is not a function, exit 1. The same mutation againstorigin/main's runner filters exits 0 and prints nothing, because the file was excluded.