Run tests w/ fallback on wasm32-unknown-unknown #1114
Conversation
With this change, CI can actually run tests against this target which should help catch any bugs early. This is a bit invasive, because wasm32-unknown-unknown requires a custom `#[wasm_bindgen_test]` attributes as well as a custom runner instead of the standard `#[test]`, but there is no way around it, and, having those changes only at the top of test files, I believe it's reasonably isolated.
There was a problem hiding this comment.
This is a bit invasive, because this target requires a custom
#[wasm_bindgen_test]attributes as well as a custom runner instead of the standard#[test], but there is no way around it, and, having those changes only at the top of test files, I believe it's reasonably isolated.
Is this needed for src/*/test.rs modules too?
| targets: wasm32-unknown-unknown | ||
| - run: cargo build --verbose --target wasm32-unknown-unknown ${{ matrix.cargoflags }} | ||
| - uses: jetli/[email protected] |
There was a problem hiding this comment.
I'm wary of adding 3rd-party github actions -- can we just use the official installer like I did for wasmtime below?
There was a problem hiding this comment.
This is a reasonably popular action, but, sure, we can use official installer too, especially since wasm-pack provides prebuilt binaries and doesn't need recompilation from source.
| targets: wasm32-unknown-unknown | ||
| - run: cargo build --verbose --target wasm32-unknown-unknown ${{ matrix.cargoflags }} | ||
| - uses: jetli/[email protected] | ||
| - run: wasm-pack test --chrome --headless -- --verbose ${{ matrix.cargoflags }} |
There was a problem hiding this comment.
Should we also test rayon-core?
|
BTW, you should be able to copy the action to |
I tested it on my branch by just changing |
I didn't even see those but yeah :( This is a bit annoying... |
|
I guess I mainly opened the PR to gather feedback first anyway. On one hand, it's a more invasive change than I'd like (and clearly needs to affect even more places), but, on another, it's potentially useful as it catches more errors. Do you think it's worth pursuing further? |
|
Maybe it's better to have a more targeted test? Just enough to exercise the |
This test doesn't really verify web_spin_lock - for that I have a small targeted test in wasm-bindgen-rayon. Instead, it verifies the non-threaded fallback behaviour in the browser, which was previously built but never executed. |
|
I guess given that it's already tested by wasm32-wasi, it indeed shouldn't be any different barring bugs in Wasm engines (but then, it's outside of scope of Rayon anyway). I'll probably close this for now. |
|
Right, I'm happy with wasi for fallback testing. I thought you were trying to run real wasm threads given the |
Unfortunately not, as I wrote in that comment it would still require wasm-bindgen-rayon in addition to those flags. |
|
Oh, indeed you did write that -- reading is hard sometimes. 🙂 |
With this change, CI can actually run tests against wasm32-unknown-unknown with the fallback non-threaded implementation, which should help catch any bugs early.
This is a bit invasive, because this target requires a custom
#[wasm_bindgen_test]attributes as well as a custom runner instead of the standard#[test], but there is no way around it, and, having those changes only at the top of test files, I believe it's reasonably isolated.