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

Don't use libtest harness for wast tests #4861

Closed
jameysharp opened this issue Sep 3, 2022 · 1 comment · Fixed by #8598
Closed

Don't use libtest harness for wast tests #4861

jameysharp opened this issue Sep 3, 2022 · 1 comment · Fixed by #8598

Comments

@jameysharp
Copy link
Contributor

Feature

Currently, the wasmtime top-level crate has a build.rs to generate a bunch of #[test]-annotated functions in $OUT_DIR. At build-time it finds all the "wast" tests in tests/misc_testuite and tests/spec_testsuite (if the latter submodule is present), and generates a couple test functions for each.

Instead I think we should set harness = false in Cargo.toml for tests/all/wast.rs, and give it a main function. It should find these tests when the test runs, not when it's compiled.

Benefit

I noticed this because adding a new test in tests/misc_testsuite didn't cause $OUT_DIR/wast_testsuite_tests.rs to get regenerated. Avoiding the build step means we don't have to make cargo re-run the build step when tests are added or changed.

Locating the wast files during runtime also means that if somebody didn't have tests/spec_testsuite cloned before trying to run the tests, they just have to run the appropriate git submodule update command and then try running tests again.

This should improve build time, both because we don't have to compile and run a build.rs plus rustfmt, and also because we don't have to compile a 140kB Rust source file when running tests.

Implementation

We have a similar situation in cranelift/tests/filetests.rs, which switched away from the libtest harness in 54f9587.

With the current libtest-based implementation, we can run a specific wast test by passing its name to cargo test. It'd be nice to keep that feature. So the new main function would need to interpret std::env::args as passed to it by cargo test, and ideally match how libtest interprets filters. Maybe some of the other libtest command-line options would be nice to have too.

The parts of build.rs that find all the tests should get moved into tests/all/wast.rs. (I think it's especially helpful to preserve the warning that's reported if tests/spec_testsuite is empty.) Otherwise a significant part of the code is just arranging to emit calls to run_wast, which the non-libtest version can just call directly.

Under libtest, tests are run in parallel by default, but there's some effort in tests/all/wast.rs to somewhat limit parallelism. I think a purely sequential test-runner would probably do okay here, especially for a first cut. We could use rayon to add parallelism later if we want.

Alternatives

We could keep the current implementation. For use in CI, the current approach is okay and helps us catch bugs. I think it's mostly a footgun for developers who might try to add tests. But, uh, that's kind of important too.

We could also stop trying to use the cargo/rustc test infrastructure entirely, and either use shell scripts or a dedicated Rust program along the lines of clif-util. But it's nice to have all tests run in the same framework.

@bjorn3
Copy link
Contributor

bjorn3 commented Sep 3, 2022

The downside of this is that it would mean giving up on a lot of features libtest offers out of the box like the way it prints test results, showing failed test crash backtraces and stdout/stderr (relevant libstd api's are unstable), test filtering, ...

alexcrichton added a commit to alexcrichton/wasmtime that referenced this issue May 12, 2024
This commit moves testing of `*.wast` files out of the `all` test suite
binary and into its own separate binary. The motivation for this is
well-described in bytecodealliance#4861 with one of the chief reasons being that if the
test suite is run and then a new file is added re-running the test suite
won't see the file.

The `libtest-mimic` crate provides an easy way of regaining most of the
features of the `libtest` harness such as parallel test execution and
filters, meaning that it's pretty easy to switch everything over. The
only slightly-tricky bit was redoing the filter for whether a test is
ignored or not, but most of the pieces were copied over from the
previous `build.rs` logic.

Closes bytecodealliance#4861
github-merge-queue bot pushed a commit that referenced this issue May 13, 2024
* Move wast tests to their own test suite

This commit moves testing of `*.wast` files out of the `all` test suite
binary and into its own separate binary. The motivation for this is
well-described in #4861 with one of the chief reasons being that if the
test suite is run and then a new file is added re-running the test suite
won't see the file.

The `libtest-mimic` crate provides an easy way of regaining most of the
features of the `libtest` harness such as parallel test execution and
filters, meaning that it's pretty easy to switch everything over. The
only slightly-tricky bit was redoing the filter for whether a test is
ignored or not, but most of the pieces were copied over from the
previous `build.rs` logic.

Closes #4861

* Fix the `all` suite

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

Successfully merging a pull request may close this issue.

2 participants