test: fix embedding test for Windows

[vmoroz]

PR #52905 had added a new test case for embedding test which was failing for Windows.
The reason was that the expected error message has the Posix-specific line ending \n while on Windows the output string contains \r\n:

stderr: `${binary}: NODE_REPL_EXTERNAL_MODULE can't be used with kDisableNodeOptionsEnv\n`,

PR #52905 temporary disabled this new test case for Windows.

This PR removes the temporary test disablement for Windows and implements the fix by replacing \n in the test with os.EOL. After this change the embedding test is passing for Windows.

[joyeecheung]

LGTM although I think it's fine to simply use a looser regexp in test-embedding.js and just match

stderr: /NODE_REPL_EXTERNAL_MODULE can't be used with kDisableNodeOptionsEnv/

[vmoroz]

LGTM although I think it's fine to simply use a looser regexp in test-embedding.js and just match

stderr: /NODE_REPL_EXTERNAL_MODULE can't be used with kDisableNodeOptionsEnv/

Thanks! I had considered it initially, but decided on the str.replaceAll solution mostly to avoid similar issues in future. Developers who do not have access to Windows machines can use the \n in any other tests and not to be blocked anymore.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[aduh95]

Consistent failure on Windows:

---
duration_ms: 599.985
exitcode: 1
severity: fail
stack: |-
  Testing with --turbo-fast-api-calls: freopen
  [process 7672]: --- stderr ---
  (node:7672) ExperimentalWarning: WASI is an experimental feature and might change at any time
  (Use `node --trace-warnings ...` to show where the warning was created)
  fd_prestat_get(3, 70936)
  fd_prestat_dir_name(3, 70960, 8)
  fd_prestat_get(4, 70936)
  fd_prestat_dir_name(4, 70960, 4)
  fd_prestat_get(5, 70936)
  fd_fdstat_get(3, 70872)
  path_open(3, 1, 1062, 9, 0, 264240830, 268435455, 0, 70868)
  fd_fdstat_get(3, 70856)
  path_open(3, 1, 1081, 10, 0, 264240830, 268435455, 0, 70852)
  fd_renumber(6, 5)
  fd_close(4294967295)
  fd_read(5, 70872, 1, 70868)
  fd_fdstat_get(1, 70872)
  fd_write(1, 70880, 2, 70876)
  fd_read(5, 70872, 1, 70868)

  [process 7672]: --- stdout ---
  hello from input2.txt

  [process 7672]: status = 0, signal = null
  C:\workspace\node-test-binary-windows-js-suites\node\test\common\child_process.js:90
      throw new Error(`${failures.join('\n')}`);
      ^

  Error: - stdout did not match 'hello from input2.txt\r\n'
      at logAndThrow (C:\workspace\node-test-binary-windows-js-suites\node\test\common\child_process.js:90:11)
      at expectSyncExit (C:\workspace\node-test-binary-windows-js-suites\node\test\common\child_process.js:113:5)
      at spawnSyncAndAssert (C:\workspace\node-test-binary-windows-js-suites\node\test\common\child_process.js:135:10)
      at testWasiPreview1 (C:\workspace\node-test-binary-windows-js-suites\node\test\common\wasi.js:18:3)
      at Object.<anonymous> (C:\workspace\node-test-binary-windows-js-suites\node\test\wasi\test-wasi-io.js:6:1)
      at Module._compile (node:internal/modules/cjs/loader:1505:14)
      at Module._extensions..js (node:internal/modules/cjs/loader:1589:10)
      at Module.load (node:internal/modules/cjs/loader:1283:32)
      at Module._load (node:internal/modules/cjs/loader:1099:12)
      at TracingChannel.traceSync (node:diagnostics_channel:315:14)

  Node.js v23.0.0-pre
...

[vmoroz]

@aduh95 , yes, the CI failure is caused by this PR. The PR normalizes the output to use only the \n line ends while one of the tests does the comparison adjusted for the platform. I am going to fix this issue soon.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.09%. Comparing base (37f9eca) to head (d8314e9).
Report is 165 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #53659   +/-   ##
=======================================
  Coverage   87.09%   87.09%           
=======================================
  Files         647      647           
  Lines      181817   181843   +26     
  Branches    34883    34887    +4     
=======================================
+ Hits       158348   158382   +34     
+ Misses      16764    16763    -1     
+ Partials     6705     6698    -7     

see 29 files with indirect coverage changes

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[vmoroz]

None of the failures seems to be related to this PR.
I wonder if we have any PR that can cleanly pass all the CI checks in the last few weeks.
They either fail because of flaky tests or CI machine runs out of disk space (on Macs).
@mhdawson , @legendecas , could you help with merging this PR?

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[legendecas]

@vmoroz I noticed that you may pushed "rebuild" button on jenkins. An alternative to "rebuild" is "resume" which does not re-run successful sub-tasks. This can greatly help with the churn of flakiness that are unrelated to this PR.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 0b3ae01