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 a wasm-pack test subcommand #271

Closed
wants to merge 2 commits into from

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented Aug 27, 2018

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

@fitzgen fitzgen force-pushed the wasm-pack-test branch 3 times, most recently from a7dc79b to a6ad544 Compare August 28, 2018 21:14
@fitzgen fitzgen requested a review from mgattozzi August 28, 2018 21:14
@fitzgen
Copy link
Member Author

fitzgen commented Aug 28, 2018

@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
Copy link
Member Author

fitzgen commented Aug 29, 2018

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
Copy link
Contributor

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

@fitzgen
Copy link
Member Author

fitzgen commented Aug 30, 2018

Working on it 👍

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

fitzgen commented Sep 5, 2018

All that is left is

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

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

fitzgen commented Sep 5, 2018

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 fitzgen changed the title [WIP] Add a wasm-pack test subcommand Add a wasm-pack test subcommand Sep 5, 2018
@fitzgen
Copy link
Member Author

fitzgen commented Sep 5, 2018

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
Copy link
Member Author

fitzgen commented Sep 5, 2018

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...

@fitzgen fitzgen force-pushed the wasm-pack-test branch 2 times, most recently from 57a4cfd to db86bb5 Compare September 5, 2018 22:52
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
Copy link
Member Author

fitzgen commented Sep 5, 2018

CI is green!

@ashleygwilliams ashleygwilliams added this to the 0.5.0 milestone Sep 6, 2018
Copy link
Member

@ashleygwilliams ashleygwilliams left a comment

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
@@ -1,14 +1,17 @@
install:
- ps: Install-Product node 10
Copy link
Member

Choose a reason for hiding this comment

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

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...)

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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...

Copy link
Member

Choose a reason for hiding this comment

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

sure thing! i think that makes sense

.travis.yml Outdated
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
Copy link
Member

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
tests/all/utils/fixture.rs Show resolved Hide resolved
tests/fixtures/wbg-test-browser/src/lib.rs Outdated Show resolved Hide resolved
crate-type = ["cdylib"]

[dependencies]
wasm-bindgen = "0.2.17"
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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

fitzgen commented Sep 7, 2018

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
Copy link
Member Author

fitzgen commented Sep 7, 2018

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

Time to dig into the reflog.

@fitzgen
Copy link
Member Author

fitzgen commented Sep 8, 2018

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
Copy link
Member

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

@fitzgen
Copy link
Member Author

fitzgen commented Sep 12, 2018

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

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

Choose a reason for hiding this comment

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

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?)

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

Choose a reason for hiding this comment

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

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
Copy link
Member

r+ from me once the rebase happens :)

@fitzgen
Copy link
Member Author

fitzgen commented Sep 13, 2018

Moved to #306

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants