wasmtime: Introduce the test-macros crate
#8622
Conversation
This commit introduces the `test-macros` crate. The whole idea behind this crate is to export a single or multiple macros to make it easier to configure Wasmtime for integration tests. The main use-case at this time, is running a subset of the integration tests with Cranelift and Winch. This crate could be extended to serve other use-cases, like testing pooling allocator and/or GC configurations. This commit introduces a single example of how this macro could be used. If there's agreement on merging this change in some shape or form, I'll follow up with migrating the current tests to use `#[wasmtime_test]` where applicable. Part of what's implemented in this PR was discussed in Cranelift's meeting on [April 24th, 2024](https://github.com/bytecodealliance/meetings/blob/main/cranelift/2024/cranelift-04-24.md), however there are several discussion points that are still "in the air", like for example, what's the best way to avoid the combinatorial explosion problem for the potential test matrix.
saulecabrera
force-pushed
the
add-test-macros
branch
from
6cfd9b1
to
bb155c4
Compare
saulecabrera
force-pushed
the
add-test-macros
branch
from
ff2208c
to
61cde97
Compare
|
Excited for this! Will take a look today |
crates/test-macros/src/lib.rs
Outdated
|
|
||
| parse_macro_input!(attrs with parser); | ||
|
|
||
| match expand(&strategies, parse_macro_input!(item as ItemFn)) { |
There was a problem hiding this comment.
One possible concern about ItemFn here is that in the limit the entire test crate will be tagged with #[wasmtime_test] and that means the entire crate is parsed through syn and emitted back out. Technically the only needed part here is the function signature.
I'm not sure if it's worth it, but this could try to be a bit clever where it parses syn::ItemFn manually up to the block and then keeps the block as an opaque TokenStream (since it's known that it's brace-delimited anyway).
There was a problem hiding this comment.
Good point. Took me a bit to figure out, but I was able to make this idea work by introducing a partial function parser as per your suggestion. Let me know what you think!
| #[wasmtime_test(strategies(Cranelift, Winch))] | ||
| #[cfg_attr(miri, ignore)] | ||
| fn call_wasm_to_wasm() -> Result<()> { | ||
| fn call_wasm_to_wasm(config: &mut Config) -> Result<()> { |
There was a problem hiding this comment.
Yep, this is exactly what I was imagining usage would look like 👍
- Fix version - Add `[lints]` - Add publish key
This commit adds a `TestConfig` struct that holds the supported Wasmtime test configuration. Additonally this commit introduces a partial function parser, in which only the attributes, visibility, and signature are fully parsed, leaving the function body as an opaque `TokenStream`
|
Thanks for the reviews Alex and Nick. I've addressed all the comments, I think this is ready for another round of reviews. |
There was a problem hiding this comment.
Looks great to me, thanks!
I'm going to go ahead and merge, but some other assorted thoughts (which are fine as follow-ups or to ignore entirely):
- Should the default perhaps be the "full matrix" of tests? For example pooling, default settings (cranelift), and winch might be the three main configurations to execute in right now.
- Instead of opting-in to strategy should the attributes be "skip this test on winch" or similar? (e.g. eagerly running all tests instead of having to opt-in to testing)
In any case it might make more sense to expand the current strategy implemented and see how it looks after?
|
Those are good suggestions Alex, I like the idea of opting out instead of opting in, it has the potential to make the macro less verbose and easier to integrate. I'll start migrating the tests and see how things shape up and update the macro definition if needed. |
This commit introduces the
test-macroscrate. The whole idea behind this crate is to export a single or multiple macros to make it easier to configure Wasmtime for integration tests. The main use-case at this time, is running a subset of the integration tests with Cranelift and Winch. This crate could be extended to serve other use-cases, like testing pooling allocator and/or GC configurations.This commit introduces a single example of how this macro could be used. If there's agreement on merging this change in some shape or form, I'll follow up with migrating the current tests to use
#[wasmtime_test]where applicable.Part of what's implemented in this PR was discussed in Cranelift's meeting on April 24th, 2024, however there are several discussion points that are still "in the air", like for example, what's the best way to avoid the combinatorial explosion problem for the potential test matrix.
--
cc/ @alexcrichton @fitzgen I'm opening this a draft, but feel free to leave any feedback according the discussion in the Cranelift meeting. I'm a bit concerned because I don't really have a good answer to the combinatorial problem of the test matrix, at least initially I suspect this won't be an issue necessarily, but if this macro grows in functionality I think it might be a problem.