Skip to content

Fix failing tests under node-canary. NFC #24075

New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom
Merged
merged 1 commit into from
Apr 9, 2025
Merged

Fix failing tests under node-canary. NFC #24075

merged 1 commit into from
Apr 9, 2025

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Apr 9, 2025

One change here is that new versions of node now support navigator.language/languages natively.

@sbc100
Fix failing tests under node-canary. NFC
5dfc75c 
One change here is that new versions of node now support
navigator.language/languages natively.
@sbc100 sbc100 requested review from kripken and dschuff April 9, 2025 00:05
dschuff
dschuff approved these changes Apr 9, 2025
View reviewed changes
test/test_other.py
@@ -13374,7 +13374,7 @@ def test_split_module(self, customLoader, jspi, opt):
self.emcc_args += ['-g', '-sJSPI_EXPORTS=say_hello']
self.emcc_args += ['-sEXPORTED_FUNCTIONS=_malloc,_free']
output = self.do_other_test('test_split_module.c')
if jspi:
Copy link
Member

Choose a reason for hiding this comment

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

Does node canary support JSPI yet? I guess maybe with a flag, and soon without? Or does it need changes in node in addition to its V8?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This condition here is not related to JSPI per sey, but more about the fact that d8 cannot write files out to disc.

The if jspi: condition was really trying to say if d8: and it worked since we only ever ran JSPI tests under d8 I guess. Running this test with node canary ended up in situation where were both using node and jspi which broke this check.

kripken
kripken reviewed Apr 9, 2025
View reviewed changes
test/test_other.py
@@ -13374,7 +13374,7 @@ def test_split_module(self, customLoader, jspi, opt):
self.emcc_args += ['-g', '-sJSPI_EXPORTS=say_hello']
self.emcc_args += ['-sEXPORTED_FUNCTIONS=_malloc,_free']
output = self.do_other_test('test_split_module.c')
if jspi:
if self.js_engines == [config.V8_ENGINE]:
# TODO remove this when https://chromium-review.googlesource.com/c/v8/v8/+/4159854
Copy link
Member

Choose a reason for hiding this comment

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

This TODO was landed - can we remove this if?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we would need to do some more work there to make it work.

@sbc100 sbc100 merged commit 0994edc into emscripten-core:main Apr 9, 2025
28 checks passed
@sbc100 sbc100 deleted the fix_node_canary branch April 9, 2025 17:21
@dschuff
Copy link
Member

dschuff commented Apr 9, 2025

This seems to be failing on MacOS: https://logs.chromium.org/logs/emscripten-releases/buildbucket/cr-buildbucket/8718030851337111473/+/u/Emscripten_testsuite__other_/stdout

env_modify: {'LC_ALL': 'en_US'}
Fatal Python error: config_get_locale_encoding: failed to get the locale encoding: nl_langinfo(CODESET) failed
Python runtime state: preinitialized

So not because of node, but because of Python.

@dschuff
Copy link
Member

dschuff commented Apr 10, 2025

Interestingly it doesn't reproduce for me on my mac. It must have something to do with the python on the Chromium builders.

sbc100 added a commit to sbc100/emscripten that referenced this pull request Apr 10, 2025
@sbc100
Avoid setting LC_ALL in test_browser_language_detection. NFC
1cd87a7 
Followup to emscripten-core#24075.

This should fix the mac failures we have been seeing
@sbc100 sbc100 mentioned this pull request Apr 10, 2025
Merged
sbc100 added a commit to sbc100/emscripten that referenced this pull request Apr 10, 2025
@sbc100
Avoid setting LC_ALL in test_browser_language_detection. NFC
aeb27df 
Followup to emscripten-core#24075.

This should fix the mac failures we have been seeing
sbc100 added a commit to sbc100/emscripten that referenced this pull request Apr 10, 2025
@sbc100
Avoid setting LC_ALL in test_browser_language_detection. NFC
40a3d66 
Followup to emscripten-core#24075.

This should fix the mac failures we have been seeing
@dschuff
Copy link
Member

dschuff commented Apr 10, 2025

Weird, is there even any code in this change that would cause Python to do anything different wrt getting locale encodings?

@sbc100
Copy link
Collaborator Author

sbc100 commented Apr 10, 2025

Weird, is there even any code in this change that would cause Python to do anything different wrt getting locale encodings?

I'm just no longer setting LC_ALL at all. Setting LC_ALL has an effect on all child processes including emcc/python.exe

dschuff pushed a commit that referenced this pull request Apr 10, 2025
@sbc100
Avoid setting LC_ALL in test_browser_language_detection. NFC (#24088)
06d1062 
Followup to #24075.

This should fix the mac failures we have been seeing
@dschuff
Copy link
Member

dschuff commented Apr 10, 2025

Ah, got it, thanks

# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@kripken kripken kripken left review comments

@dschuff dschuff dschuff approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sbc100 @dschuff @kripken