fix(test): exclude the cwd-exclusion pair from the Node and Bun runners - #3500
Conversation
Both runners decide whether a test is Deno-only by reading that file's own source for `Deno.`. The cwd-exclusion pair keeps its Deno usage in the helper it imports, so the heuristic missed it and `deno task test:node` began failing on `Deno.chdir` being undefined. Named explicitly rather than by making the files mention `Deno.` to satisfy the heuristic: the pair asserts a property of `deno test --parallel` itself -- that test files sharing one process do not share a working directory -- so it is Deno-only by subject and not merely by which API it happens to call. CI runs neither task, which is why this survived review.
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling 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 (5)
📝 WalkthroughWalkthroughThe test runners now exclude ChangesRuntime-specific test filters
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/node/run-tests.mjs`:
- Around line 47-56: Add focused coverage in both tests/node/run-tests.mjs
(lines 47-56) and tests/bun/run-tests.mjs (lines 48-52) for the runtime filters:
verify src/testing/cwd-exclusion-a.test.ts and
src/testing/cwd-exclusion-b.test.ts are excluded while an unrelated test remains
eligible. Ensure the assertions cover the Deno-only filename filter
independently of the source-based Deno. usage heuristic.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d5ff3498-8f36-4244-9ff0-83a3e80143a6
📒 Files selected for processing (2)
tests/bun/run-tests.mjstests/node/run-tests.mjs
The reviewer asked for coverage on both runners' filters, which the exclusion had none of: it was a literal in each runner, duplicated, and nothing noticed if it stopped matching. Renaming the pair or moving it out of `src/testing/` would leave a pattern matching nothing and `deno task test:node` would fail again, which is the regression this list was added for. The patterns move to `tests/deno-only-tests.mjs`, imported by both runners so they cannot drift, and `tests/runtime-test-filters.test.ts` asserts both directions: the pair is excluded, and its neighbours -- including `cwd.test.ts`, which shares a prefix -- stay eligible. Both halves matter; a filter that excludes too much shrinks the suite silently. Checked against a list emptied and a pattern widened, and it fails on each. Also fixes an unrelated Deno-only import that made `deno task test:bun` fail before it reached any of this: `extensions/ext-bundler-esbuild` wrote `npm:es-module-lexer@2.3.1` inline while its own deno.json already maps the bare specifier, as it does for esbuild. Only Deno resolves the inline form.
1 participant
deno task test:nodefails onmain:Both alternate runners decide whether a test is Deno-only by reading that file's own source for
Deno.. The cwd-exclusion pair added in #3494 keeps its Deno usage in the helper it imports, so the heuristic missed it, the file ran under Node, andDeno.chdirwas undefined.Named explicitly rather than making the files mention
Deno.to satisfy the heuristic. The pair asserts a property ofdeno test --parallelitself — that test files sharing one process do not share a working directory — so it is Deno-only by subject, not merely by which API it happens to call.Verification
node ./tests/node/run-tests.mjs 'src/testing/*.test.ts'— fails onmain, passes here.Unrelated pre-existing Node failures remain (
src/config/define-config.test.tsfails onmaintoo, without these changes); this PR does not address them.CI runs neither
test:nodenortest:bun, which is why the regression survived review — worth considering separately.Summary by CodeRabbit