test(command/build): add a test for build command

[huangjj27]

this PR is to check if a minimal example fixture::js_hello_world can be built in the test.

it provides precondition for #396, and closing #414

---

Make sure these boxes are checked! 📦✅

• You have the latest version of rustfmt installed and have your
  cloned directory set to nightly

$ rustup override set nightly
$ rustup component add rustfmt-preview --toolchain nightly

• You ran rustfmt on the code base before submitting
• You reference which issue is being closed in the PR text

✨✨ 😄 Thanks so much for contributing to wasm-pack! 😄 ✨✨

[huangjj27]

As is expected, this PR would fail because we set CARGO_TARGET_DIR to wasm-pack/target,
so when we run_wasm_pack with a temporary project(wasm-pack/target/t/.tmpXXXXXX/wasm-pack), the outputted files that should be in tmp_project/wasm32-unknown-unknown would be located at wasm-pack/target/wasm32-unknown-unknown.

Besides, wasm-bindgen also try to find target at tmp_project:

wasm-bindgen failed to execute properly. stderr:

error: failed to read `target/wasm32-unknown-unknown/release/js_hello_world.wasm`
	caused by: 系统找不到指定的路径。 (os error 3) system can't  find the path

Pending #396 until this test is passed

[huangjj27]

Besides, here is a question: would it be able to modify the same CARGO_TARGET_DIR when multiple tests are running?

[huangjj27]

I found the related code: https://github.com/rustwasm/wasm-pack/blob/master/src/bindgen.rs#L131, it is dead code to a certain path and will not change according to $CARGO_TARGET_DIR.

one more concern is that if we share the same target directory, will it possible to change the target files when there is multiple test running?

[fitzgen]

  error: failed to read `target/wasm32-unknown-unknown/release/js_hello_world.wa
| 	caused by: No such file or directory (os error 2)
test build::it_should_build_js_hello_world_example ... FAILED

I hope this is not actually trying to read a .wa file, and this is just the output getting cut off and it really is a .wasm file.

Besides, here is a question: would it be able to modify the same CARGO_TARGET_DIR when multiple tests are running?

Cargo ensures that target dirs are used in a mutually exclusive way, so we are safe from races. Is that your concern?

I found the related code: https://github.com/rustwasm/wasm-pack/blob/master/src/bindgen.rs#L131, it is dead code to a certain path and will not change according to $CARGO_TARGET_DIR.

I don't see how it is dead code, can you explain some more?

To make this test pass, we should be using https://docs.rs/cargo_metadata/0.6.0/cargo_metadata/struct.Metadata.html#structfield.target_directory to get the target directory. We are already using that crate for getting hte workspace root:

wasm-pack/src/lockfile.rs

Lines 69 to 73 in 60bdb80

	let crate_root = cargo_metadata::metadata(Some(&manifest))
	.map_err(|_| Error::CrateConfig {
	message: String::from("Error while processing crate metadata"),
	})?
	.workspace_root;

[huangjj27]

I hope this is not actually trying to read a .wa file

It's a mis-copying.

I don't see how it is dead code, can you explain some more?

I means that the command try to find wasm files in the relative target directory while the wasms have been outputed at $CARGO_TARGET_DIR/wasm32-unknown-unknown

To make this test pass, we should be using https://docs.rs/cargo_metadata/0.6.0/cargo_metadata/struct.Metadata.html#structfield.target_directory to get the target directory.

I've used std::env::var("CARGO_TARGET_DIR") to get the target directory as is described in cargo-book/environment-variables. It seems to work fine. Must I change for cargo metadata?

[fitzgen]

I've used std::env::var("CARGO_TARGET_DIR") to get the target directory as is described in cargo-book/environment-variables. It seems to work fine. Must I change for cargo metadata?

Hm, I'm actually not sure. But @alexcrichton would know.

[alexcrichton]

I think CARGO_TARGET_DIR is only read by cargo, not written, so it's not guaranteed to be set. Reading it from cargo metadata would be the most reliable strategy!

[huangjj27]

@alexcrichton

so it's not guaranteed to be set.

No, it's not, so in the codes I match if the environment variable exists.

But I'll change it.

[huangjj27]

It seems that the build outputs too much and takes a long time. I think we don't need to output logs from the wasm-pack command in sub-process, because if we get something wrong, we could get results from sub-processes' stdout:

---- bindgen::can_download_prebuilt_wasm_bindgen stdout ----
thread 'bindgen::can_download_prebuilt_wasm_bindgen' panicked at 'called `Result::unwrap()` on an `Err` value: Http { message: "when requesting https://github.com/rustwasm/wasm-bindgen/releases/download/0.2.21/wasm-bindgen-0.2.21-x86_64-pc-windows-msvc.tar.gz" }', libcore\result.rs:1009:5
note: Run with `RUST_BACKTRACE=1` for a backtrace.

---- webdriver::can_install_geckodriver stdout ----
Created fixture at C:\Users\34937\Documents\wasm-pack\target\t\.tmp5JG25s\wasm-pack
thread 'webdriver::can_install_geckodriver' panicked at 'assertion failed: webdriver::install_geckodriver(&fixture.path).is_ok()', tests\all\webdriver.rs:25:5

Besides not outputing sub-command's output in test improves the CI performence. What do you think about it? @fitzgen @alexcrichton

[fitzgen]

It looks like it is hung at the end of the CI run. Will try poking CI and seeing if that fixes it.

Ideally we would detect when stdout is not a real terminal and avoid doing the spinner and fancy output and just do the normal output. We have an issue on file somewhere...

[huangjj27]

@fitzgen

Ideally we would detect when stdout is not a real terminal and avoid doing the spinner and fancy output and just do the normal output.

I think we can redirect the stderr to a null, for all test result output to stdout and corespondent error will output there too

[fitzgen]

Thanks @huangjj27 :)

[fitzgen]

I think we can redirect the stderr to a null, for all test result output to stdout and corespondent error will output there too

I'm not sure I follow your proposal, do you mean from the script command in .travis.yml or inside wasm-pack code itself?

[huangjj27]

I'm not sure I follow your proposal, do you mean from the script command in .travis.yml or inside wasm-pack code itself?

I mean the script command. for example, cargo test --locked 2>test.log, if we don't need that output, I think cargo test --locked 2>/dev/null will work fine too.