-
Notifications
You must be signed in to change notification settings - Fork 409
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
test(command/build): add a test for build command #408
Conversation
As is expected, this PR would fail because we set Besides, wasm-bindgen also try to find target at tmp_project:
Pending #396 until this test is passed |
Besides, here is a question: would it be able to modify the same CARGO_TARGET_DIR when multiple tests are running? |
the test case would pass after solving rustwasm#408
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? |
I hope this is not actually trying to read a
Cargo ensures that target dirs are used in a mutually exclusive way, so we are safe from races. Is that your concern?
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: Lines 69 to 73 in 60bdb80
|
this commit fixes rustwasm#390, rustwasm#414, and closes rustwasm#408 for the test case have to pass after a successful build.
the old code are hard coded path with "/", which may cause error on windows, thus changing to use PathBuf.join. fixing rustwasm#414
It's a mis-copying.
I means that the command try to find wasm files in the relative target directory while the wasms have been outputed at
I've used |
Hm, I'm actually not sure. But @alexcrichton would know. |
I think |
No, it's not, so in the codes I match if the environment variable exists. But I'll change it. |
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:
Besides not outputing sub-command's output in test improves the CI performence. What do you think about it? @fitzgen @alexcrichton |
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... |
I think we can redirect the stderr to a null, for all test result output to stdout and corespondent error will output there too |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @huangjj27 :)
I'm not sure I follow your proposal, do you mean from the |
I mean the script command. for example, |
the test case would pass after solving rustwasm#408
this commit fixes rustwasm#390, rustwasm#414, and closes rustwasm#408 for the test case have to pass after a successful build.
Change to INFO when alerting users about missing fields in Cargo.toml Combine filed missing messages into one INFO line Fix bad formating Merge pull request rustwasm#394 from mstallmo/master Change to INFO when alerting users about missing fields Cargo.toml child: Always print everything to our output Also don't buffer the child's stdout and stderr. error: Add stdout to the `Error::Cli` variant refactor: Return failure::Error instead of wasm_pack::error::Error refactor: Import self and use full module path for failure Use full module path for failure to be consistent since it's used like that in other modules. refactor: Return failure::Error instead of wasm_pack::error::Error chore: Run rustfmt chore: Run rustfmt Merge pull request rustwasm#392 from fitzgen/child-process-and-output-management Child process and output management Merge pull request rustwasm#401 from drager/return-failure-error Return `Result<T, failure::Error>` instead of `Result<T, wasm_pack::error::Error>` v0.5.1 Merge pull request rustwasm#404 from rustwasm/0.5.1 v0.5.1 feat(website): bump vers Merge pull request rustwasm#405 from rustwasm/website-update feat(website): bump vers test(command/build): add a test for build command useing local wasm-bindgen Fix typo in test function name for copying the README bugfix(bindgen-target-dir): use PathBuf to join the old code are hard coded path with "/", which may cause error on windows, thus changing to use PathBuf.join. fixing rustwasm#414 change for cargo_metadata Merge branch 'master' into test-build-for-example Merge pull request rustwasm#408 from huangjj27/test-build-for-example test(command/build): add a test for build command Merge pull request rustwasm#412 from mstallmo/typo-fix Fix typo in test function name for copying the README Merge branch 'master' into fix-canonical-paths-on-windows this change is not related to this PR use absolutize remove unused use cargo fmt
the test case would pass after solving rustwasm#408
this commit fixes rustwasm#390, rustwasm#414, and closes rustwasm#408 for the test case have to pass after a successful build.
the test case would pass after solving rustwasm#408
this commit fixes rustwasm#390, rustwasm#414, and closes rustwasm#408 for the test case have to pass after a successful build.
the test case would pass after solving rustwasm#408
this commit fixes rustwasm#390, rustwasm#414, and closes rustwasm#408 for the test case have to pass after a successful build.
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! 📦✅
rustfmt
installed and have yourcloned directory set to nightly
$ rustup override set nightly $ rustup component add rustfmt-preview --toolchain nightly
rustfmt
on the code base before submitting✨✨ 😄 Thanks so much for contributing to wasm-pack! 😄 ✨✨