-
Notifications
You must be signed in to change notification settings - Fork 36
Use latest node version to run wasm based tests #400
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #400 +/- ##
==========================================
- Coverage 81.78% 81.77% -0.02%
==========================================
Files 20 20
Lines 950 845 -105
Branches 87 87
==========================================
- Hits 777 691 -86
+ Misses 173 154 -19 see 14 files with indirect coverage changes 🚀 New features to boost your workflow:
|
| - name: Setup latest Node.js | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 'latest' |
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.
If the user needs to use the latest node for the Emscripten tests to pass in the referenced PR, then this would needs to be mentioned in the readme. The user may not have the latest node installed, and executing node will get emsdks downloaded version. We should be using the node version which comes with emsdk to be consistent. When we move to a newer emsdk we get a very recent node. Maybe for now we need something in the readme about emsdk install node-24.7.0-64bit
|
Okay I am just confused here. The test C++ emscripten job shows the following at the 40 second mark itself. I'm not sure what is it waiting for there after. I added the exit runtime flag to force the exit once main runs but that didn't help The emscripten_wasm (osx15-arm, macos-15) jobs times out not doing anything after showing success and just runs till the 4 minute mark. Even other jobs like deploy run for 4 mins even though the tests have already been executed. I don't expect that to happen through node (atleast doesn't happen locally ... I always build the .js and not the .html through emrun) @mcbarton as you introduced the testing in the browser thingy, do you know what's happening here ? The native kernel tests pass under a minute, I see no reason why the wasm kernel tests should take more than a minute in that case ! |
As far as I can tell it is running the browser tests fine. Its when it uses the node where it stalls (put an echo command such as 'running Emscripten C++ tests in node' just before the node command). This stalling when trying to run the tests in node has happens to me locally when it tries to use emsdk 3.1.73 old version of node. I think there is a strong possibility it is not using the node 24 you install (maybe try a node --version command after you do the echo command). The Emscripten C++ tests can run with the old node emsdk currently uses, but it takes a long time (on the ci runners you have to remove the timeout and it takes about 20 minutes before it starts running the C++ tests in node). With the latest node it runs instantly for me. |
|
@anutosh491 To see how long it takes for the Emscripten C++ tests to run with emsdk 3.1.73 version of node see this PR I have open #386 . This should all be fixed when we upgrade to emsdk version 4 |
Hmm, I still don't understand if you're proposing something to be done here ? Do you have a current way ahead ? |
If you see the new ci output it prints node version v16.20.2 . What is happening is your installing a new node version, but with the changes you've made to the ci its now using the version which comes with emsdk version 3.1.73 which is quite old. Instead of using the action, get emsdk 3.1.73 in Emscripten forge to install node 24 using emsdk install node-24.7.0-64bit and it should be be able to run within the 4 minute timeout. Otherwise remove the time limit and the tests will run in that old version eventually. |
|
Not sure that works for me locally |
This is working on my Macbook, so unsure why your Macbook doesn't accept it |
|
Weird, would you like to take over ? The goal here was to upgrade the node version cause node 20 gives a memory out of bounds error for #398 , whereas there is nothing wrong with the change/test added there. Upgrading to 23/24 should fix that ! |
Description
Refer #398 (comment)
Type of change
Please tick all options which are relevant.