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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add wasm-pack tests for net to CI #486

Merged

Conversation

littlebenlittle
Copy link
Contributor

@littlebenlittle littlebenlittle commented Jul 18, 2024

Tests to reproduce #484

Fixed by #485 #474

ranile
ranile previously requested changes Jul 18, 2024
Copy link
Collaborator

@ranile ranile left a comment

Choose a reason for hiding this comment

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

Please ensure that the tests aren't failing

@ranile ranile dismissed their stale review July 18, 2024 18:10

reviewed before seeing #474

Copy link
Collaborator

@ranile ranile left a comment

Choose a reason for hiding this comment

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

You'd want to bring this PR up to date with master to get the latest code for CI

@@ -3,8 +3,6 @@ use once_cell::sync::Lazy;
use serde::{Deserialize, Serialize};
use wasm_bindgen_test::*;

wasm_bindgen_test_configure!(run_in_browser);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is needed for browser tests, please keep this in (and gate it behind wasm32-unknown-unknown target)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clippy doesn't like #[cfg(target="wasm32-unkonwn-unknown")] but it seems to work. Let me know if I misunderstood.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think target_arch refers to wasm32 in this case (see https://doc.rust-lang.org/reference/conditional-compilation.html#target_arch). That might be why clippy complains. You want to update CI so clippy is also run on wasm32 target.

You can look at how wasi is cfg-gated in other parts of the code base and do the opposite of it here. If it's target_os = "wasi" then you may be able to do not(target_os = "wasi") to target browser wasm targets

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this seems like a quirk of wasm_bindgen_test. As far as I can tell there's no way straightforward way to run in both node and browser.

Here's one suggestion: rustwasm/wasm-bindgen#2571 (comment)

Added to latest commit.

@@ -1,8 +1,6 @@
use gloo_net::http::QueryParams;
use wasm_bindgen_test::*;

wasm_bindgen_test_configure!(run_in_browser);
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as previous comment

Cargo.lock Outdated Show resolved Hide resolved
Copy link
Collaborator

@ranile ranile left a comment

Choose a reason for hiding this comment

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

Having to add a feature is sad but this is the best we can really do. Thanks for the PR!

@ranile ranile merged commit 1043eaf into rustwasm:master Jul 21, 2024
20 checks passed
@littlebenlittle littlebenlittle deleted the littlebenlittle/ci/net_wasmpack_tests branch July 25, 2024 03:24
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.

2 participants