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

Running tests through wasm-pack doesn't work if JS snippets are used #1458

Closed
koute opened this issue Apr 14, 2019 · 3 comments · Fixed by #1465
Closed

Running tests through wasm-pack doesn't work if JS snippets are used #1458

koute opened this issue Apr 14, 2019 · 3 comments · Fixed by #1465
Labels

Comments

@koute
Copy link

koute commented Apr 14, 2019

Describe the Bug

Using JS snippets prevents wasm-pack from being able to run tests.

Steps to Reproduce

  1. Create a new crate.
  2. Paste this into Cargo.toml:
[dependencies]
wasm-bindgen = "0.2"

[dev-dependencies]
wasm-bindgen-test = "0.2"
  1. Paste this into src/lib.rs:
extern crate wasm_bindgen;

#[cfg(test)]
extern crate wasm_bindgen_test;

use wasm_bindgen::prelude::*;

#[cfg(test)]
use wasm_bindgen_test::*;

#[cfg(test)]
#[wasm_bindgen_test]
fn foobar() {
    #[wasm_bindgen(inline_js = "export function foo() {}")]
    extern "C" {
        fn foo();
    }

    foo();
}
  1. Run:
$ wasm-pack test --chrome --headless

Actual Behavior

$ wasm-pack test --chrome --headless
[INFO]: Checking for the Wasm target...
    Finished dev [unoptimized + debuginfo] target(s) in 0.03s
    Finished dev [unoptimized + debuginfo] target(s) in 0.03s
     Running target/wasm32-unknown-unknown/debug/deps/foobar-27ec9db4a368c0d0.wasm
error: executing `wasm-bindgen` over the wasm file
	caused by: failed to generate bindings for JS import `foo`
	caused by: local JS snippets are not supported with `--target nodejs`; see rustwasm/rfcs#6 for more details, but this restriction will be lifted in the future
error: test failed, to rerun pass '--lib'
Error: Running Wasm tests with wasm-bindgen-test failed
Caused by: failed to execute `cargo test`: exited with exit code: 1

Expected Behavior

I expect the tests to run.

Additional Context

I need this to be able to run stdweb's test suite. I could probably hack around it by having a custom test runner, but I'd rather not do that and just use standard tooling.

Also, it seems somewhat counterintuitive that even though I want to run the tests under headless Chrome they're being built for Node.js by wasm-pack? Is this expected?

$ wasm-pack --version
wasm-pack 0.8.1
$ wasm-bindgen --version
wasm-bindgen 0.2.42
@alexcrichton
Copy link
Contributor

Thanks for the report!

This is actually sort of an extension of #822 and is a relic of how the testing situation still has an unresolve question here between wasm-pack and wasm-bindgen that we haven't gotten around to fixing.

What's happening here under the hood is that each test binary (the wasm file) is either configured to automatically run in node.js or in a browser. This configuration is done in code currently with the wasm_bindgen_test_configure! macro (it's a pretty bad interface). When cargo test "executes" all wasm binaries then wasm-bindgen-test-runner will attempt to execute all tests in the test suite that they expect to be run in. This means that --chrome to wasm-pack is also running Node.js tests. I believe --node also runs headless browser tests. (as you can see, not a great situation!)

The problem here is that the default for all tests is to execute in Node.js. The test above is actually configured to run in Node.js because nothing else was mentioned. As a result, wasm-pack test --chrome is failing because it's attempting to execute the test in Node.js. You can fix this with a call to wasm_bindgen_test::wasm_bindgen_test_configure!(run_in_browser); in the file.

@fitzgen IIRC you were working on this way back when wasm-pack test was first implemented, I forget if you had thoughts of how we might solve this?

@fitzgen
Copy link
Member

fitzgen commented Apr 15, 2019

I never started work on resolving this impedance mismatch.

As a strawperson proposal for an end state resolution:

  • Let's make wasm-bindgen-test generate code that does feature detection for where it is running in (don't remember exactly what this would entail)
  • Allow both wasm-pack test --node and wasm-pack test --firefox --headless to work if the code doesn't depend on anything only present in one or the other environment
  • Allow the source to declare that it only supports node or browsers, in which case attempts to run in any other environment exit with an error code and helpful message

How does this sound?

@alexcrichton
Copy link
Contributor

While wasm-bindgen-test does have code to detect if it's running in node/browsers I think it wouldn't solve this issue because tests in browsers may even fail the wasm-bindgen step when they're generating for node (aka the snippets issue). I'm realizing though we have actually previously talked about this so I'm gonna look into implementing that.

alexcrichton added a commit to alexcrichton/wasm-bindgen that referenced this issue Apr 16, 2019
This is intended to handle rustwasm#1458 and rustwasm#822. These issues stem from
behavior where:

    wasm-pack test --node

will actually run both Node.js and browser tests! Two new env vars are
read here, `WASM_BINDGEN_TEST_ONLY_{NODE,WEB}`, which control which
tests are executed by `wasm-bindgen-test-runner`. The intention is that
these will be set by `wasm-pack` to configure which tests are run, and
if test suites are skipped due to the env vars we'll print an
informational message indicating how they can be run.

Closes rustwasm#822
Closes rustwasm#1458
alexcrichton added a commit to alexcrichton/wasm-bindgen that referenced this issue Apr 16, 2019
This is intended to handle rustwasm#1458 and rustwasm#822. These issues stem from
behavior where:

    wasm-pack test --node

will actually run both Node.js and browser tests! Two new env vars are
read here, `WASM_BINDGEN_TEST_ONLY_{NODE,WEB}`, which control which
tests are executed by `wasm-bindgen-test-runner`. The intention is that
these will be set by `wasm-pack` to configure which tests are run, and
if test suites are skipped due to the env vars we'll print an
informational message indicating how they can be run.

Closes rustwasm#822
Closes rustwasm#1458
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants