-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Comments
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
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 intests/misc_testuite
andtests/spec_testsuite
(if the latter submodule is present), and generates a couple test functions for each.Instead I think we should set
harness = false
inCargo.toml
fortests/all/wast.rs
, and give it amain
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 appropriategit 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
plusrustfmt
, 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 newmain
function would need to interpretstd::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 intotests/all/wast.rs
. (I think it's especially helpful to preserve the warning that's reported iftests/spec_testsuite
is empty.) Otherwise a significant part of the code is just arranging to emit calls torun_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.
The text was updated successfully, but these errors were encountered: