test: split test-fs-cp.js

[joyeecheung]

This test previously squeezed 70+ test cases into one single file and has been constantly crashing on Windows with exit code 3221226505 and no stack trace. As it is already marked as flaky there is no way to understand which test case is failing and the Windows CI was constantly orange. This patch splits the test cases into different files so it's easier to find out which case is exactly failing and to be skipped.

Refs: #56794

[joyeecheung]

Windows CI to figure out who is crashing: https://ci.nodejs.org/job/node-test-commit-windows-fanned/72339/

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.90%. Comparing base (f3adc11) to head (83d3b6b).
⚠️ Report is 12 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #59408      +/-   ##
==========================================
+ Coverage   89.79%   89.90%   +0.11%     
==========================================
  Files         653      655       +2     
  Lines      192394   192829     +435     
  Branches    37650    37793     +143     
==========================================
+ Hits       172753   173369     +616     
+ Misses      12270    12013     -257     
- Partials     7371     7447      +76     

see 59 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/68512/

[joyeecheung]

7 split test cases have failed on Windows. I noticed that there have been quite a lot of changes to fs.cpSync even after the test has been marked as flaky #56799 - for the purpose of this PR, I will just mark them as flaky. But it deserves more investigation because the tests could've already been broken by the recent changes and they were just ignored due to the flaky status. cc @dario-piotrowicz

See failures

not ok 339 parallel/test-fs-cp-sync-error-on-exist
  ---
  duration_ms: 267.00200
  severity: fail
  exitcode: 1
  stack: |-
    node:internal/modules/run_main:107
        triggerUncaughtException(
        ^
    
    AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
    + actual - expected
    
      Comparison {
    +   code: ''
    -   code: 'ERR_FS_CP_EEXIST'
      }
    
        at file:///c:/workspace/node-test-binary-windows-js-suites/node/test/parallel/test-fs-cp-sync-error-on-exist.mjs:14:8
        at ModuleJob.run (node:internal/modules/esm/module_job:371:25)
        at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:669:26)
        at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:101:5) {
      generatedMessage: true,
      code: 'ERR_ASSERTION',
      actual: Error: , The file exists. '\\?\c:\workspace\node-test-binary-windows-js-suites\node\test\.tmp.339\copy_%1\a\b'
          at copyDir (node:internal/fs/cp/cp-sync:145:22)
          at onDir (node:internal/fs/cp/cp-sync:137:10)
          at getStats (node:internal/fs/cp/cp-sync:68:12)
          at cpSyncFn (node:internal/fs/cp/cp-sync:58:10)
          at cpSync (node:fs:3123:3)
          at assert.throws.code (file:///c:/workspace/node-test-binary-windows-js-suites/node/test/parallel/test-fs-cp-sync-error-on-exist.mjs:15:9)
          at getActual (node:assert:584:5)
          at ok.throws (node:assert:732:24)
          at file:///c:/workspace/node-test-binary-windows-js-suites/node/test/parallel/test-fs-cp-sync-error-on-exist.mjs:14:8
          at ModuleJob.run (node:internal/modules/esm/module_job:371:25) {
        errno: 80,
        code: '',
        path: '\\\\?\\c:\\workspace\\node-test-binary-windows-js-suites\\node\\test\\.tmp.339\\copy_%1\\a\\b',
        syscall: 'cp'
      },
      expected: { code: 'ERR_FS_CP_EEXIST' },
      operator: 'throws',
      diff: 'simple'
    }
    
    Node.js v25.0.0-pre
  ...

not ok 337 parallel/test-fs-cp-sync-copy-symlink-not-pointing-to-folder
  ---
  duration_ms: 154.00100
  severity: fail
  exitcode: 1
  stack: |-
    node:internal/modules/run_main:107
        triggerUncaughtException(
        ^
    
    AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
    + actual - expected
    
    + 'C:\\workspace\\node-test-binary-windows-js-suites\\node\\test\\.tmp.337\\copy_%1'
    - 'c:\\workspace\\node-test-binary-windows-js-suites\\node\\test\\.tmp.337\\copy_%1'
    
        at file:///c:/workspace/node-test-binary-windows-js-suites/node/test/parallel/test-fs-cp-sync-copy-symlink-not-pointing-to-folder.mjs:19:8
        at ModuleJob.run (node:internal/modules/esm/module_job:371:25)
        at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:669:26)
        at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:101:5) {
      generatedMessage: true,
      code: 'ERR_ASSERTION',
      actual: 'C:\\workspace\\node-test-binary-windows-js-suites\\node\\test\\.tmp.337\\copy_%1',
      expected: 'c:\\workspace\\node-test-binary-windows-js-suites\\node\\test\\.tmp.337\\copy_%1',
      operator: 'strictEqual',
      diff: 'simple'
    }
    
    Node.js v25.0.0-pre
  ...

not ok 343 parallel/test-fs-cp-sync-symlink-points-to-dest-error
  ---
  duration_ms: 172.00200
  severity: fail
  exitcode: 1
  stack: |-
    node:internal/modules/run_main:107
        triggerUncaughtException(
        ^
    
    AssertionError [ERR_ASSERTION]: Missing expected exception.
        at file:///c:/workspace/node-test-binary-windows-js-suites/node/test/parallel/test-fs-cp-sync-symlink-points-to-dest-error.mjs:17:8
        at ModuleJob.run (node:internal/modules/esm/module_job:371:25)
        at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:669:26)
        at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:101:5) {
      generatedMessage: false,
      code: 'ERR_ASSERTION',
      actual: undefined,
      expected: { code: 'ERR_FS_CP_EINVAL' },
      operator: 'throws',
      diff: 'simple'
    }
    
    Node.js v25.0.0-pre
  ...

not ok 344 parallel/test-fs-cp-sync-resolve-relative-symlinks-false
  ---
  duration_ms: 184.00600
  severity: fail
  exitcode: 1
  stack: |-
    node:internal/modules/run_main:107
        triggerUncaughtException(
        ^
    
    AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
    + actual - expected
    
    + 'C:\\workspace\\node-test-binary-windows-js-suites\\node\\test\\.tmp.343\\copy_%1\\foo.js'
    - 'c:\\workspace\\node-test-binary-windows-js-suites\\node\\test\\.tmp.343\\copy_%1\\foo.js'
    
        at file:///c:/workspace/node-test-binary-windows-js-suites/node/test/parallel/test-fs-cp-sync-resolve-relative-symlinks-false.mjs:21:8
        at ModuleJob.run (node:internal/modules/esm/module_job:371:25)
        at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:669:26)
        at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:101:5) {
      generatedMessage: true,
      code: 'ERR_ASSERTION',
      actual: 'C:\\workspace\\node-test-binary-windows-js-suites\\node\\test\\.tmp.343\\copy_%1\\foo.js',
      expected: 'c:\\workspace\\node-test-binary-windows-js-suites\\node\\test\\.tmp.343\\copy_%1\\foo.js',
      operator: 'strictEqual',
      diff: 'simple'
    }
    
    Node.js v25.0.0-pre
  ...

not ok 335 parallel/test-fs-cp-async-symlink-points-to-dest
  ---
  duration_ms: 197.00000
  severity: fail
  exitcode: 1
  stack: |-
    file:///c:/workspace/node-test-binary-windows-js-suites/node/test/parallel/test-fs-cp-async-symlink-points-to-dest.mjs:19
      assert.strictEqual(err.code, 'ERR_FS_CP_EINVAL');
                             ^
    
    TypeError: Cannot read properties of null (reading 'code')
        at file:///c:/workspace/node-test-binary-windows-js-suites/node/test/parallel/test-fs-cp-async-symlink-points-to-dest.mjs:19:26
        at c:\workspace\node-test-binary-windows-js-suites\node\test\common\index.js:437:15
        at node:fs:180:23
        at process.processTicksAndRejections (node:internal/process/task_queues:90:21)
    
    Node.js v25.0.0-pre
  ...

not ok 346 parallel/test-fs-cp-sync-unicode-folder-names
  ---
  duration_ms: 271.00300
  severity: fail
  exitcode: 3221226505
  stack: |-
  ...

not ok 343 parallel/test-fs-cp-sync-resolve-relative-symlinks-default
  ---
  duration_ms: 173.00700
  severity: fail
  exitcode: 1
  stack: |-
    node:internal/modules/run_main:107
        triggerUncaughtException(
        ^
    
    AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
    + actual - expected
    
    + 'C:\\workspace\\node-test-binary-windows-js-suites\\node\\test\\.tmp.342\\copy_%1\\foo.js'
    - 'c:\\workspace\\node-test-binary-windows-js-suites\\node\\test\\.tmp.342\\copy_%1\\foo.js'
    
        at file:///c:/workspace/node-test-binary-windows-js-suites/node/test/parallel/test-fs-cp-sync-resolve-relative-symlinks-default.mjs:21:8
        at ModuleJob.run (node:internal/modules/esm/module_job:371:25)
        at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:669:26)
        at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:101:5) {
      generatedMessage: true,
      code: 'ERR_ASSERTION',
      actual: 'C:\\workspace\\node-test-binary-windows-js-suites\\node\\test\\.tmp.342\\copy_%1\\foo.js',
      expected: 'c:\\workspace\\node-test-binary-windows-js-suites\\node\\test\\.tmp.342\\copy_%1\\foo.js',
      operator: 'strictEqual',
      diff: 'simple'
    }
    
    Node.js v25.0.0-pre
  ...

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/68519/

[joyeecheung]

FWIW in the 7 failed test cases, I can reproduce the 3 involving errors locally, I think they are just test cases being broken by recent changes without being noticed. The ones related to readlinkSync not returning an expected result (drive letters with different case) might have been breakages from readlink and specific to the setup of the CI machines. I don't know what's going on with the crashes with exist code 3221226505 but I cannot reproduce them.

[jasnell]

Thank you for doing this!!! Sorely needed and it's been on my todo list for a while but just hadn't been able to find time. One suggestion that hopefully might make this easier... let's split this into two PRs... One PR containing all of the separated out tests that are not problematic at all, that we can hopefully get landed easily, and another PR that just separates the tests that are known to be problematic. That should (hopefully) make it at least a little bit easier to focus on just the tests that highlight problems.

[jasnell]

@joyeecheung ... one thing to point out on some of the cp-sync errors. I noticed that in manual testing that at least a couple of the error messages themselves were incorrect due to argument order on the error construction being reversed but the tests were missing it entirely because the tests weren't looking at the error messages, only the codes. It was specifically cpSync that I saw this on but, again, due to just pure lack of available time, I haven't had a moment to correct it yet.

[joyeecheung]

One PR containing all of the separated out tests that are not problematic at all, that we can hopefully get landed easily, and another PR that just separates the tests that are known to be problematic.

Are you referring to one PR or one file? I don't think it's wise to continue keeping them in one file, that just repeats the mistake of keeping them in a monolith. This PR just splits all of them and marked the ones that have failed as flaky, if it's good enough for the Windows CI to not be red we can just go from there - people who have time to fix the individual flakes can just start fixing them in follow up PRs. If there are any more failures we can just mark individual test cases as flaky without disabling others. But if we keep them in one file, there will be a lot more churn than just adding an entry to the status file if there are still flakes in the monolith.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/68527/

[jasnell]

Are you referring to one PR or one file?...

PR, not file. It's awesome that you're splitting this into separate files. Let's take all the separate tests that pass with no problem and separate them out into separate files in one PR, then take all the ones that don't pass for whatever reason, or have bugs, and split those out into a separate PR. That way we can more easily ignore the ones that don't have any issues and we can more easily focus on the problems in the bad ones.

[joyeecheung]

Let's take all the separate tests that pass with no problem and separate them out into separate files in one PR, then take all the ones that don't pass for whatever reason, or have bugs, and split those out into a separate PR.

Isn't that effectively: let's merge the failing tests that's already split here back into one file again and continue to not have visibility into failure reasons of all of them, only to split them in a later PR?

In any case, the CI is already orange in this one, the problematic test cases are already captured in the status file and the passing ones are left alone, so I think it has already arrived at the state after the two proposed separate PRs. Unless you insist that this PR must merge the split flakes back into one file, so that someone can open a follow up PR to split them again (I can't see what's the benefit of that?) I think this is good to land?

[joyeecheung]

From what I can tell, @jasnell 's suggestion is non-blocking, that was just a suggestion to make it easier to land, but then this PR is already ready to land, and splitting it up would just make things more difficult and stalling it (I don't think I'll have time to merge the already split tests back into one file only to split them up again in another PR). Unless I heard otherwise, going to land it later today to make the CI output of the flakes less useless.

[joyeecheung]

Looks like this needs a re-approval after the last push. @panva @lpinca @daeyeon @himself65 can you take a look again? Thanks

[nodejs-github-bot]

Landed in aac7925

[dario-piotrowicz]

@joyeecheung ... one thing to point out on some of the cp-sync errors. I noticed that in manual testing that at least a couple of the error messages themselves were incorrect due to argument order on the error construction being reversed but the tests were missing it entirely because the tests weren't looking at the error messages, only the codes. It was specifically cpSync that I saw this on but, again, due to just pure lack of available time, I haven't had a moment to correct it yet.

When I made changes in cpSync I did manually test the error cases as well (and I also didn't see the tests failing either, probably due to the flakiness), sorry if I missed something 😓

anyways the changes I've made in cpSync (as well as those introduced by @anonrig I believe) are all perf related and should be simple to revert. That could be good also for #58869. What do you think? Would that maybe be a good path forward? (as in, moving back things to the js layer, consolidate cp/cpSync so that they share as much logic as possible and afterwords retry to move things in the C++ layer (with a better understanding and awareness of the various issues that surfaces since))

[joyeecheung]

I did manually test the error cases as well

FWIW I am not trying to blame anyone, if anything I'd blame the test monoliths. I think it may have gone unnoticed if you did not run it on Windows? Because the failures are Windows-specific. If the tests were split up you'd notice individual failures in the Windows CI. But being in a monolith marked as flaky and if it's just crashing without stack trace, you won't be able to figure out anything unless you do a splitting like what this PR does first. I was raising the possibility because the monolith was marked as flaky 6 months ago so theoretically people would still have noticed breakages before that. But after that unless you go check the orange platforms you wouldn't notice it was failing.

What do you think? Would that maybe be a good path forward?

I think it's been long enough that reverting may become complicated. But if the revert comes back with no source conflicts it would be better to revert - generally I prefer "revert ASAP, fix and reland later", which is how Chromium/V8 manages to keep their CI mostly green. If not, I'd suggest fixing the 3 failures around error reporting mentioned above to at least make the CI less orange (or I think they are actual bugs because they reproduce consistently on Windows locally).

[dario-piotrowicz]

I did manually test the error cases as well

FWIW I am not trying to blame anyone, if anything I'd blame the test monoliths. I think it may have gone unnoticed if you did not run it on Windows? Because the failures are Windows-specific. If the tests were split up you'd notice individual failures in the Windows CI. But being in a monolith marked as flaky and if it's just crashing without stack trace, you won't be able to figure out anything unless you do a splitting like what this PR does first. I was raising the possibility because the monolith was marked as flaky 6 months ago so theoretically people would still have noticed breakages before that. But after that unless you go check the orange platforms you wouldn't notice it was failing.

What do you think? Would that maybe be a good path forward?

I think it's been long enough that reverting may become complicated. But if the revert comes back with no source conflicts it would be better to revert - generally I prefer "revert ASAP, fix and reland later", which is how Chromium/V8 manages to keep their CI mostly green. If not, I'd suggest fixing the 3 failures around error reporting mentioned above to at least make the CI less orange (or I think they are actual bugs because they reproduce consistently on Windows locally).

Thanks @joyeecheung (and so sorry for the late reply 🙇)

Thanks a lot for the understanding, anyways I will poke around to see if the revert is feasible 🙂🙏