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

Get test suite working with wasm #38800

Closed
brson opened this Issue Jan 3, 2017 · 5 comments

Comments

Projects
None yet
4 participants
@brson
Copy link
Contributor

brson commented Jan 3, 2017

Last I looked compiletest could not deal with wasm as outputted today by rustc because the binaryen interpreter expects the wasm file to be located in a specific relative path that compiletest does not set up correctly.

I recall the fix being a matter of refactoring compiletest to pass certain paths down to the process spawning node.

@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented May 10, 2017

This is still a critical wasm bug, and not a difficult one to fix. Run the test suite against the wasm target to see how it's currently broken. Most should simply be failing to load the wasm correctly. It should be possible to get verbose output from the test suite, then take that output and run the node.js test command by hand to figure out the correct invocation; then modify compiletest to do it correctly.

@derekdreery

This comment has been minimized.

Copy link
Contributor

derekdreery commented May 10, 2017

I'm going to have a look at this but I make no promises so someone else who wants to take it should do so.

@derekdreery

This comment has been minimized.

Copy link
Contributor

derekdreery commented May 14, 2017

OK my strategy is

  1. Make compiletest::procsrv::run and compiletest::procsrv::run_background take an extra parameter, current_dir: Option<String>, and if Some add cmd.current_dir to command.
  2. Pass None to these functions everywhere that calls them
  3. For emscripten, set it to Some(self.output_base_name().parent()) in compiletest::runtest::program_output

I'm away for a few days so if this looks good I'll probably do it on wednesday :)

@derekdreery

This comment has been minimized.

Copy link
Contributor

derekdreery commented May 14, 2017

Oh and I also had to pass "-s BINARYEN_METHOD='native-wasm,interpret-binary'" as it defaulted to just native-wasm, which I think isn't implemented yet (I'm still trying to understand emscripten properly).

bors added a commit that referenced this issue Jul 26, 2017

Auto merge of #42059 - derekdreery:bugfix/fix_emscripten_tests, r=ale…
…xcrichton

Make compiletest set cwd before running js tests

Proposed fix for #38800.

Not all tests pass yet - I will mention failures here once the test suite has finished.
@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Aug 27, 2017

I believe this has largely be done and can be verified with the containers in-tree, so closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.