wasmtime / PR #8622 wasmtime: Introduce the `test-macros`... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #8622 wasmtime: Introduce the `test-macros`...

Wasmtime GitHub notifications bot (May 15 2024 at 00:12):

saulecabrera opened PR #8622 from saulecabrera:add-test-macros to bytecodealliance:main:

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

Wasmtime GitHub notifications bot (May 15 2024 at 00:14):

saulecabrera updated PR #8622.

Wasmtime GitHub notifications bot (May 15 2024 at 00:16):

saulecabrera edited PR #8622:

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

Wasmtime GitHub notifications bot (May 15 2024 at 10:36):

saulecabrera updated PR #8622.

Wasmtime GitHub notifications bot (May 15 2024 at 10:38):

saulecabrera updated PR #8622.

Wasmtime GitHub notifications bot (May 15 2024 at 10:39):

saulecabrera updated PR #8622.

Wasmtime GitHub notifications bot (May 15 2024 at 11:52):

saulecabrera updated PR #8622.

Wasmtime GitHub notifications bot (May 15 2024 at 12:22):

saulecabrera updated PR #8622.

Wasmtime GitHub notifications bot (May 15 2024 at 15:18):

fitzgen commented on PR #8622:

Excited for this! Will take a look today

Wasmtime GitHub notifications bot (May 15 2024 at 17:19):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 15 2024 at 17:19):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 15 2024 at 17:19):

alexcrichton created PR review comment:

I might recommend replacing strategies with something like config: TestConfig which internally has Vec<(String,Ident)> or similar to make this more easily growable in the future to multiple configuration options.

Wasmtime GitHub notifications bot (May 15 2024 at 17:19):

alexcrichton created PR review comment:

Mind also adding publish = false here? (may also require changes to publish.rs, I sort of forget)

Also, to handle the clippy issues on CI you can add:
[lints]
workspace = true
to inherit the defaults of the workspace which allows clippy lints by default.

Wasmtime GitHub notifications bot (May 15 2024 at 17:19):

alexcrichton created PR review 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).

Wasmtime GitHub notifications bot (May 15 2024 at 17:19):

alexcrichton created PR review comment:

It might be good to replace this with parse_nested_meta perhaps?

Wasmtime GitHub notifications bot (May 15 2024 at 18:08):

fitzgen submitted PR review:

Definitely going in a good direction!

Wasmtime GitHub notifications bot (May 15 2024 at 18:08):

fitzgen submitted PR review:

Definitely going in a good direction!

Wasmtime GitHub notifications bot (May 15 2024 at 18:08):

fitzgen created PR review comment:

This shouldn't be necessary because a crate in the workspace depends on this crate.

Wasmtime GitHub notifications bot (May 15 2024 at 18:08):

fitzgen created PR review comment:

Yep, this is exactly what I was imagining usage would look like :+1:

Wasmtime GitHub notifications bot (May 24 2024 at 18:54):

saulecabrera updated PR #8622.

Wasmtime GitHub notifications bot (May 24 2024 at 18:54):

saulecabrera has marked PR #8622 as ready for review.

Wasmtime GitHub notifications bot (May 24 2024 at 18:54):

saulecabrera requested wasmtime-core-reviewers for a review on PR #8622.

Wasmtime GitHub notifications bot (May 24 2024 at 18:54):

saulecabrera requested pchickey for a review on PR #8622.

Wasmtime GitHub notifications bot (May 24 2024 at 18:54):

saulecabrera requested wasmtime-default-reviewers for a review on PR #8622.

Wasmtime GitHub notifications bot (May 24 2024 at 18:54):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (May 24 2024 at 18:54):

saulecabrera created PR review comment:

Done, thanks!

Wasmtime GitHub notifications bot (May 24 2024 at 18:54):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (May 24 2024 at 18:54):

saulecabrera created PR review comment:

Done!

Wasmtime GitHub notifications bot (May 24 2024 at 18:55):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (May 24 2024 at 18:55):

saulecabrera created PR review comment:

Good point. Took me a bit to figure out, but I was able to make this idea work by introduce a partial function parser as per your suggestion. Let me know what you think!

Wasmtime GitHub notifications bot (May 24 2024 at 18:55):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (May 24 2024 at 18:55):

saulecabrera created PR review comment:

Done!

Wasmtime GitHub notifications bot (May 24 2024 at 18:55):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (May 24 2024 at 18:55):

saulecabrera created PR review comment:

Done, thanks!

Wasmtime GitHub notifications bot (May 24 2024 at 18:56):

saulecabrera commented on PR #8622:

Thanks for the review Alex and Nick. I've addressed all the comments, I think this is ready for another round of reviews.

Wasmtime GitHub notifications bot (May 24 2024 at 18:56):

saulecabrera edited a comment on PR #8622:

Thanks for the reviews Alex and Nick. I've addressed all the comments, I think this is ready for another round of reviews.

Wasmtime GitHub notifications bot (May 24 2024 at 18:59):

saulecabrera edited PR review comment.

Wasmtime GitHub notifications bot (May 28 2024 at 14:42):

alexcrichton submitted PR review:

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?

Wasmtime GitHub notifications bot (May 28 2024 at 14:57):

alexcrichton merged PR #8622.

Wasmtime GitHub notifications bot (May 29 2024 at 13:44):

saulecabrera commented on PR #8622:

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.

Last updated: Dec 23 2024 at 12:05 UTC