-
-
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
Changes from all commits
650c4bd
15e008e
f91c6e0
f963b40
84dfe03
ebf0a38
6113b0b
0f8ad4b
4e9a8a3
dfc7f80
328abbd
27dab3d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,7 @@ Commands: | |
fmt format code, fail if bad | ||
test run unit tests (no Godot needed) | ||
itest run integration tests (from within Godot) | ||
testweb run unit tests on web environment (requires node.js and emcc) | ||
clippy validate clippy lints | ||
klippy validate + fix clippy | ||
doc generate docs for 'godot' crate | ||
|
@@ -179,6 +180,38 @@ function cmd_itest() { | |
run "$godotBin" $GODOT_ARGS --path itest/godot --headless -- "[${extraArgs[@]}]" | ||
} | ||
|
||
function cmd_testweb() { | ||
# Add more debug symbols to build | ||
common_flags="-C link-args=-g" | ||
|
||
# Avoid problems with emcc potentially writing to read-only dir | ||
cache_dir="$(realpath ./target)/emscripten_cache" | ||
mkdir -p "${cache_dir}" | ||
|
||
echo "===============================" | ||
echo "Initiating threaded Wasm tests." | ||
echo "===============================" | ||
|
||
# For the runner env var: https://github.com/rust-lang/cargo/issues/7471 | ||
# More memory (256 MiB) is needed for the parallel godot-cell tests which | ||
# spawn 70 threads each. | ||
Comment on lines
+196
to
+197
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. |
||
CARGO_TARGET_WASM32_UNKNOWN_EMSCRIPTEN_RUNNER=node RUSTFLAGS="-C link-args=-pthread \ | ||
-C target-feature=+atomics \ | ||
-C link-args=-sINITIAL_MEMORY=268435456 \ | ||
${common_flags}" EM_CACHE="${cache_dir}" run cargo +nightly test \ | ||
--features godot/experimental-wasm,godot/lazy-function-tables \ | ||
-Zbuild-std --target wasm32-unknown-emscripten | ||
|
||
echo "===================================" | ||
echo "Initiating non-threaded Wasm tests." | ||
echo "===================================" | ||
|
||
CARGO_TARGET_WASM32_UNKNOWN_EMSCRIPTEN_RUNNER=node RUSTFLAGS="${common_flags}" \ | ||
EM_CACHE="${cache_dir}" run cargo +nightly test \ | ||
--features godot/experimental-wasm-nothreads,godot/experimental-wasm,godot/lazy-function-tables \ | ||
-Zbuild-std --target wasm32-unknown-emscripten | ||
} | ||
|
||
function cmd_doc() { | ||
run cargo doc --lib -p godot --no-deps "${extraCargoArgs[@]}" | ||
} | ||
|
@@ -211,7 +244,7 @@ while [[ $# -gt 0 ]]; do | |
--double) | ||
extraCargoArgs+=("--features" "godot/double-precision,godot/api-custom") | ||
;; | ||
fmt | test | itest | clippy | klippy | doc | dok) | ||
fmt | test | itest | testweb | clippy | klippy | doc | dok) | ||
cmds+=("$arg") | ||
;; | ||
-f | --filter) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,6 +40,10 @@ impl MyClass { | |
/// | ||
/// This should not cause borrow failures and should not lead to deadlocks. | ||
#[test] | ||
#[cfg_attr( | ||
all(target_family = "wasm", not(target_feature = "atomics")), | ||
ignore = "Threading not available" | ||
)] | ||
Comment on lines
+43
to
+46
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Oh, good idea! |
||
fn calls_parallel() { | ||
use std::thread; | ||
|
||
|
@@ -72,6 +76,10 @@ fn calls_parallel() { | |
/// Runs each method several times in a row. This should reduce the non-determinism that comes from | ||
/// scheduling of threads. | ||
#[test] | ||
#[cfg_attr( | ||
all(target_family = "wasm", not(target_feature = "atomics")), | ||
ignore = "Threading not available" | ||
)] | ||
fn calls_parallel_many_serial() { | ||
use std::thread; | ||
|
||
|
@@ -106,6 +114,10 @@ fn calls_parallel_many_serial() { | |
/// Runs all the tests several times. This is different from [`calls_parallel_many_serial`] as that calls the | ||
/// methods like AAA...BBB...CCC..., whereas this interleaves the methods like ABC...ABC...ABC... | ||
#[test] | ||
#[cfg_attr( | ||
all(target_family = "wasm", not(target_feature = "atomics")), | ||
ignore = "Threading not available" | ||
)] | ||
fn calls_parallel_many_parallel() { | ||
use std::thread; | ||
|
||
|
@@ -142,6 +154,10 @@ fn calls_parallel_many_parallel() { | |
/// a) Thread A holds mutable reference AND thread B holds no references. | ||
/// b) One or more threads hold shared references AND thread A holds no references | ||
#[test] | ||
#[cfg_attr( | ||
all(target_family = "wasm", not(target_feature = "atomics")), | ||
ignore = "Threading not available" | ||
)] | ||
fn non_blocking_reborrow() { | ||
use std::thread; | ||
let instance_id = MyClass::init(); | ||
|
@@ -172,6 +188,10 @@ fn non_blocking_reborrow() { | |
/// This verifies that the thread which initialized the `GdCell` does not panic when it attempts to mutably borrow while there is already a | ||
/// shared borrow on an other thread. | ||
#[test] | ||
#[cfg_attr( | ||
all(target_family = "wasm", not(target_feature = "atomics")), | ||
ignore = "Threading not available" | ||
)] | ||
fn no_mut_panic_on_main() { | ||
use std::thread; | ||
let instance_id = MyClass::init(); | ||
|
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 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.