-
-
Notifications
You must be signed in to change notification settings - Fork 241
Wasm unit tests on CI #1275
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: master
Are you sure you want to change the base?
Wasm unit tests on CI #1275
Conversation
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1275 |
8f3e5bd
to
556bb1c
Compare
779c043
to
ddc5b76
Compare
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.
Thanks a lot!
I'll also rebase onto lastest master
, to fix a CI issue that has been merged in the meantime.
#[cfg_attr( | ||
all(target_family = "wasm", not(target_feature = "atomics")), | ||
ignore = "Threading not available" | ||
)] |
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.
Might be worth a module to not annotate each individual test? 🙂
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, good idea!
./emsdk/emsdk install $EMSCRIPTEN_VERSION | ||
./emsdk/emsdk activate $EMSCRIPTEN_VERSION | ||
source ./emsdk/emsdk_env.sh | ||
echo PATH=$PATH >> $GITHUB_ENV |
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.
echo PATH=$PATH >> $GITHUB_ENV | |
echo "PATH=$PATH" >> $GITHUB_ENV |
I don't expect spaces, but these are a real pain to debug 😬
But apart from that, this just defines it with itself?
Should we not use GITHUB_PATH
?
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.
Good point, I didn't know about that variable. We should probably be using it.
# More memory (256 MiB) is needed for the parallel godot-cell tests which | ||
# spawn 70 threads each. |
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.
Good observation. Do you think this still makes sense, pushing the limits also on web?
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 think it's fine for what it's worth. We could consider reducing the tests a bit, but I'd rather get existing tests to work first.
since it's only used for tests
no feature needed
this needs fixing in the book
this might call for another composite... second attempt at pinning emscripten
556bb1c
to
27dab3d
Compare
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.
Thanks for taking a look. I've been busy with other tasks lately, so sorry for the slow progress, but don't worry, we will get this through the finish line :)
./emsdk/emsdk install $EMSCRIPTEN_VERSION | ||
./emsdk/emsdk activate $EMSCRIPTEN_VERSION | ||
source ./emsdk/emsdk_env.sh | ||
echo PATH=$PATH >> $GITHUB_ENV |
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.
Good point, I didn't know about that variable. We should probably be using it.
#[cfg_attr( | ||
all(target_family = "wasm", not(target_feature = "atomics")), | ||
ignore = "Threading not available" | ||
)] |
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, good idea!
# More memory (256 MiB) is needed for the parallel godot-cell tests which | ||
# spawn 70 threads each. |
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 think it's fine for what it's worth. We could consider reducing the tests a bit, but I'd rather get existing tests to work first.
Runs wasm unit tests on CI. Browsers / exports not tested here yet.
Full details here https://typst.app/project/r314DoDea5XiYYr5bq5IFP
(I will summarize them here later)
P.S. I'll be organizing commits later, sorry for the mess :)
TODO:
test_global_would_block
test inglobal.rs
when testing Wasm nothreads