Add a `wasm-pack test` subcommand #271

fitzgen · 2018-08-27T21:38:43Z

Still need to write some tests and figure out how to get CI integration working.

fitzgen · 2018-08-28T21:16:27Z

@mgattozzi I think this is ready for review to start.

Still todo:

Use curl, flate2, and tar crates instead of forking out to utilities that probably don't exist on windows in the tests.
Maybe move the test utilities that fetch geckodriver into wasm-pack proper, since we have to write that code anyways? That way no one will have to download a webdriver and put it on $PATH themselves...
Download + install node and firefox in CI so we can run the tests there

fitzgen · 2018-08-29T00:30:25Z

Laying out some infra for properly downloading, un-gzipping, and untaring in #273

Will rebase on that to make use of it once that merges

mgattozzi · 2018-08-30T18:58:58Z

@fitzgen Since #273 landed mind rebasing so I can review the PR?

fitzgen · 2018-08-30T20:09:04Z

Working on it 👍

fitzgen · 2018-09-05T00:16:28Z

All that is left is

rebasing again
getting CI setup with a firefox and a chrome for testing

fitzgen · 2018-09-05T20:51:45Z

Ok, I think this is ready for review!

@mgattozzi @ashleygwilliams whoever wants to take this on, it is a little bit of a behemoth, sorry :-p

I'd like to merge this fairly soon, just so that I don't have to keep rebasing.

fitzgen · 2018-09-05T21:45:19Z

Hrmmm still getting some windows failures in appveyor:

failures:
---- test::it_can_run_browser_tests stdout ----
wasm-pack: copying test fixture '\\?\C:\projects\wasm-pack-071k0\tests\fixtures\wbg-test-browser' to temporary directory 'C:\Users\appveyor\AppData\Local\Temp\1\.tmpdGgs9M\wasm-pack'
thread 'test::it_can_run_browser_tests' panicked at 'assertion failed: geckodriver.is_file()', src\webdriver.rs:101:5
note: Run with `RUST_BACKTRACE=1` for a backtrace.
---- webdriver::can_install_geckodriver stdout ----
wasm-pack: copying test fixture '\\?\C:\projects\wasm-pack-071k0\tests\fixtures\js-hello-world' to temporary directory 'C:\Users\appveyor\AppData\Local\Temp\1\.tmpOFCwbJ\wasm-pack'
thread 'webdriver::can_install_geckodriver' panicked at 'assertion failed: geckodriver.is_file()', src\webdriver.rs:101:5
failures:
    test::it_can_run_browser_tests
    webdriver::can_install_geckodriver
test result: FAILED. 23 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out
error: test failed, to rerun pass '--test all'

fitzgen · 2018-09-05T21:53:53Z

Hrmmm still getting some windows failures in appveyor:

Pretty sure this is because of the .exe file extension for the binaries on windows. Just pushed a fix, let's see if that get's us green...

This is a wrapper over `cargo test --target wasm32-unknown-unknown` and the `wasm-bindgen-test` crate. It will run tests in both node and browsers using WebDriver clients. Fixes rustwasm#248

fitzgen · 2018-09-05T23:59:27Z

CI is green!

ashleygwilliams

ok! this took me nearly an hour to review- but YAY nice work.

so overall the comments are nits with a few reminders to add some stuff we discussed in IRC. overall- if at all possible, i'd like to see the larger functions in the binary fetching and some of the test functions be made into smaller functions. additionally- i think it'd be nice to see some unit tests for those, and making them smaller will make them way easier to test :) in the long run it might actually make much more sense for the binary fetching to be it's own crate, but we don't have to necessarily decide that right now. great work and just ping me when the PR is updated!

.appveyor.yml

.travis.yml

ashleygwilliams · 2018-09-07T17:22:23Z

.appveyor.yml

@@ -1,14 +1,17 @@
 install:
+  - ps: Install-Product node 10


i'm not entirely sure how this works, but we might want to use a node version manager for this as well? there are windows ones, last time i tried (admittedly a while ago), i like nodist (which now that i link to it looks like it needs a maintainer...)

i'd also like to not hardcode a version but specify LTS, not sure how Install Product works, but if we switched to a version manager i'm sure we could

AFAICT, Install-Product cannot do LTS.

As for using a node version manager, I'd like to punt on that until if/when it becomes an issue, unless you feel strongly about it. Tinkering with CI for a different OS is not fun...

sure thing! i think that makes sense

ashleygwilliams · 2018-09-07T17:23:49Z

.travis.yml

+    rustup target add wasm32-unknown-unknown
+    curl -o- https://raw.githubusercontent.com/creationix/nvm/v0.33.8/install.sh | bash
+    source ~/.nvm/nvm.sh
+    nvm install v10.5


instead of using a hardcoded version, we could use LTS... nvm support page is here: https://github.com/creationix/nvm#long-term-support

nvm install lts

src/binaries.rs

tests/all/utils/fixture.rs

tests/fixtures/wbg-test-browser/src/lib.rs

tests/fixtures/wbg-test-browser/tests/browser.rs

ashleygwilliams · 2018-09-07T18:20:58Z

tests/fixtures/wbg-test-fail/Cargo.toml

+crate-type = ["cdylib"]
+
+[dependencies]
+wasm-bindgen = "0.2.17"


hardcoding wasm-bindgen version has proved to be a mess for us before- probably want to use "0.2"

This needs to be hard coded until we support reading from the Cargo.lock

ashleygwilliams · 2018-09-07T18:21:18Z

tests/fixtures/wbg-test-fail/src/lib.rs

+mod tests {
+    #[test]
+    fn it_works() {
+        assert_eq!(2 + 2, 4);


this says test fail, but this test looks as though it should pass....

Again, this was the default cargo project, and I'll remove it. The important part is in tests/

fitzgen · 2018-09-07T23:51:36Z

Thanks for the review -- I think this is ready for another round!

if at all possible, i'd like to see the larger functions in the binary fetching and some of the test functions be made into smaller functions.

Done!

i think it'd be nice to see some unit tests for those, and making them smaller will make them way easier to test :)

We have tests that we can download teh binaries and unpack them -- did you want tests for isolated parts of that? Like the string interpolation that does URL construction? If you're unsatisfied with this new iteration, then some more specific requests along this line would be appreciated :)

fitzgen · 2018-09-07T23:59:29Z

Oh no, something happened with rebasing/merging and everything got lost :-p

Time to dig into the reflog.

fitzgen · 2018-09-08T00:21:18Z

Ok, this has the up-to-date changes again, but I somehow accidentally squashed a bunch of work into a merge commit (that is somehow authored by you, @ashleygwilliams??) and I can't figure out how to pull that apart without losing a bunch of the review fixups. So this is the weird history it has now, I guess....

ashleygwilliams · 2018-09-11T15:17:49Z

i am so confused about what happened here... anyways, will give this a review later today!

fitzgen · 2018-09-12T20:16:28Z

btw, I've added some docs for wasm-bindgen-test in the wasm-bindgen guide in this PR: rustwasm/wasm-bindgen#818

ashleygwilliams · 2018-09-13T14:00:59Z

src/binaries.rs

@@ -0,0 +1,215 @@
+//! Utilities for finding and installing binaries that we depend on.


let's land this but open an isue to potentially move it out. i could see it being useful for other CLI projects potentially (might even want to add it to cargo at somepoint?)

ashleygwilliams · 2018-09-13T14:04:08Z

src/build.rs

+    let output = {
+        let mut cmd = Command::new("cargo");
+        cmd.current_dir(path)
+            .arg("+nightly")


do we want to be forcing nightly here? there's an issue open about forcing nightly- im' happy to keep this for now but then review it with that issue later?

ashleygwilliams · 2018-09-13T21:21:49Z

r+ from me once the rebase happens :)

fitzgen · 2018-09-13T23:31:10Z

Moved to #306

fitzgen force-pushed the wasm-pack-test branch 3 times, most recently from a7dc79b to a6ad544 Compare August 28, 2018 21:14

fitzgen requested a review from mgattozzi August 28, 2018 21:14

fitzgen force-pushed the wasm-pack-test branch 3 times, most recently from c4bc8f0 to 031d70e Compare September 5, 2018 00:15

fitzgen force-pushed the wasm-pack-test branch 5 times, most recently from ffbff90 to 238781c Compare September 5, 2018 20:50

fitzgen requested a review from ashleygwilliams September 5, 2018 20:51

fitzgen force-pushed the wasm-pack-test branch from 238781c to a4013f4 Compare September 5, 2018 21:12

fitzgen changed the title ~~[WIP] Add a wasm-pack test subcommand~~ Add a wasm-pack test subcommand Sep 5, 2018

fitzgen force-pushed the wasm-pack-test branch from a4013f4 to 6ec0d1a Compare September 5, 2018 21:53

fitzgen force-pushed the wasm-pack-test branch 2 times, most recently from 57a4cfd to db86bb5 Compare September 5, 2018 22:52

feature: Implement a wasm-pack test subcommand

041438e

This is a wrapper over `cargo test --target wasm32-unknown-unknown` and the `wasm-bindgen-test` crate. It will run tests in both node and browsers using WebDriver clients. Fixes rustwasm#248

fitzgen force-pushed the wasm-pack-test branch from db86bb5 to 041438e Compare September 5, 2018 23:32

ashleygwilliams added the needs review label Sep 6, 2018

ashleygwilliams added this to the 0.5.0 milestone Sep 6, 2018

ashleygwilliams requested changes Sep 7, 2018

View reviewed changes

fitzgen force-pushed the wasm-pack-test branch 2 times, most recently from 4053222 to ccc2158 Compare September 7, 2018 23:13

Merge branch 'master' into wasm-pack-test

dab809d

fitzgen force-pushed the wasm-pack-test branch from ccc2158 to dab809d Compare September 7, 2018 23:49

fitzgen force-pushed the wasm-pack-test branch from dab809d to 8f40e5d Compare September 7, 2018 23:53

fitzgen force-pushed the wasm-pack-test branch from 8f40e5d to dab809d Compare September 8, 2018 00:02

data-pup mentioned this pull request Sep 12, 2018

(WIP) Use Cargo.lock to identify wasm-bindgen version. #302

Closed

3 tasks

fitzgen added Blocking Rust 2018 labels Sep 13, 2018

ashleygwilliams approved these changes Sep 13, 2018

View reviewed changes

ashleygwilliams added needs rebase and removed needs review labels Sep 13, 2018

fitzgen closed this Sep 13, 2018

fitzgen mentioned this pull request Sep 13, 2018

feature(test): Add a wasm-pack test subcommand #306

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a `wasm-pack test` subcommand #271

Add a `wasm-pack test` subcommand #271

fitzgen commented Aug 27, 2018

fitzgen commented Aug 28, 2018 •

edited

Loading

fitzgen commented Aug 29, 2018

mgattozzi commented Aug 30, 2018

fitzgen commented Aug 30, 2018

fitzgen commented Sep 5, 2018 •

edited

Loading

fitzgen commented Sep 5, 2018

fitzgen commented Sep 5, 2018

fitzgen commented Sep 5, 2018

fitzgen commented Sep 5, 2018

ashleygwilliams left a comment

ashleygwilliams Sep 7, 2018

ashleygwilliams Sep 7, 2018

fitzgen Sep 7, 2018

ashleygwilliams Sep 13, 2018

ashleygwilliams Sep 7, 2018

ashleygwilliams Sep 7, 2018

fitzgen Sep 7, 2018

ashleygwilliams Sep 7, 2018

fitzgen Sep 7, 2018

fitzgen commented Sep 7, 2018

fitzgen commented Sep 7, 2018

fitzgen commented Sep 8, 2018

ashleygwilliams commented Sep 11, 2018

fitzgen commented Sep 12, 2018

ashleygwilliams Sep 13, 2018

ashleygwilliams Sep 13, 2018

ashleygwilliams commented Sep 13, 2018

fitzgen commented Sep 13, 2018

		@@ -0,0 +1,215 @@
		//! Utilities for finding and installing binaries that we depend on.

Add a wasm-pack test subcommand #271

Add a wasm-pack test subcommand #271

Conversation

fitzgen commented Aug 27, 2018

fitzgen commented Aug 28, 2018 • edited Loading

fitzgen commented Aug 29, 2018

mgattozzi commented Aug 30, 2018

fitzgen commented Aug 30, 2018

fitzgen commented Sep 5, 2018 • edited Loading

fitzgen commented Sep 5, 2018

fitzgen commented Sep 5, 2018

fitzgen commented Sep 5, 2018

fitzgen commented Sep 5, 2018

ashleygwilliams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fitzgen commented Sep 7, 2018

fitzgen commented Sep 7, 2018

fitzgen commented Sep 8, 2018

ashleygwilliams commented Sep 11, 2018

fitzgen commented Sep 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashleygwilliams commented Sep 13, 2018

fitzgen commented Sep 13, 2018

Add a `wasm-pack test` subcommand #271

Add a `wasm-pack test` subcommand #271

fitzgen commented Aug 28, 2018 •

edited

Loading

fitzgen commented Sep 5, 2018 •

edited

Loading