Skip to content

node:fs: add Node's test for cpSync errorOnExist on an existing directory - #43988

Open
robobun wants to merge 1 commit into
mainfrom
robobun/e7508c2e/vendor-cp-sync-dir-exists-test
Open

robobun wants to merge 1 commit into
mainfrom
robobun/e7508c2e/vendor-cp-sync-dir-exists-test

Conversation

@robobun

@robobun robobun commented Sep 25, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • No test on main fails when fs.cpSync stops rejecting an existing destination directory. The options are { recursive: true, force: false, errorOnExist: true }.
  • The guard in onDir (src/js/internal/fs/cp-sync.ts:455) gives the rejection. With the guard removed, the 77 vendored test-fs-cp-* files and test/js/node/fs/cp.test.ts still pass. node:fs: apply cpSync errorOnExist per entry, not to the destination directory #33416 (closed) proposed that removal.

Fix

  • Add Node's test/parallel/test-fs-cp-sync-dir-exists-error-on-exist.mjs, unmodified. The git blob is bcc95476a9f6, the same as at nodejs/node tag v26.10.0.
  • The test copies a directory onto an existing directory that holds no colliding file. It expects ERR_FS_CP_EEXIST.
  • This PR changes nothing under src/. The test passes on main. It fails with AssertionError: Missing expected exception. when the guard is removed.
  • Verified: the new file on a debug build, through scripts/runner.node.ts. Also all 78 test-fs-cp-* files and test/js/node/fs/cp.test.ts.

Background

Notes

Results for the new file, all on linux-x64:

Runtime Result
Node v26.10.0 (tarball sha256 checked against SHASUMS256.txt) exit 0
Node v26.3.0 (before nodejs/node#64124) AssertionError [ERR_ASSERTION]: Missing expected exception.
bun 1.4.3-canary.1+367d939d9 (release) exit 0
bun debug build of this branch exit 0
bun debug build, guard at cp-sync.ts:455-463 removed AssertionError: Missing expected exception., exit 1
  • bun run node:test:cp fetches from the main branch of nodejs/node, not from a tag. The file on main was identical to the file at v26.10.0 at fetch time. The file does not exist at v26.9.0.
  • cpSync takes the native fast path only when errorOnExist is false and force is true (src/js/node/fs.ts:973). This test always runs the JavaScript walker, on every platform.
  • In the tracked test tree, errorOnExist occurs in cp.test.ts, in 5 vendored test-fs-cp-* files (the new file included), and in one helper under test/js/node/test/common/. All of these ran against the build without the guard.
  • test/js/node/fs/cp.test.ts on the debug build: 47 pass, 5 skip (Windows only), 0 fail, with and without the guard.
  • fs.cp: take the native recursive copy again when it gives the walker's result #42906 (open) changes when cpSync takes the native path. Its diff keeps the rejection of an existing destination directory.

no test proof · iteration 0 · no src or test change; test-proof not applicable

…tory

Add test/parallel/test-fs-cp-sync-dir-exists-error-on-exist.mjs from
nodejs/node, unmodified. The file is identical to the one at tag
v26.10.0 (git blob bcc9547).

The test calls fs.cpSync with { recursive: true, force: false,
errorOnExist: true } on a destination directory that exists and holds
no colliding file. It expects ERR_FS_CP_EEXIST.

The guard in onDir (src/js/internal/fs/cp-sync.ts) already gives this
result. With the guard removed, the 77 other test-fs-cp-* files and
test/js/node/fs/cp.test.ts still pass on linux-x64.
@robobun

robobun commented Sep 25, 2026

Copy link
Copy Markdown
Collaborator Author

Status: ready for review. This PR adds one test file and changes nothing under src/.

How to check it:

  1. Run bun bd run test/js/node/test/parallel/test-fs-cp-sync-dir-exists-error-on-exist.mjs. It exits 0.
  2. Remove the guard at src/js/internal/fs/cp-sync.ts:455-463 and run the same command. It fails with AssertionError: Missing expected exception.

The same file exits 0 on Node v26.10.0 and fails on Node v26.3.0, which does not have nodejs/node#64124.

@coderabbitai

coderabbitai Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: oven-sh/bun/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: 0c443711-4f80-4dcf-b4f1-e786cb999ffa

📥 Commits

Reviewing files that changed from the base of the PR and between 29d9638 and 04c950d.

📒 Files selected for processing (1)
  • test/js/node/test/parallel/test-fs-cp-sync-dir-exists-error-on-exist.mjs

Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review.


Walkthrough

Adds a test that checks cpSync throws ERR_FS_CP_EEXIST when it copies a directory into an existing destination directory with recursive: true, errorOnExist: true, and force: false.

Changes

cpSync existing-directory error test

Layer / File(s) Summary
Assert error for an existing destination directory
test/js/node/test/parallel/test-fs-cp-sync-dir-exists-error-on-exist.mjs
Creates source and destination directories with different files. Checks that cpSync throws ERR_FS_CP_EEXIST with the specified options.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 04c95

The added test covers the intended error case; no issue identified here prevents merging after normal checks.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The description clearly explains the problem, the test change, expected behavior, and verification results. It does not use the exact template headings, but it provides the required information in equ…
Title check ✅ Passed The title clearly identifies the main change: adding a Node.js test for cpSync errorOnExist behavior on an existing directory.

Comment @coderabbitai help to get the list of available commands.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — a single ported Node test file, no runtime changes.

What was reviewed:

  • The new file matches the sibling async test (test-fs-cp-async-dir-exists-error-on-exist.mjs) line-for-line apart from cpSync/assert.throws, and the diff header's new-blob hash (bcc95476) is consistent with the claimed upstream blob.
  • Traced the asserted path: with errorOnExist true the src/js/node/fs.ts:973 native fast path is skipped, so the JS walker's onDir guard at src/js/internal/fs/cp-sync.ts:455 fires and decorateSystemError leaves .code as ERR_FS_CP_EEXIST.
  • Ruled out the mixed-separator path concern on Windows: the identical construction already runs in the async sibling, and fs helpers accept it.
Extended reasoning...

The PR adds one 32-line file under test/js/node/test/parallel, a directory of vendored Node.js tests that Bun runs as scripts, and touches nothing under src/. It exercises fs.cpSync with { recursive: true, errorOnExist: true, force: false } against an existing destination directory with non-colliding contents and expects ERR_FS_CP_EEXIST. No security-sensitive surface is involved. The existing onDir guard in cp-sync.ts already produces this error and the fast-path condition in fs.ts excludes errorOnExist, so the test should pass on main while covering a guard that previously had no dedicated test; the file is structurally identical to the already-vendored async sibling, and no CODEOWNER covers the path.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant