Stream: git-wasmtime

Topic: wasmtime / issue #4861 Don't use libtest harness for wast...

Wasmtime GitHub notifications bot (Sep 03 2022 at 00:45):

jameysharp opened issue #4861:

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 54f9587569f80c5899d1e75482197d87d2406e15.

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.

Wasmtime GitHub notifications bot (Sep 03 2022 at 05:38):

bjorn3 commented on issue #4861:

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, ...

Wasmtime GitHub notifications bot (May 13 2024 at 16:21):

alexcrichton closed issue #4861:

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 54f9587569f80c5899d1e75482197d87d2406e15.

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.

Last updated: Nov 22 2024 at 17:03 UTC