Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add wasm tests using was-bindgen-test and wasm-pack #346

Merged
merged 5 commits into from Dec 1, 2023

Conversation

lpotthast
Copy link
Contributor

A few notes:

  1. Wasm32 related tests cannot be run that easily, see Testing strategy for wasm32-unknown-unknown聽rustwasm/team#173. You have to use https://rustwasm.github.io/wasm-bindgen/wasm-bindgen-test/usage.html and wasm-pack.

  2. The criterion dev-dependency, only used for benchmarks, prohibits any compilation to the wasm32 target when using its default features (rayon in particular). This is now dependant on the targeted architecture.

  3. Examples could not be compiled to wasm32, or any 32 bit target. The Claims struct from both custom_hader.rs and validation.rs had a usize field which got initialized with a value outside the u32 (usize on wasm32) range. Switching to an explicit u64 type in these structs seemed to be a reasonable fix?

  4. Most crates provide some wasm-... feature, to enable wasm support. This crate uses [target.'cfg(target_arch = "wasm32")'.dependencies] inside its Cargo.toml. This is obviously easier for users, as there is no feature which could be forgotten. Are there any downsides to this method?

  5. CI will most likely still not work, as

Example wasm-pack output:

lukas@[...]:~/dev/jsonwebtoken$ wasm-pack test --node
[INFO]: 馃幆  Checking for the Wasm target...
   Compiling jsonwebtoken v9.1.0 (/home/lukas/dev/jsonwebtoken)
    Finished dev [unoptimized + debuginfo] target(s) in 0.84s
   Compiling jsonwebtoken v9.1.0 (/home/lukas/dev/jsonwebtoken)
    Finished test [unoptimized + debuginfo] target(s) in 0.28s
     Running unittests src/lib.rs (target/wasm32-unknown-unknown/debug/deps/jsonwebtoken-ae8fe4284f1b721f.wasm)
no tests to run!
     Running tests/hmac.rs (target/wasm32-unknown-unknown/debug/deps/hmac-14855738297bd30c.wasm)
no tests to run!
     Running tests/lib.rs (target/wasm32-unknown-unknown/debug/deps/lib-17a50cd144c5e104.wasm)
Set timeout to 20 seconds...
running 21 tests                                  

test lib::ecdsa::roundtrip_with_jwtio_example ... ok
test lib::ecdsa::ed_jwk ... ok
test lib::ecdsa::ec_x_y ... ok
test lib::ecdsa::round_trip_claim ... ok
test lib::ecdsa::round_trip_sign_verification_pem ... ok
test lib::ecdsa::round_trip_sign_verification_pk8 ... ok
test lib::rsa::roundtrip_with_jwtio_example_jey ... ok
test lib::rsa::rsa_jwk ... ok
test lib::rsa::rsa_modulus_exponent ... ok
test lib::rsa::round_trip_claim ... ok
test lib::rsa::round_trip_sign_verification_der ... ok
test lib::rsa::round_trip_sign_verification_pem_pkcs8 ... ok
test lib::rsa::round_trip_sign_verification_pem_pkcs1 ... ok
test lib::eddsa::ed_jwk ... ok
test lib::eddsa::ed_x ... ok
test lib::eddsa::round_trip_claim ... ok
test lib::eddsa::round_trip_sign_verification_pem ... ok
test lib::eddsa::round_trip_sign_verification_pk8 ... ok
test lib::header::x5c_der_invalid_chain ... ok
test lib::header::x5c_der_valid_chain ... ok
test lib::header::x5c_der_empty_chain ... ok

test result: ok. 21 passed; 0 failed; 0 ignored

@lpotthast lpotthast mentioned this pull request Nov 19, 2023
@@ -201,6 +209,7 @@ fn rsa_jwk() {
// https://jwt.io/ is often used for examples so ensure their example works with jsonwebtoken
#[cfg(feature = "use_pem")]
#[test]
#[wasm_bindgen_test]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to put that on every test? :(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is unavoidable. Couldn't find aything about the reusability of the standard #[test] macro.

@lpotthast
Copy link
Contributor Author

Added the #[wasm_bindgen_test] macro to all remaining tests, including unit-tests. I do not know if unit-test should be kept out though, and running integration-tests only is enough for the wasm case..

@Keats
Copy link
Owner

Keats commented Nov 20, 2023

I do not know if unit-test should be kept out though, and running integration-tests only is enough for the wasm case..

I think only on integration tests. No need to run unit tests i guess. Although we should probably only have that attribute on wasm target to avoid adding compile time/deps when not needed?

@Keats Keats merged commit 459cdad into Keats:release Dec 1, 2023
6 checks passed
@Keats
Copy link
Owner

Keats commented Dec 1, 2023

Fixed the CI, thanks!

Keats added a commit that referenced this pull request Dec 1, 2023
* Add wasm tests using was-bindgen-test and wasm-pack

* Remove custom wasm feature

* Add #[wasm_bindgen_test] to remaining tests

* Try to fix CI

---------

Co-authored-by: Vincent Prouillet <github@vincentprouillet.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants