-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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.
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
Conversation
|
Test failures seem to be flakes. Running this PR in http://localhost:8010/#/builders/11/builds/287 does seem to work as expected. |
| # EM_CONFIG stuff | ||
| if not JS_ENGINES: | ||
| JS_ENGINES = [NODE_JS] | ||
| JS_ENGINES = [NODE_JS_TEST if NODE_JS_TEST else NODE_JS] |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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]
There was a problem hiding this comment.
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]
sbc100
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, I'll try to move this code into test files later.
Emsdk generates
when activating regular and big endian node. This results in a non-working Emscripten installation that complains about NODE_JS not found.
Comment at #25063 (comment) suggests that the intent is that if NODE_JS_TEST is populated, it should go to JS_ENGINES, so make that happen.