test,doc: enable running embedtest for Windows #52646

vmoroz · 2024-04-22T17:29:50Z

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.
node.gyp is changed to add tools include directory for the embedtest project. It is need to include the executable_wrapper.h.
Make sure that we read and write snapshot files as binary files. Windows uses text mode by default.
Run embedtest from vcbuild.bat
Fix small typo in embedding.md file.

nodejs-github-bot · 2024-04-22T17:29:55Z

Review requested:

@nodejs/gyp

nodejs-github-bot · 2024-04-23T05:43:39Z

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 · 2024-04-23T17:21:10Z

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 · 2024-04-23T18:13:15Z

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 · 2024-04-23T21:02:41Z

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 · 2024-04-24T21:21:01Z

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 · 2024-04-24T21:42:15Z

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

joyeecheung · 2024-04-25T03:38:29Z

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 · 2024-04-25T05:07:46Z

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

nodejs-github-bot · 2024-04-29T22:13:03Z

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

mhdawson

LGTM

nodejs-github-bot · 2024-05-01T21:56:51Z

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

nodejs-github-bot · 2024-05-02T13:38:13Z

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

nodejs-github-bot · 2024-05-03T15:47:20Z

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

vmoroz · 2024-05-04T17:55:54Z

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

nodejs-github-bot · 2024-05-23T07:38:01Z

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

StefanStojanovic · 2024-05-23T07:51:55Z

@StefanStojanovic , it seems that the VS2022 build for ARM64 fails because of insufficient hard drive space. Is there a way to increase it?

This happens sometimes. I'm investigating ARM64 cross-compilation issues in CI and will hopefully have a more permanent solution soon.

nodejs-github-bot · 2024-05-23T17:34:47Z

Commit Queue failed

- Loading data for nodejs/node/pull/52646 ✔ Done loading data for nodejs/node/pull/52646 ----------------------------------- PR info ------------------------------------ Title test,doc: enable running embedtest for Windows (#52646) Author Vladimir Morozov (@vmoroz) Branch vmoroz:pr/enable_embedtest_for_windows -> nodejs:main Labels windows, build, needs-ci Commits 2 - test,doc: enable running embedtest for Windows - use existing executable_wrapper.h Committers 1 - Vladimir Morozov PR-URL: https://github.com/nodejs/node/pull/52646 Reviewed-By: Joyee Cheung Reviewed-By: Michael Dawson Reviewed-By: James M Snell ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/52646 Reviewed-By: Joyee Cheung Reviewed-By: Michael Dawson Reviewed-By: James M Snell -------------------------------------------------------------------------------- ℹ This PR was created on Mon, 22 Apr 2024 17:29:50 GMT ✔ Approvals: 3 ✔ - Joyee Cheung (@joyeecheung) (TSC): https://github.com/nodejs/node/pull/52646#pullrequestreview-2069697036 ✔ - Michael Dawson (@mhdawson) (TSC): https://github.com/nodejs/node/pull/52646#pullrequestreview-2071969985 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/52646#pullrequestreview-2039461466 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2024-05-23T07:38:01Z: https://ci.nodejs.org/job/node-test-pull-request/59361/ - Querying data for job/node-test-pull-request/59361/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 52646 From https://github.com/nodejs/node * branch refs/pull/52646/merge -> FETCH_HEAD ✔ Fetched commits as c137d6eb3101..91b5225d7946 -------------------------------------------------------------------------------- [main c9fde46b15] test,doc: enable running embedtest for Windows Author: Vladimir Morozov Date: Mon Apr 22 10:12:59 2024 -0700 7 files changed, 82 insertions(+), 4 deletions(-) create mode 100644 test/embedding/utf8_args.c create mode 100644 test/embedding/utf8_args.h [main 0b310539dd] use existing executable_wrapper.h Author: Vladimir Morozov Date: Tue Apr 23 14:30:34 2024 -0700 5 files changed, 5 insertions(+), 76 deletions(-) delete mode 100644 test/embedding/utf8_args.c delete mode 100644 test/embedding/utf8_args.h ✔ Patches applied There are 2 commits in the PR. Attempting autorebase. Rebasing (2/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test,doc: enable running embedtest for Windows

PR-URL: #52646
Reviewed-By: Joyee Cheung [email protected]
Reviewed-By: Michael Dawson [email protected]
Reviewed-By: James M Snell [email protected]

[detached HEAD bfaf50e60f] test,doc: enable running embedtest for Windows
Author: Vladimir Morozov [email protected]
Date: Mon Apr 22 10:12:59 2024 -0700
7 files changed, 82 insertions(+), 4 deletions(-)
create mode 100644 test/embedding/utf8_args.c
create mode 100644 test/embedding/utf8_args.h
Rebasing (3/4)
Rebasing (4/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
use existing executable_wrapper.h

PR-URL: #52646
Reviewed-By: Joyee Cheung [email protected]
Reviewed-By: Michael Dawson [email protected]
Reviewed-By: James M Snell [email protected]

[detached HEAD 455cd1577d] use existing executable_wrapper.h
Author: Vladimir Morozov [email protected]
Date: Tue Apr 23 14:30:34 2024 -0700
5 files changed, 5 insertions(+), 76 deletions(-)
delete mode 100644 test/embedding/utf8_args.c
delete mode 100644 test/embedding/utf8_args.h

Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

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

nodejs-github-bot · 2024-05-23T18:33:41Z

Landed in 98a1ecf

2 PRs that landed independently caused this issue which makes every native suites run in CI fail on Windows. This is just a quick patch to unblock the CI. Refs: nodejs#52905 Refs: nodejs#52646

2 PRs that landed independently caused this issue which makes every native suites run in CI fail on Windows. This is just a quick patch to unblock the CI. Refs: #52905 Refs: #52646 PR-URL: #53173 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>

PR-URL: #52646 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>

2 PRs that landed independently caused this issue which makes every native suites run in CI fail on Windows. This is just a quick patch to unblock the CI. Refs: #52905 Refs: #52646 PR-URL: #53173 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>

PR-URL: nodejs#52646 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>

2 PRs that landed independently caused this issue which makes every native suites run in CI fail on Windows. This is just a quick patch to unblock the CI. Refs: nodejs#52905 Refs: nodejs#52646 PR-URL: nodejs#53173 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>

PR-URL: nodejs#52646 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>

2 PRs that landed independently caused this issue which makes every native suites run in CI fail on Windows. This is just a quick patch to unblock the CI. Refs: nodejs#52905 Refs: nodejs#52646 PR-URL: nodejs#53173 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>

PR-URL: #52646 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>

2 PRs that landed independently caused this issue which makes every native suites run in CI fail on Windows. This is just a quick patch to unblock the CI. Refs: #52905 Refs: #52646 PR-URL: #53173 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>

PR-URL: #52646 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>

2 PRs that landed independently caused this issue which makes every native suites run in CI fail on Windows. This is just a quick patch to unblock the CI. Refs: #52905 Refs: #52646 PR-URL: #53173 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>

vmoroz requested a review from joyeecheung April 22, 2024 17:29

nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. windows Issues and PRs related to the Windows platform. labels Apr 22, 2024

lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 23, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 23, 2024

joyeecheung reviewed Apr 23, 2024

View reviewed changes

vmoroz marked this pull request as draft April 23, 2024 21:12

vmoroz marked this pull request as ready for review April 23, 2024 21:30

joyeecheung approved these changes Apr 25, 2024

View reviewed changes

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 25, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 25, 2024

vmoroz added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 29, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 29, 2024

mhdawson approved these changes May 1, 2024

View reviewed changes

jasnell approved these changes May 4, 2024

View reviewed changes

vmoroz added the commit-queue Add this label to land a pull request using GitHub Actions. label May 23, 2024

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels May 23, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 23, 2024

nodejs-github-bot merged commit 98a1ecf into nodejs:main May 23, 2024
86 checks passed

vmoroz deleted the pr/enable_embedtest_for_windows branch May 24, 2024 13:44

zcbenz mentioned this pull request May 25, 2024

build: fix building embedtest in GN build #53145

Closed

StefanStojanovic mentioned this pull request May 27, 2024

test: fix Windows native test suites #53173

Merged

targos pushed a commit that referenced this pull request Jun 1, 2024

test,doc: enable running embedtest for Windows

c61f909

PR-URL: #52646 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>

RafaelGSS mentioned this pull request Jun 7, 2024

v22.3.0 proposal #53379

Merged

marco-ippolito pushed a commit that referenced this pull request Jul 19, 2024

test,doc: enable running embedtest for Windows

54183a1

PR-URL: #52646 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>

marco-ippolito pushed a commit that referenced this pull request Jul 19, 2024

test,doc: enable running embedtest for Windows

dfa4986

PR-URL: #52646 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>

marco-ippolito mentioned this pull request Jul 19, 2024

v20.16.0 proposal #53945

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test,doc: enable running embedtest for Windows #52646

test,doc: enable running embedtest for Windows #52646

vmoroz commented Apr 22, 2024 •

edited

Loading

nodejs-github-bot commented Apr 22, 2024

nodejs-github-bot commented Apr 23, 2024

joyeecheung left a comment •

edited

Loading

vmoroz commented Apr 23, 2024

joyeecheung commented Apr 23, 2024

vmoroz commented Apr 23, 2024

vmoroz commented Apr 24, 2024

mhdawson commented Apr 24, 2024

joyeecheung commented Apr 25, 2024

nodejs-github-bot commented Apr 25, 2024

nodejs-github-bot commented Apr 29, 2024

mhdawson left a comment

nodejs-github-bot commented May 1, 2024

nodejs-github-bot commented May 2, 2024

nodejs-github-bot commented May 3, 2024

vmoroz commented May 4, 2024

nodejs-github-bot commented May 23, 2024

StefanStojanovic commented May 23, 2024

nodejs-github-bot commented May 23, 2024

PR-URL: #52646
Reviewed-By: Joyee Cheung [email protected]
Reviewed-By: Michael Dawson [email protected]
Reviewed-By: James M Snell [email protected]

PR-URL: #52646
Reviewed-By: Joyee Cheung [email protected]
Reviewed-By: Michael Dawson [email protected]
Reviewed-By: James M Snell [email protected]

Successfully rebased and updated refs/heads/main.

nodejs-github-bot commented May 23, 2024

test,doc: enable running embedtest for Windows #52646

test,doc: enable running embedtest for Windows #52646

Conversation

vmoroz commented Apr 22, 2024 • edited Loading

nodejs-github-bot commented Apr 22, 2024

nodejs-github-bot commented Apr 23, 2024

joyeecheung left a comment • edited Loading

Choose a reason for hiding this comment

vmoroz commented Apr 23, 2024

joyeecheung commented Apr 23, 2024

vmoroz commented Apr 23, 2024

vmoroz commented Apr 24, 2024

mhdawson commented Apr 24, 2024

joyeecheung commented Apr 25, 2024

nodejs-github-bot commented Apr 25, 2024

nodejs-github-bot commented Apr 29, 2024

mhdawson left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented May 1, 2024

nodejs-github-bot commented May 2, 2024

nodejs-github-bot commented May 3, 2024

vmoroz commented May 4, 2024

nodejs-github-bot commented May 23, 2024

StefanStojanovic commented May 23, 2024

nodejs-github-bot commented May 23, 2024

PR-URL: #52646 Reviewed-By: Joyee Cheung [email protected] Reviewed-By: Michael Dawson [email protected] Reviewed-By: James M Snell [email protected]

PR-URL: #52646 Reviewed-By: Joyee Cheung [email protected] Reviewed-By: Michael Dawson [email protected] Reviewed-By: James M Snell [email protected]

Successfully rebased and updated refs/heads/main.

nodejs-github-bot commented May 23, 2024

vmoroz commented Apr 22, 2024 •

edited

Loading

joyeecheung left a comment •

edited

Loading

PR-URL: #52646
Reviewed-By: Joyee Cheung [email protected]
Reviewed-By: Michael Dawson [email protected]
Reviewed-By: James M Snell [email protected]

PR-URL: #52646
Reviewed-By: Joyee Cheung [email protected]
Reviewed-By: Michael Dawson [email protected]
Reviewed-By: James M Snell [email protected]