Skip to content

Fix JS engines to use NODE_JS_TEST #25074

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
juj merged 8 commits into from
Aug 27, 2025
Merged

Fix JS engines to use NODE_JS_TEST #25074

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2ee8959
Fix JS engines to use NODE_JS_TEST
juj Aug 27, 2025
d04c4c9
Update instructions to access qemu node
juj Aug 27, 2025
74fcade
dir
juj Aug 27, 2025
5681b22
Update instructions
juj Aug 27, 2025
455143f
Review
juj Aug 27, 2025
5b8bb5b
Restore ternary
juj Aug 27, 2025
5e2e449
Restore to original form
juj Aug 27, 2025
ae6e6a1
Merge remote-tracking branch 'origin/main' into node_js_test
juj Aug 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 11 additions & 19 deletions test/test_core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10041,26 +10041,18 @@ def setUp(self):
minimal0 = make_run('minimal0', cflags=['-g'], settings={'MINIMAL_RUNTIME': 1})
llvmlibc = make_run('llvmlibc', cflags=['-lllvmlibc'])

# To run the big endian test suite on a little endian Linux host:
# To run the big endian test suite (supported by emsdk on a little endian Linux host only):
# 1. sudo apt install -y qemu-user libc6-s390x-cross libstdc++6-s390x-cross
# 2. wget https://nodejs.org/dist/v22.16.0/node-v22.16.0-linux-s390x.tar.xz
# 3. tar xJf node-v22.14.0-linux-s390x.tar.xz
# 4. run a little endian emsdk install, e.g. `emsdk install sdk-main-64bit`
# 5. activate little endian emsdk install, e.g. `emsdk activate sdk-main-64bit`
# 6. modify the generated emsdk install: edit file .emscripten under emsdk/ root directory, and change the line
# NODE_JS = emsdk_path + '/node/22.16.0_64bit/bin/node'
# to point to big endian node:
# NODE_JS = ['qemu-s390x', '-L', '/usr/s390x-linux-gnu/', '/home/user/node-v22.16.0-linux-s390x/bin/node']
# 7. enter emsdk environment in current terminal with `source ./emsdk_env.sh`
# 8. modify EMSDK_NODE environment variable to also point to the big endian node:
# export EMSDK_NODE="qemu-s390x -L /usr/s390x-linux-gnu/ /home/user/node-v22.16.0-linux-s390x/bin/node"
# 9. run some tests in big endian mode: in Emscripten root directory, run
# `test/runner bigendian` to run all tests, or a single test with
# `test/runner bigendian.test_jslib_i64_params`

# The above test scheme has a small quirk that it will also use the Big Endian version of node.js for internal
# Emscripten compilation. At a quick test, this actually worked, so maybe this would be fine for this test harness.
# Alternatively, we would want to find a way to separate compiler node from runtime node somehow in the harness.
# 2. install emsdk with big-endian node: `git clone https://github.com/emscripten-core/emsdk.git`
# and `./emsdk install sdk-main-64bit node-big-endian-crosscompile-22.16.0-64bit`
# 3. activate emsdk tools: `./emsdk activate sdk-main-64bit node-big-endian-crosscompile-22.16.0-64bit`
# 4. enter emsdk environment in current terminal with `source ./emsdk_env.sh`
# 5. run some tests in big endian mode: `cd emscripten/main` to enter Emscripten root directory, and run
# `test/runner bigendian0` to run all tests, or a single test with
# `test/runner bigendian0.test_jslib_i64_params`

# This setup will still use the native x64 Node.js in Emscripten internal use to compile code, but
# runs all unit tests via qemu on the s390x big endian version of Node.js.
bigendian0 = make_run('bigendian0', cflags=['-O0'], settings={'SUPPORT_BIG_ENDIAN': 1})

# TestCoreBase is just a shape for the specific subclasses, we don't test it itself
Expand Down
2 changes: 1 addition & 1 deletion tools/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ def normalize_config_settings():

# EM_CONFIG stuff
if not JS_ENGINES:
JS_ENGINES = [NODE_JS]
JS_ENGINES = [NODE_JS_TEST if NODE_JS_TEST else NODE_JS]
Copy link
Collaborator

@sbc100 sbc100 Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move these 2 lines to test/common.py after the assignment to NODE_JS_TEST (then you don't need the embedded if/else here).

Copy link
Collaborator Author

@juj juj Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check

Copy link
Collaborator Author

@juj juj Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some kind of type analysis checker?

mypy step fails with

test/common.py:142: error: List item 0 has incompatible type "Optional[Any]"; expected "List[str]"  [list-item]
Found 1 error in 1 file (checked 130 source files)

Copy link
Collaborator Author

@juj juj Aug 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I did the change something wrong, though after moving the two lines, it seems the effect is as if I didn't have those lines at all, i.e. the run fails like it did before this PR altogether. (http://localhost:8010/api/v2/logs/33118/raw_inline)

Copy link
Collaborator Author

@juj juj Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, I still need the ternary, I misunderstood the move was intended to avoid that.

Copy link
Collaborator Author

@juj juj Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(then you don't need the embedded if/else here).

Hmm, it looks like I do, because in common.py, the check is

if not config.NODE_JS_TEST:
  config.NODE_JS_TEST = config.NODE_JS

but in emsdk case, config.NODE_JS_TEST is set, it has both. So this check doesn't do anything.

So it needs

if not config.JS_ENGINES:
  config.JS_ENGINES = [config.NODE_JS_TEST if config.NODE_JS_TEST else config.NODE_JS]

Copy link
Collaborator Author

@juj juj Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reverted back to the original form, that helped the mypy type check, that insisted thinking that config.NODE_JS is Optional[Any] and not List[Str]


SPIDERMONKEY_ENGINE = fix_js_engine(SPIDERMONKEY_ENGINE, listify(SPIDERMONKEY_ENGINE))
NODE_JS = fix_js_engine(NODE_JS, listify(NODE_JS))
Expand Down
Loading