From 23452046ecc5531082e3dc2061b27fee7c77c82c Mon Sep 17 00:00:00 2001 From: Azriel Hoh Date: Mon, 18 May 2020 14:41:42 +1200 Subject: [PATCH 1/8] Passes `extra_options` to `cargo build --tests` when running `wasm-pack test`. Issue #698 --- src/build/mod.rs | 19 ++++++++++++++-- src/command/test.rs | 2 +- tests/all/test.rs | 53 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 71 insertions(+), 3 deletions(-) diff --git a/src/build/mod.rs b/src/build/mod.rs index f87133dd..c19d12ac 100644 --- a/src/build/mod.rs +++ b/src/build/mod.rs @@ -112,11 +112,24 @@ pub fn cargo_build_wasm( Ok(()) } -/// Run `cargo build --tests` targetting `wasm32-unknown-unknown`. +/// Runs `cargo build --tests` targeting `wasm32-unknown-unknown`. /// /// This generates the `Cargo.lock` file that we use in order to know which version of /// wasm-bindgen-cli to use when running tests. -pub fn cargo_build_wasm_tests(path: &Path, debug: bool) -> Result<(), Error> { +/// +/// Note that the command to build tests and the command to run tests must use the same parameters, i.e. features to be +/// disabled / enabled must be consistent for both `cargo build` and `cargo test`. +/// +/// # Parameters +/// +/// * `path`: Path to the crate directory to build tests. +/// * `debug`: Whether to build tests in `debug` mode. +/// * `extra_options`: Additional parameters to pass to `cargo` when building tests. +pub fn cargo_build_wasm_tests( + path: &Path, + debug: bool, + extra_options: &[String], +) -> Result<(), Error> { let mut cmd = Command::new("cargo"); cmd.current_dir(path).arg("build").arg("--tests"); @@ -131,6 +144,8 @@ pub fn cargo_build_wasm_tests(path: &Path, debug: bool) -> Result<(), Error> { cmd.arg("--target").arg("wasm32-unknown-unknown"); + cmd.args(extra_options); + child::run(cmd, "cargo build").context("Compilation of your program failed")?; Ok(()) } diff --git a/src/command/test.rs b/src/command/test.rs index cc77a104..2f1a7e40 100644 --- a/src/command/test.rs +++ b/src/command/test.rs @@ -243,7 +243,7 @@ impl Test { fn step_build_tests(&mut self) -> Result<(), Error> { info!("Compiling tests to wasm..."); - build::cargo_build_wasm_tests(&self.crate_path, !self.release)?; + build::cargo_build_wasm_tests(&self.crate_path, !self.release, &self.extra_options)?; info!("Finished compiling tests to wasm."); Ok(()) diff --git a/tests/all/test.rs b/tests/all/test.rs index e0e204aa..70083c9c 100644 --- a/tests/all/test.rs +++ b/tests/all/test.rs @@ -337,3 +337,56 @@ fn test_output_is_printed_once_in_both_stdout_and_failures() { out.matches("YABBA DABBA DOO").count() == log_cnt * 2 })); } + +#[test] +fn extra_options_is_passed_to_cargo_when_building_tests() { + let fixture = fixture::Fixture::new(); + fixture + .readme() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + authors = [] + + [dev-dependencies] + wasm-bindgen-test = "0.2" + + [features] + default = ["native"] + native = [] + "#, + ) + .file( + "src/lib.rs", + r#" + pub fn foo() -> u32 { + #[cfg(feature = "native")] + compile_error!("Test should pass through `--no-default-features` for this to pass."); + + 1 + } + "#, + ) + .file( + "tests/foo.rs", + r#" + extern crate wasm_bindgen_test; + use wasm_bindgen_test::*; + + #[wasm_bindgen_test] + fn smoke() { + foo::foo(); + } + "#, + ) + .install_local_wasm_bindgen(); + let _lock = fixture.lock(); + fixture + .wasm_pack() + .args(&["test", "--node", "--", "--no-default-features"]) + .assert() + .success(); +} From 44e826e8200cb3387a151c4d391070f25b6e8eac Mon Sep 17 00:00:00 2001 From: Azriel Hoh Date: Mon, 18 May 2020 14:55:44 +1200 Subject: [PATCH 2/8] Updates `CHANGELOG.md`. --- CHANGELOG.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 17eb651f..e00db725 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,20 @@ # Changelog +## 🤍 Unreleased + +- ### 🤕 Fixes + + - **Pass through extra options when building tests - [azriel91], [issue/698] [pull/851]** + + `wasm-pack test` accepts extra options to pass through to `cargo` when running tests. + Under the hood, this runs `cargo build` before `cargo test`, and the additional options were only passed through to the `test` command. This meant that crates that enabled native features by default could not be built using `wasm-pack`, as it would attempt to build tests for the `wasm32-unknown-unknown` target with the native features enabled. + + This PR passes through the extra options to `cargo` when building the tests as well. + + [azriel91]: https://github.com/azriel91 + [pull/851]: https://github.com/rustwasm/wasm-pack/pull/851 + [issue/698]: https://github.com/rustwasm/wasm-pack/issues/698 + ## ☁️ 0.9.1 - ### 🤕 Fixes From d1cd431a51c4b7a7212b9caf9cd67bbcf8f8341f Mon Sep 17 00:00:00 2001 From: Azriel Hoh Date: Mon, 18 May 2020 19:39:16 +1200 Subject: [PATCH 3/8] Don't pass through test filter to `cargo build`. --- src/command/test.rs | 10 +++++++++- tests/all/test.rs | 14 +++++++++++++- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/src/command/test.rs b/src/command/test.rs index 2f1a7e40..10e87379 100644 --- a/src/command/test.rs +++ b/src/command/test.rs @@ -243,7 +243,15 @@ impl Test { fn step_build_tests(&mut self) -> Result<(), Error> { info!("Compiling tests to wasm..."); - build::cargo_build_wasm_tests(&self.crate_path, !self.release, &self.extra_options)?; + // If the user has run `wasm-pack test -- --features "f1" -- test_name`, then we want to only pass through + // `--features "f1"` to `cargo build` + let extra_options = + if let Some(index) = self.extra_options.iter().position(|arg| arg == "--") { + &self.extra_options[..index] + } else { + &self.extra_options + }; + build::cargo_build_wasm_tests(&self.crate_path, !self.release, extra_options)?; info!("Finished compiling tests to wasm."); Ok(()) diff --git a/tests/all/test.rs b/tests/all/test.rs index 70083c9c..a013818d 100644 --- a/tests/all/test.rs +++ b/tests/all/test.rs @@ -380,13 +380,25 @@ fn extra_options_is_passed_to_cargo_when_building_tests() { fn smoke() { foo::foo(); } + + #[wasm_bindgen_test] + fn fire() { + panic!("This should be filtered from test execution."); + } "#, ) .install_local_wasm_bindgen(); let _lock = fixture.lock(); fixture .wasm_pack() - .args(&["test", "--node", "--", "--no-default-features"]) + .args(&[ + "test", + "--node", + "--", + "--no-default-features", + "--", + "smoke", + ]) .assert() .success(); } From 37685515582f3e34a320d6213c5d733a55a9165a Mon Sep 17 00:00:00 2001 From: Azriel Hoh Date: Sat, 3 Oct 2020 12:14:41 +1300 Subject: [PATCH 4/8] Update `structopt` to `0.3`. --- Cargo.lock | 42 +++++++++++++++++++++++++++++++++++------- Cargo.toml | 2 +- 2 files changed, 36 insertions(+), 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ac2e51b2..8b618e1c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1,5 +1,7 @@ # This file is automatically @generated by Cargo. # It is not intended for manual editing. +version = 3 + [[package]] name = "adler32" version = "1.0.4" @@ -1260,6 +1262,30 @@ dependencies = [ "treeline", ] +[[package]] +name = "proc-macro-error" +version = "1.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "da25490ff9892aab3fcf7c36f08cfb902dd3e71ca0f9f9517bea02a73a5ce38c" +dependencies = [ + "proc-macro-error-attr", + "proc-macro2 1.0.8", + "quote 1.0.2", + "syn 1.0.14", + "version_check", +] + +[[package]] +name = "proc-macro-error-attr" +version = "1.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a1be40180e52ecc98ad80b184934baf3d0d29f979574e439af5a55274b35f869" +dependencies = [ + "proc-macro2 1.0.8", + "quote 1.0.2", + "version_check", +] + [[package]] name = "proc-macro2" version = "0.4.30" @@ -1819,24 +1845,26 @@ checksum = "8ea5119cdb4c55b55d432abb513a0429384878c15dde60cc77b1c99de1a95a6a" [[package]] name = "structopt" -version = "0.2.18" +version = "0.3.21" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "16c2cdbf9cc375f15d1b4141bc48aeef444806655cd0e904207edc8d68d86ed7" +checksum = "5277acd7ee46e63e5168a80734c9f6ee81b1367a7d8772a2d765df2a3705d28c" dependencies = [ "clap", + "lazy_static 1.4.0", "structopt-derive", ] [[package]] name = "structopt-derive" -version = "0.2.18" +version = "0.4.14" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "53010261a84b37689f9ed7d395165029f9cc7abb9f56bbfe86bee2597ed25107" +checksum = "5ba9cdfda491b814720b6b06e0cac513d922fc407582032e8706e9f137976f90" dependencies = [ "heck", - "proc-macro2 0.4.30", - "quote 0.6.13", - "syn 0.15.44", + "proc-macro-error", + "proc-macro2 1.0.8", + "quote 1.0.2", + "syn 1.0.14", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index 1b918217..158774b4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -30,7 +30,7 @@ serde_ignored = "0.0.4" serde_json = "1.0.26" strsim = "0.8.0" siphasher = "0.2.3" -structopt = "0.2" +structopt = "0.3" toml = "0.4" which = "2.0.0" binary-install = "0.0.2" From b75773a2fe0ab01f7de207e076d92cbc6a6812e4 Mon Sep 17 00:00:00 2001 From: Azriel Hoh Date: Sat, 3 Oct 2020 12:15:08 +1300 Subject: [PATCH 5/8] Use `--manifest-path` for `Cargo.toml`, and modify `AppSettings`. This allows the extra arguments to be forwarded through to `cargo` without requiring `--` beforehand. --- src/command/test.rs | 8 ++++++-- tests/all/test.rs | 9 +-------- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/src/command/test.rs b/src/command/test.rs index 10e87379..388ef010 100644 --- a/src/command/test.rs +++ b/src/command/test.rs @@ -12,12 +12,17 @@ use log::info; use manifest; use std::path::PathBuf; use std::time::Instant; +use structopt::clap::AppSettings; use test::{self, webdriver}; #[derive(Debug, Default, StructOpt)] +#[structopt( + setting = AppSettings::AllowLeadingHyphen, + setting = AppSettings::TrailingVarArg, +)] /// Everything required to configure the `wasm-pack test` command. pub struct TestOptions { - #[structopt(parse(from_os_str))] + #[structopt(parse(from_os_str), long = "manifest-path")] /// The path to the Rust crate. If not set, searches up the path from the current dirctory. pub path: Option, @@ -74,7 +79,6 @@ pub struct TestOptions { /// Build with the release profile. pub release: bool, - #[structopt(last = true)] /// List of extra options to pass to `cargo test` pub extra_options: Vec, } diff --git a/tests/all/test.rs b/tests/all/test.rs index a013818d..bc95e8d6 100644 --- a/tests/all/test.rs +++ b/tests/all/test.rs @@ -391,14 +391,7 @@ fn extra_options_is_passed_to_cargo_when_building_tests() { let _lock = fixture.lock(); fixture .wasm_pack() - .args(&[ - "test", - "--node", - "--", - "--no-default-features", - "--", - "smoke", - ]) + .args(&["test", "--node", "--no-default-features", "--", "smoke"]) .assert() .success(); } From f4f658859599600e9b146bce9ac426c550c4c2b0 Mon Sep 17 00:00:00 2001 From: Azriel Hoh Date: Sat, 3 Oct 2020 12:34:09 +1300 Subject: [PATCH 6/8] Document why we use `AppSettings`. --- src/command/test.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/command/test.rs b/src/command/test.rs index 388ef010..2fc569f0 100644 --- a/src/command/test.rs +++ b/src/command/test.rs @@ -17,7 +17,10 @@ use test::{self, webdriver}; #[derive(Debug, Default, StructOpt)] #[structopt( + // Allows unknown `--option`s to be parsed as positional arguments, so we can forward it to `cargo`. setting = AppSettings::AllowLeadingHyphen, + + // Allows `--` to be parsed as an argument, so we can forward it to `cargo`. setting = AppSettings::TrailingVarArg, )] /// Everything required to configure the `wasm-pack test` command. From 7a7abf934eb03eee490e2b39afc3bd74968cd540 Mon Sep 17 00:00:00 2001 From: Azriel Hoh Date: Sun, 18 Oct 2020 13:42:22 +1300 Subject: [PATCH 7/8] Parse path and extra options together, then split them after. This allows `wasm-pack` to have the same command line interface as `cargo` to pass through options such as `--no-default-features`. --- src/command/test.rs | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/src/command/test.rs b/src/command/test.rs index 2fc569f0..9c5ab9b1 100644 --- a/src/command/test.rs +++ b/src/command/test.rs @@ -11,6 +11,7 @@ use lockfile::Lockfile; use log::info; use manifest; use std::path::PathBuf; +use std::str::FromStr; use std::time::Instant; use structopt::clap::AppSettings; use test::{self, webdriver}; @@ -25,10 +26,6 @@ use test::{self, webdriver}; )] /// Everything required to configure the `wasm-pack test` command. pub struct TestOptions { - #[structopt(parse(from_os_str), long = "manifest-path")] - /// The path to the Rust crate. If not set, searches up the path from the current dirctory. - pub path: Option, - #[structopt(long = "node")] /// Run the tests in Node.js. pub node: bool, @@ -82,8 +79,14 @@ pub struct TestOptions { /// Build with the release profile. pub release: bool, - /// List of extra options to pass to `cargo test` - pub extra_options: Vec, + /// Path to the Rust crate, and extra options to pass to `cargo test`. + /// + /// If the path is not provided, this command searches up the path from the current dirctory + /// + /// This is a workaround to allow wasm pack to provide the same command line interface as `cargo`. + /// See for more information. + #[structopt(allow_hyphen_values = true)] + pub path_and_extra_options: Vec, } /// A configured `wasm-pack test` command. @@ -111,7 +114,6 @@ impl Test { /// Construct a test command from the given options. pub fn try_from_opts(test_opts: TestOptions) -> Result { let TestOptions { - path, node, mode, headless, @@ -122,9 +124,23 @@ impl Test { geckodriver, safari, safaridriver, - extra_options, + mut path_and_extra_options, } = test_opts; + let first_arg_is_path = path_and_extra_options + .get(0) + .map(|first_arg| !first_arg.starts_with("-")) + .unwrap_or(false); + + let (path, extra_options) = if first_arg_is_path { + let path = PathBuf::from_str(&path_and_extra_options.remove(0))?; + let extra_options = path_and_extra_options; + + (Some(path), extra_options) + } else { + (None, path_and_extra_options) + }; + let crate_path = get_crate_path(path)?; let crate_data = manifest::CrateData::new(&crate_path, None)?; let any_browser = chrome || firefox || safari; From 1f13f25d0fa2d9628d60273f1725bddeda04e1b9 Mon Sep 17 00:00:00 2001 From: Azriel Hoh Date: Mon, 28 Jun 2021 14:57:37 +1200 Subject: [PATCH 8/8] Update tests with newer crate versions. --- tests/all/build.rs | 4 ++-- tests/all/download.rs | 4 ++-- tests/all/lockfile.rs | 12 ++++++------ tests/all/log_level.rs | 1 - tests/all/utils/fixture.rs | 14 +++++++------- 5 files changed, 17 insertions(+), 18 deletions(-) diff --git a/tests/all/build.rs b/tests/all/build.rs index 79053704..0c59ba85 100644 --- a/tests/all/build.rs +++ b/tests/all/build.rs @@ -146,8 +146,8 @@ fn dash_dash_web_target_has_error_on_old_bindgen() { let output = String::from_utf8(cmd.get_output().stderr.clone()).unwrap(); assert!( - output.contains("0.2.39"), - "Output did not contain '0.2.39', output was {}", + output.contains("Please update your project to wasm-bindgen version >= 0.2.39"), + "Output did not contain 'Please update your project to wasm-bindgen version >= 0.2.39', output was {}", output ); } diff --git a/tests/all/download.rs b/tests/all/download.rs index 3f5dec14..05d9f6d5 100644 --- a/tests/all/download.rs +++ b/tests/all/download.rs @@ -12,7 +12,7 @@ fn can_download_prebuilt_wasm_bindgen() { let dir = tempfile::TempDir::new().unwrap(); let cache = Cache::at(dir.path()); if let install::Status::Found(dl) = - install::download_prebuilt(&Tool::WasmBindgen, &cache, "0.2.37", true).unwrap() + install::download_prebuilt(&Tool::WasmBindgen, &cache, "0.2.74", true).unwrap() { assert!(dl.binary("wasm-bindgen").unwrap().is_file()); assert!(dl.binary("wasm-bindgen-test-runner").unwrap().is_file()) @@ -29,7 +29,7 @@ fn can_download_prebuilt_wasm_bindgen() { ))] fn downloading_prebuilt_wasm_bindgen_handles_http_errors() { let dir = tempfile::TempDir::new().unwrap(); - let bad_version = "0.2.37-some-trailing-version-stuff-that-does-not-exist"; + let bad_version = "0.2.74-some-trailing-version-stuff-that-does-not-exist"; let cache = Cache::at(dir.path()); let result = install::download_prebuilt(&Tool::WasmBindgen, &cache, bad_version, true); assert!(result.is_err()); diff --git a/tests/all/lockfile.rs b/tests/all/lockfile.rs index 1a42ce88..f7a87843 100644 --- a/tests/all/lockfile.rs +++ b/tests/all/lockfile.rs @@ -8,7 +8,7 @@ fn it_gets_wasm_bindgen_version() { fixture.cargo_check(); let data = CrateData::new(&fixture.path, None).unwrap(); let lock = Lockfile::new(&data).unwrap(); - assert_eq!(lock.wasm_bindgen_version(), Some("0.2.37"),); + assert_eq!(lock.wasm_bindgen_version(), Some("0.2.74"),); } #[test] @@ -17,7 +17,7 @@ fn it_gets_wasm_bindgen_test_version() { fixture.cargo_check(); let data = CrateData::new(&fixture.path, None).unwrap(); let lock = Lockfile::new(&data).unwrap(); - assert_eq!(lock.wasm_bindgen_test_version(), Some("0.2.37"),); + assert_eq!(lock.wasm_bindgen_test_version(), Some("0.3.24"),); } #[test] @@ -46,7 +46,7 @@ fn it_gets_wasm_bindgen_version_in_crate_inside_workspace() { crate-type = ["cdylib"] [dependencies] - wasm-bindgen = "=0.2.37" + wasm-bindgen = "=0.2.74" "#, ) .file( @@ -62,7 +62,7 @@ fn it_gets_wasm_bindgen_version_in_crate_inside_workspace() { fixture.cargo_check(); let data = CrateData::new(&fixture.path.join("blah"), None).unwrap(); let lock = Lockfile::new(&data).unwrap(); - assert_eq!(lock.wasm_bindgen_version(), Some("0.2.37"),); + assert_eq!(lock.wasm_bindgen_version(), Some("0.2.74"),); } #[test] @@ -91,7 +91,7 @@ fn it_gets_wasm_bindgen_version_from_dependencies() { crate-type = ["cdylib"] [dependencies] - wasm-bindgen = "=0.2.37" + wasm-bindgen = "=0.2.74" "#, ) .file( @@ -130,5 +130,5 @@ fn it_gets_wasm_bindgen_version_from_dependencies() { fixture.cargo_check(); let data = CrateData::new(&fixture.path.join("parent"), None).unwrap(); let lock = Lockfile::new(&data).unwrap(); - assert_eq!(lock.wasm_bindgen_version(), Some("0.2.37"),); + assert_eq!(lock.wasm_bindgen_version(), Some("0.2.74"),); } diff --git a/tests/all/log_level.rs b/tests/all/log_level.rs index d067742d..95a08cab 100644 --- a/tests/all/log_level.rs +++ b/tests/all/log_level.rs @@ -9,7 +9,6 @@ fn matches_info() -> impl Predicate + PredicateReflection { contains("[INFO]: Checking for the Wasm target...") .and(contains("[INFO]: Compiling to Wasm...")) .and(contains("[INFO]: License key is set in Cargo.toml but no LICENSE file(s) were found; Please add the LICENSE file(s) to your project directory")) - .and(contains("[INFO]: Installing wasm-bindgen...")) .and(contains("[INFO]: Optimizing wasm binaries with `wasm-opt`...")) .and(contains("[INFO]: :-) Done in ")) .and(contains("[INFO]: :-) Your wasm pkg is ready to publish at ")) diff --git a/tests/all/utils/fixture.rs b/tests/all/utils/fixture.rs index 6c7c1c48..e51d904c 100644 --- a/tests/all/utils/fixture.rs +++ b/tests/all/utils/fixture.rs @@ -143,10 +143,10 @@ impl Fixture { # bindgen downloaded is what we expect, and if `=` is # removed then it will download whatever the newest version # of wasm-bindgen is which may not be what's listed here. - wasm-bindgen = "=0.2.37" + wasm-bindgen = "=0.2.74" [dev-dependencies] - wasm-bindgen-test = "0.2" + wasm-bindgen-test = "0.3" "#, name ), @@ -221,7 +221,7 @@ impl Fixture { static INSTALL_WASM_BINDGEN: Once = Once::new(); let cache = self.cache(); - let version = "0.2.37"; + let version = "0.2.74"; let download = || { if let Ok(download) = @@ -417,7 +417,7 @@ pub fn no_cdylib() -> Fixture { wasm-bindgen = "0.2" [dev-dependencies] - wasm-bindgen-test = "0.2" + wasm-bindgen-test = "0.3" "#, ); fixture @@ -590,7 +590,7 @@ pub fn transitive_dependencies() -> Fixture { project_b = { path = "../project_b" } [dev-dependencies] - wasm-bindgen-test = "0.2" + wasm-bindgen-test = "0.3" "#, ); fixture.file( @@ -639,7 +639,7 @@ pub fn transitive_dependencies() -> Fixture { project_b = { path = "../project_b" } [dev-dependencies] - wasm-bindgen-test = "0.2" + wasm-bindgen-test = "0.3" "#, ); fixture.file( @@ -688,7 +688,7 @@ pub fn transitive_dependencies() -> Fixture { wasm-bindgen = "0.2" [dev-dependencies] - wasm-bindgen-test = "0.2" + wasm-bindgen-test = "0.3" "#, ); fixture.file(