Test Refactor #1382

syrusakbary · 2020-04-15T23:42:52Z

Description

This PR is the continuation of #1380.
It refactors our testing infrastructure based on various points:

There is no longer a "default compiler" on tests
Tests are automatically generated for: emscripten and wasi
It accelerates testing into multiple threads when possible (with the exception of the llvm backend)
It automatically detects the backends available in the host

Summary of changes:

Review

Add a short description of the the change to the CHANGELOG.md file

syrusakbary · 2020-04-15T23:54:43Z

lib/wasi/src/state/builder.rs

@@ -56,15 +57,23 @@ impl std::fmt::Debug for WasiStateBuilder {
 }

 /// Error type returned when bad data is given to [`WasiStateBuilder`].
-#[derive(Debug, PartialEq, Eq)]
+#[derive(Error, Debug, PartialEq, Eq)]


Note, this is needed because anyhow (used in the wasi tests) needs to receive an Rust error, and it throw me a compiler error when trying to map our errors to anyhow.

So it's actually something that makes our API better as well!
I prefered to do it in one stand, rather than in a PR apart because otherwise I will forget.

build.rs

Cargo.toml

nlewycky

Unfinished review comments.

Cargo.toml

Makefile

build.rs

lib/runtime/src/lib.rs

tests/custom/multiple-traps.wast

tests/generate-wasi-tests/src/set_up_toolchain.rs

lib/wasi/Cargo.toml

MarkMcCaskey · 2020-04-16T00:05:35Z

lib/runtime/src/lib.rs

@@ -291,7 +298,7 @@ pub fn instantiate(wasm: &[u8], import_object: &ImportObject) -> error::Result<I
 /// The output of this function can be controlled by the mutually
 /// exclusive `default-backend-llvm`, `default-backend-singlepass`,
 /// and `default-backend-cranelift` feature flags.
-pub fn default_compiler() -> impl Compiler {
+pub fn default_compiler() -> Box<dyn Compiler> {


What's the reason this is now a dynamic trait object?

I thought it might be because the compiler can't be statically known anymore, but that doesn't seem to be the case

The main reason for this is because I wanted runtime to be compilable even when no backends where set (via the backend- feature).

However, this caused a compilation error in Rust, mentioning that it didn't know the size of Compiler (since for Rust this looked like an empty function).
I tried to resolve it by doing panic! at the end of the function body, but the compilation error still happened, so I changed into a Box` that Rust liked it.

Perhaps there are other ways of doing this, let me know if you have ideas @MarkMcCaskey!

From an efficiency perspective, this doesn't matter at all, but there are some usability concerns.

If users can interact with compilers outside of the Compiler trait, then impl Compiler is strictly better because it's more direct:

let compiler: LLVMCompiler = default_compiler(); compiler.llvm_compiler_specific_method();

Whereas with Box<dyn Compiler> the user has to do some kind of downcasting (I'm not sure how to do this without looking at docs, but I believe it's possible and will require an unwrap).

Also to avoid API breaking it might be worth it to make it work...

One way to do this is to add a Fail compiler which panics on every method... though that really raises the question, in what scenario will a user be compiling with no backends and be calling default_compiler? The only scenario I can think of right now is if the user is making a mistake: if that's the case, then perhaps we want to just remove the default_compiler function if no compiler backends are enabled 🤔

Cargo.toml

build.rs

Co-Authored-By: nlewycky <[email protected]>

MarkMcCaskey

Looks good to me! I want to spend some time trying to break the tests locally before approving -- I'll let you know if I find anything wrong with the tests!

MarkMcCaskey · 2020-04-16T18:55:11Z

lib/api/src/lib.rs

@@ -351,6 +351,10 @@ pub mod error {
 /// Idea for generic trait; consider rename; it will need to be moved somewhere else
 pub trait CompiledModule {
    fn new(bytes: impl AsRef<[u8]>) -> error::CompileResult<Module>;
+    fn new_with_compiler(
+        bytes: impl AsRef<[u8]>,
+        compiler: Box<dyn compiler::Compiler>,


This doesn't strictly have to take a Box we can use &dyn compiler::Compiler or AsRef<&dyn compiler::Compiler> or maybe Deref to be slightly more ergonmic.

Small bike-shed which we can do in a separate pass over our new public APIs before the next release instead of in this PR.

MarkMcCaskey · 2020-04-16T18:55:30Z

lib/api/src/lib.rs

@@ -365,6 +369,14 @@ impl CompiledModule for Module {
        wasmer_runtime_core::compile_with(bytes, &compiler::default_compiler())
    }

+    fn new_with_compiler(
+        bytes: impl AsRef<[u8]>,
+        compiler: Box<dyn compiler::Compiler>,


MarkMcCaskey · 2020-04-16T18:56:01Z

lib/runtime-c-api/src/module.rs

@@ -285,7 +285,7 @@ pub unsafe extern "C" fn wasmer_module_deserialize(
    let serialized_module: &[u8] = &*(serialized_module as *const &[u8]);

    match Artifact::deserialize(serialized_module) {
-        Ok(artifact) => match load_cache_with(artifact, &default_compiler()) {
+        Ok(artifact) => match load_cache_with(artifact, &*default_compiler()) {


I'm not 100% but it may be possible to just do default_compiler() here; though if we revert it back to impl Compiler return value, then we'll still need that &

I tried with both (default_compiler() and &default_compiler()) and Rust complained.

src/commands/run.rs

tests/custom/multiple-traps.wast

tests/test-generator/src/lib.rs

MarkMcCaskey · 2020-04-16T20:00:59Z

tests/test-generator/src/processors.rs

+        return None;
+    }
+
+    // Ignore files starting with `.`, which could be editor temporary files


There are crates that do file globbing that respect .gitignores I think 🤔 , might be simpler

Just looked into it. The most obvious crate is:
https://crates.io/crates/ignore

But I think it's not trivial (as a few mins of work) to add it. If required we can add it in a PR apart. The simple solution works for now though :)

tests/test-generator/src/processors.rs

tests/wasi_serialization.rs

syrusakbary · 2020-04-16T20:27:38Z

bors try

Co-Authored-By: Mark McCaskey <[email protected]>

bors · 2020-04-16T20:55:04Z

try

Build succeeded:

wasmerio.wasmer

syrusakbary · 2020-04-16T22:50:28Z

bors r+

1382: Test Refactor r=syrusakbary a=syrusakbary # Description This PR is the continuation of #1380. It refactors our testing infrastructure based on various points: * There is no longer a "default compiler" on tests * Tests are automatically generated for: emscripten and wasi * It accelerates testing into multiple threads when possible (with the exception of the llvm backend) * It automatically detects the backends available in the host Summary of changes: * [x] Removed all extra logic for test creation in emscripten test generator * [x] Removed all extra logic for test creation in the wasi test generator * [x] Divided wasmer wast test in different files (under `tests/custom/`) * [x] Refactored trap asserts using the wast format * [x] Improved the build script by adding emscripten and wasi test generators (separated from build) * [x] Improved WASI errors to be Rust error types (by deriving from `thiserror::Error`) * [x] Fixed toolchain error creation * [x] Removed leaking of testing logic from generators into build * [x] Refactored import tests to be compatible with multiple backends * [x] Improved wasmer binary to work even when no backends are set (useful for testing without any backend available). * [x] Removed assumption that Cranelift will be the default in the `wasmer` binary * [x] Migrated middleware # Review - [x] Add a short description of the the change to the CHANGELOG.md file Co-authored-by: Syrus <[email protected]> Co-authored-by: Syrus Akbary <[email protected]>

bors · 2020-04-16T23:20:41Z

Build failed:

wasmerio.wasmer

syrusakbary · 2020-04-16T23:23:16Z

bors r+

bors · 2020-04-16T23:45:15Z

Build succeeded:

wasmerio.wasmer

syrusakbary added 3 commits April 15, 2020 16:31

Second part of the refactor

ce14c6c

Merge branch 'master' into test-refactor-2

52c969a

Only run shell tput in *Unix

51a4546

syrusakbary requested review from Hywan and MarkMcCaskey as code owners April 15, 2020 23:42

syrusakbary commented Apr 15, 2020

View reviewed changes

build.rs Outdated Show resolved Hide resolved

syrusakbary commented Apr 16, 2020

View reviewed changes

Cargo.toml Show resolved Hide resolved

nlewycky reviewed Apr 16, 2020

View reviewed changes

MarkMcCaskey reviewed Apr 16, 2020

View reviewed changes

syrusakbary and others added 15 commits April 15, 2020 17:47

Update tests/custom/multiple-traps.wast

8c9d8b2

Co-Authored-By: nlewycky <[email protected]>

Update build.rs

e14a740

Co-Authored-By: nlewycky <[email protected]>

Update Cargo.toml

6a840a7

Co-Authored-By: nlewycky <[email protected]>

Update tests/generate-wasi-tests/src/set_up_toolchain.rs

b1a7dbe

Co-Authored-By: nlewycky <[email protected]>

Update build.rs (truthy env unwrap suggestion)

c541521

Moved processors out of build

dfb7432

Test forgotten function in imports

7b41c2b

Removed unused code on wasitests

37afcd3

Improved tests semantics

cce851f

Improve llvm compat

23a1896

Fixed suggestions

bb3c1a6

Formatted rust files

2835140

Fixed lints

21f7bae

Removed unneeded parenthesis on ARM

8369431

Ignore wasi tests on aarch64

2a897fd

syrusakbary requested a review from losfair as a code owner April 16, 2020 01:42

syrusakbary added 4 commits April 15, 2020 18:48

Fixed syntax

21cef31

Fixed example testing

8a669cf

Fixed command names

6415579

Test only on the backends available

444a68d

MarkMcCaskey reviewed Apr 16, 2020

View reviewed changes

Simplified azure testing

081ab36

bors bot added a commit that referenced this pull request Apr 16, 2020

Try #1382:

c1da047

syrusakbary and others added 7 commits April 16, 2020 13:40

Update tests/test-generator/src/lib.rs

8b177b5

Co-Authored-By: Mark McCaskey <[email protected]>

Update tests/test-generator/src/processors.rs

6021d17

Co-Authored-By: Mark McCaskey <[email protected]>

Update tests/test-generator/src/processors.rs

80ed212

Co-Authored-By: Mark McCaskey <[email protected]>

Fixed suggestions

fbdfa62

Fixed suggestions based on feedback

a2d2710

Fixed forgot name/body improvement in processors

a858401

Fixed llvm tests running cranelift/singlepass

695ac6d

syrusakbary and others added 2 commits April 16, 2020 14:16

Fixed makefile

c9713e4

Conditionally compile default_compiler and related functions

ce62115

MarkMcCaskey approved these changes Apr 16, 2020

View reviewed changes

syrusakbary added 6 commits April 16, 2020 15:30

Make makefile standard for wasitests

9d63da5

Added README to custom wast tests dir

b159163

Fixed lint issue

11a0ab0

Detect singlepass first

ab4a51d

Re-enabled middleware

565462d

Addedd changes into Changelog

c79b736

syrusakbary changed the title ~~Refactor~~ Test Refactor Apr 16, 2020

Fixed dereference error

e714f55

bors bot merged commit e19dbbe into master Apr 16, 2020

bors bot deleted the test-refactor-2 branch April 16, 2020 23:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test Refactor #1382

Test Refactor #1382

syrusakbary commented Apr 15, 2020 •

edited

Loading

syrusakbary Apr 15, 2020

nlewycky left a comment

MarkMcCaskey Apr 16, 2020

MarkMcCaskey Apr 16, 2020

syrusakbary Apr 16, 2020

syrusakbary Apr 16, 2020

MarkMcCaskey Apr 16, 2020

MarkMcCaskey left a comment

MarkMcCaskey Apr 16, 2020

MarkMcCaskey Apr 16, 2020

MarkMcCaskey Apr 16, 2020

syrusakbary Apr 16, 2020

MarkMcCaskey Apr 16, 2020

syrusakbary Apr 16, 2020

syrusakbary commented Apr 16, 2020

bors bot commented Apr 16, 2020

syrusakbary commented Apr 16, 2020

bors bot commented Apr 16, 2020

syrusakbary commented Apr 16, 2020

bors bot commented Apr 16, 2020

Test Refactor #1382

Test Refactor #1382

Conversation

syrusakbary commented Apr 15, 2020 • edited Loading

Description

Review

Choose a reason for hiding this comment

nlewycky 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

MarkMcCaskey 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

syrusakbary commented Apr 16, 2020

bors bot commented Apr 16, 2020

try

syrusakbary commented Apr 16, 2020

bors bot commented Apr 16, 2020

syrusakbary commented Apr 16, 2020

bors bot commented Apr 16, 2020

syrusakbary commented Apr 15, 2020 •

edited

Loading