test,doc: enable running embedtest for Windows

[vmoroz]

Currently the embedtest does not run on Windows.
One of the main reasons is that the Windows command line does not accept UTF-8 characters required by the test.

In this PR we enable embedtest to run on Windows:

• Use node::FixupMain from tools/executable_wrapper.h to fix argc and argv values. The NODE_MAIN macro is defined as wmain for Windows targets that receives parameters as UTF-16. Then, the node::FixupMain converts them to UTF-8.
• Make sure that we read and write snapshot files as binary files. Windows uses text mode by default.
• Add calling embedtest to vcbuild.bat as a part of cctest call. It is currently implemented the same way in the Makefile.
• Makefile is changed to add tools include directory for the embedtest project. It is need to include the executable_wrapper.h.
• Fix small typo in embedding.md file.

[nodejs-github-bot]

Review requested:

• @nodejs/gyp

[nodejs-github-bot]

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

[joyeecheung]

We already have tools/executable_wrapper.h and AFAICT it serves the same purpose. Not sure why this wasn't done for the embedtest although that wrapper was introduced later when we do need to create executable that also works on Windows (js2c). Can we just use that for the embedtest?

[vmoroz]

We already have tools/executable_wrapper.h and AFAICT it serves the same purpose. Not sure why this wasn't done for the embedtest although that wrapper was introduced later when we do need to create executable that also works on Windows (js2c). Can we just use that for the embedtest?

Great! It could work.

For the PR #43542 I will need to make tools/executable_wrapper.h compatible with C.
It was the reason why the new APIs were done to match C calling conventions.
Since tools/executable_wrapper.h is header-only it must be trivial to do as part of PR #43542.

[joyeecheung]

IIUC to share code with #43542 ultimately we need to expose a helper like node::FixupMain to node.h. That sounds like a good idea, because the UTF8 convention is not currently surfaced to node.h, and we can also use it to remove the repeated code in src/node_main.cc. Not sure about how useful/appropriate it is for the C API, I'll defer that to the n-api team.

[vmoroz]

It seems that we can avoid converting the node::FixupMain to C.
#43542 adds two new executables to the embedtest directory.
The simplest approach would be to add testmain.cc that is reused across all three executables.
It will be a C++ file and it will use the node::FixupMain. It then will call TestMain function for each test executable.
Anyway, the change will be implemented in the #43542.
This PR will be greatly simplified by using the node::FixupMain.

As for the src/node_main.cc, I see that it has a copy of tools/executable_wrapper.h code.
I guess we include tools/executable_wrapper.h to src/node_main.ccto reuse it.
I can create a separate PR for it.

[vmoroz]

It seems that macOS benchmark is failing, but the failure is not related to changes in this PR.
I guess something else makes the test to break.

[mhdawson]

I think that is one of the known flaky tests, restarted the github action on macOS

[joyeecheung]

I guess we include tools/executable_wrapper.h to src/node_main.cc to reuse it.

I think the reason why we don't do that is, we generally try to keep src/node_main.cc rely only on node.h and not any internal headers (I am not sure if that's just my impression at this point, or is that something really enforced). But tackling that in a different PR SGTM.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[mhdawson]

LGTM

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[vmoroz]

It seem that CI shows that the new embedding testing is failing on Windows and it is related to this PR. I am going to investigate/fix it. I wonder if the child.stderr is changed last week.

10:18:57 > echo running 'Release\node.exe test\embedding\test-embedding.js' 
10:18:57 running 'Release\node.exe test\embedding\test-embedding.js'
10:18:57 
10:18:57 > "Release\node.exe" test\embedding\test-embedding.js 
10:18:57 [process 0]: --- stderr ---
10:18:57 c:\workspace\node-test-binary-windows-native-suites\node\test\common\child_process.js:82
10:18:57     console.error(stderrStr === undefined ? child.stderr.toString() : stderrStr);
10:18:57                                                          ^
10:18:57 
10:18:57 TypeError: Cannot read properties of null (reading 'toString')
10:18:57     at logAndThrow (c:\workspace\node-test-binary-windows-native-suites\node\test\common\child_process.js:82:58)
10:18:57     at expectSyncExit (c:\workspace\node-test-binary-windows-native-suites\node\test\common\child_process.js:91:5)
10:18:57     at spawnSyncAndAssert (c:\workspace\node-test-binary-windows-native-suites\node\test\common\child_process.js:131:10)
10:18:57     at Object.<anonymous> (c:\workspace\node-test-binary-windows-native-suites\node\test\embedding\test-embedding.js:27:1)
10:18:57     at Module._compile (node:internal/modules/cjs/loader:1480:14)
10:18:57     at Module._extensions..js (node:internal/modules/cjs/loader:1564:10)
10:18:57     at Module.load (node:internal/modules/cjs/loader:1287:32)
10:18:57     at Module._load (node:internal/modules/cjs/loader:1103:12)
10:18:57     at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:168:12)
10:18:57     at node:internal/main/run_main_module:30:49

[github-actions]

Failed to start CI

   ⚠  Something was pushed to the Pull Request branch since the last approving review.
   ✘  Refusing to run CI on potentially unsafe PR

https://github.com/nodejs/node/actions/runs/8958495093

[vmoroz]

I had to rebase the PR since the test break on Windows seemed to be caused by PR being submitted against Node.js version 22 while now we have the version 23.

[mhdawson]

LGTM

[nodejs-github-bot]

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