Codecov Report

Attention: Patch coverage is 53.73134% with 31 lines in your changes missing coverage. Please review.

Project coverage is 89.16%. Comparing base (50d405a) to head (896e117).
Report is 94 commits behind head on main.

@@            Coverage Diff             @@
##             main   #56117      +/-   ##
==========================================
- Coverage   89.22%   89.16%   -0.06%     
==========================================
  Files         663      665       +2     
  Lines      191974   192604     +630     
  Branches    36926    37055     +129     
==========================================
+ Hits       171286   171742     +456     
- Misses      13561    13664     +103     
- Partials     7127     7198      +71     

... and 48 files with indirect coverage changes

Can you please update your first commit message to follow the guideline, i.e. each line needs to be below 72 chars. Also there are some linting issues in your js test.

@jazelly I made the updates for the first commit message and linter issues. I think it should be fine now.

@jazelly This time a linter issue: I didn't put a newline at the end of the test file. I am learning new things! Thanks for your understanding.

@jazelly @joyeecheung It's updated based on the review.

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

This doesn't add new implementation of the conversion, it just moves existing conversion macros to somewhere earlier so that it can be used in an earlier function. If you want the conversion macros to use ToU8StringView() instead of via wide strings, I think it would be better if you open a separate PR to update these macros. For the purpose of fixing this bug, which has an increasing amount of reports being filed, I think what's done in this PR is adequate, and there is no need to block a bug fix because the existing helper it uses could've been worked better in a separate PR.

I agree with @joyeecheung here. The conversion to use ToU8StringView() can be done separately.

It looks like the CI is failing on Windows:

---
duration_ms: 198.975
exitcode: 1
severity: fail
stack: |-
  node:internal/assert/utils:281
      throw err;
      ^

  AssertionError [ERR_ASSERTION]: Error message should include the path treated as a directory
      at Object.<anonymous> (C:\workspace\node-test-binary-windows-js-suites\node\test\parallel\test-fs-rmSync-special-char.js:41:3)
      at expectedException (node:assert:808:17)
      at expectsError (node:assert:931:3)
      at Function.throws (node:assert:986:3)
      at Object.<anonymous> (C:\workspace\node-test-binary-windows-js-suites\node\test\parallel\test-fs-rmSync-special-char.js:37:8)
      at Module._compile (node:internal/modules/cjs/loader:1738:14)
      at Object..js (node:internal/modules/cjs/loader:1903:10)
      at Module.load (node:internal/modules/cjs/loader:1473:32)
      at Function._load (node:internal/modules/cjs/loader:1285:12)
      at TracingChannel.traceSync (node:diagnostics_channel:322:14) {
    generatedMessage: false,
    code: 'ERR_ASSERTION',
    actual: false,
    expected: true,
    operator: '=='
  }

  Node.js v24.0.0-pre
...

It looks like the CI is failing on Windows:

It seems to fail on the additional test that is being added.

@Yeaseen Did you test it under Windows?

@lemire No. I will be trying this on Windows. I thought it was a simple fix.

@Yeaseen

I thought it was a simple fix.

Famous last words. 😄

@joyeecheung I think I figured out the main problem: it was in src/api/exceptions.cc file. I tested both on Windows 11 and Ubuntu 22.04 There's some conflict, making the last few commits messy. Sorry about that.

@lemire could you please review this and start a CI? I tested this locally, though. Thank you.

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

There's a conflict because some other code already modified what I worked on. I am gonna do this task on a fresh PR