Stream: git-wasmtime

Topic: wasmtime / PR #7182 Refactor the test-programs test suite

Wasmtime GitHub notifications bot (Oct 07 2023 at 00:25):

alexcrichton opened PR #7182 from alexcrichton:refactor-test-programs to bytecodealliance:main:

This commit is a large refactoring that reorganizes test-programs and how we tests wasms in Wasmtime. Often writing tests requires complicated interactions with the guest which can't be done via hand-written *.wat syntax and requires a compiler to get engaged. For this purpose Wasmtime currently has the crates/test-programs/* test suite which builds files from source and then runs the tests. This has been somewhat cumbersome in the past though and it's not been easy to extend this over time, so this commit attempts to address this.

The scheme implemented in this PR looks like:

• All wasm test programs live in crates/test-programs/src/bin/*.rs. All of them, no exceptions.

• Wasm tests have shared support located at crates/test-programs/src/lib.rs and its submodules, such as bindings generation for WASI.

• Wasm tests are built by a new crates/test-programs/artifacts crate. This crate compiles modules and additionally creates components for all test programs. The crate itself only records the path to these outputs and a small amount of testing support, but otherwise doesn't interact with wasmtime-the-crate itself.

• All tests in crates/test-programs/tests/*.rs have moved. For example wasi-http tests now live at crates/wasi-http/tests/*.rs. Legacy tests of wasi-common now live at crates/wasi-common/tests/*.rs. Modern tests for preview2 live at crates/wasi/tests/*.rs.

• Wasm tests are bucketed based on their filename prefix. For example preview1_* is tested in wasi-common and wasmtime-wasi. The preview2_* prefix is only tested with wasmtime-wasi, however.

• A new cli_* prefix is used to execute tests as part of tests/all/main.rs. This is a new submodule in tests/all/cli_tests.rs which executes these components on the command line. Many old "command" tests were migrated here.

• Helper macros are generated to assert that a test suite is run in its entirety. This way if a preview1_* test is added it's asserted to get added to both wasi-common and wasmtime-wasi in the various modes they run tests.

Overall this moved a number of tests around and refactored some edges of the tests, but this should not lose any tests (except one that wasn't actually testing anything). Additionally the hope is that it's much easier to add tests in the future. The process is to add a new file in crates/test-programs/src/bin/*.rs named appropriately. For example a preview2 executable is preview2_* and a CLI tests is cli_*. When building the test suite an error is generated in the appropriate module then of "please write a test here", and then a test is written in the same manner as the other tests in the module.

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Oct 07 2023 at 00:25):

alexcrichton requested fitzgen for a review on PR #7182.

Wasmtime GitHub notifications bot (Oct 07 2023 at 00:25):

alexcrichton requested wasmtime-default-reviewers for a review on PR #7182.

Wasmtime GitHub notifications bot (Oct 07 2023 at 00:25):

alexcrichton requested wasmtime-core-reviewers for a review on PR #7182.

Wasmtime GitHub notifications bot (Oct 07 2023 at 00:28):

alexcrichton updated PR #7182.

Wasmtime GitHub notifications bot (Oct 07 2023 at 00:28):

alexcrichton updated PR #7182.

Wasmtime GitHub notifications bot (Oct 07 2023 at 00:38):

alexcrichton updated PR #7182.

Wasmtime GitHub notifications bot (Oct 07 2023 at 00:53):

alexcrichton updated PR #7182.

Wasmtime GitHub notifications bot (Oct 09 2023 at 03:09):

alexcrichton updated PR #7182.

Wasmtime GitHub notifications bot (Oct 09 2023 at 14:10):

alexcrichton updated PR #7182.

Wasmtime GitHub notifications bot (Oct 09 2023 at 14:50):

alexcrichton updated PR #7182.

Wasmtime GitHub notifications bot (Oct 09 2023 at 15:08):

alexcrichton updated PR #7182.

Wasmtime GitHub notifications bot (Oct 09 2023 at 15:17):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Oct 09 2023 at 15:17):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Oct 09 2023 at 15:17):

alexcrichton created PR review comment:

This is a bug somewhere and I'm not sure what, but my guess is the clap processing of arguments or something like that. I need to bottom this out but it's unrelated to this PR in that this PR doesn't introduce a new failure mode, it only exposes an existing one.

Wasmtime GitHub notifications bot (Oct 09 2023 at 15:30):

alexcrichton updated PR #7182.

Wasmtime GitHub notifications bot (Oct 09 2023 at 16:32):

alexcrichton updated PR #7182.

Wasmtime GitHub notifications bot (Oct 09 2023 at 16:37):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Oct 09 2023 at 16:37):

alexcrichton created PR review comment:

Ok I have now since fixed this. The underlying issue was present before this PR but it's only exposed in this PR so I lumped th efix in here. The problem was that clap's parsing of PathBuf requires the value to be non-empty and we were modeling all of the arguments to wasm as Vec<PathBuf>. I've switched to Vec<OsString> to enable passing through empty arguments.

Wasmtime GitHub notifications bot (Oct 09 2023 at 17:27):

alexcrichton requested elliottt for a review on PR #7182.

Wasmtime GitHub notifications bot (Oct 09 2023 at 18:24):

elliottt submitted PR review:

This is awesome, thank you!

Wasmtime GitHub notifications bot (Oct 09 2023 at 18:24):

elliottt submitted PR review:

This is awesome, thank you!

Wasmtime GitHub notifications bot (Oct 09 2023 at 18:24):

elliottt created PR review comment:

Why do we skip the first argument now?

Wasmtime GitHub notifications bot (Oct 09 2023 at 18:24):

elliottt created PR review comment:

I copied over the same comment from the reactor tests.

// Technically this should not be here for a proxy, but given the current
// framework for tests it's required since this file is built as a `bin`
fn main() {}

Wasmtime GitHub notifications bot (Oct 09 2023 at 18:31):

alexcrichton updated PR #7182.

Wasmtime GitHub notifications bot (Oct 09 2023 at 18:32):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Oct 09 2023 at 18:32):

alexcrichton created PR review comment:

Previously this test was executed through the embedder API rather than the CLI. In that situation we had precise control over the arguments and on the CLI the input executable is implicitly the first argument. This is skipped now due to the runner of the test being moved to a different location basically.

Wasmtime GitHub notifications bot (Oct 09 2023 at 18:32):

alexcrichton has enabled auto merge for PR #7182.

Wasmtime GitHub notifications bot (Oct 09 2023 at 20:07):

alexcrichton merged PR #7182.

Last updated: Nov 22 2024 at 17:03 UTC