🏃 Add a run-mode that executes the input program once and then exits #211
Conversation
You already anticipated my primary feedback 😆 The idea of using I played around with this using [target.wasm32-wasi]
runner = "wasmtime --"
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn it_works() {
let result = add(2, 2);
assert_eq!(result, 4);
}
#[test]
fn it_doesnt() {
panic!()
}
}With regular ❯ cargo test --target wasm32-wasi
Finished test [unoptimized + debuginfo] target(s) in 0.00s
Running unittests src/lib.rs (target/wasm32-wasi/debug/deps/test_test-77eb6ad9e93c9b9a.wasm)
On the other hand, ❯ cargo nextest run --target wasm32-wasi
Finished test [unoptimized + debuginfo] target(s) in 0.01s
info: for the target platform, using target runner `wasmtime --` defined by `target.wasm32-wasi.runner` within `/tmp/test-test/.cargo/config`
Starting 2 tests across 1 binaries
FAIL [ 0.046s] test-test tests::it_doesnt
I'd like us to focus on supporting a generic but comprehensive interface for executing arbitrary programs with arguments and environment variables, much like
Let's leave this for a future iteration if people demand it.
I think it'd be reasonable to support the geolocation, dictionaries, and object store fields. But these would all be fairly easy to tack on after landing basic execution support. |
|
@acfoltzer There are still some rough edges here, but how is this general shape? running aforementioned rough edges:
|
|
@itsrainy that looks great, you nailed it!
Are you building Viceroy in
Yeah, that seems like the right call for this mode. The stdio handles should look pretty much how they would if we were running the program natively. |
Nope, I wasn't! That made it much much faster (<5s total for both the above). |
itsrainy
force-pushed
the
rainy/test-runner
branch
from
2d6b1cf to
e4266dd
Compare
itsrainy
changed the title
cargo test
itsrainy
force-pushed
the
rainy/test-runner
branch
from
e4266dd to
ec7e3a0
Compare
itsrainy
changed the title
|
I added some tests and cleaned up the PR description and doc comments and I think this is ready for another round of review |
of the given binary, passing along any parameters following `--`
itsrainy
force-pushed
the
rainy/test-runner
branch
from
faa0913 to
b0d710e
Compare
itsrainy
force-pushed
the
rainy/test-runner
branch
from
b0d710e to
5f87e0a
Compare
|
@itsrainy just want to confirm that this is ready for review, given the commit activity since your last message. Also, I'm going to untag other reviewers to make it clear that I own the review for this one |
|
@acfoltzer yes it's ready! The commit activity was rebasing/force-pushing, not any actual development :) |
There was a problem hiding this comment.
Excellent work, @itsrainy! The fundamental implementation of the feature is shippable imo. As I play with the interface, though, I'm coming to the conclusion that we should start using clap subcommands in the CLI rather than relying on particular combinations of flags.
As of this review, it's possible to run Viceroy with nonsensical argument combinations, for example:
# Passing ignored Wasm args to web server creation
$ viceroy -C fastly.toml my_app.wasm -- these_args are_ignored
# Passing ignored server args to `--run`
$ viceroy -C fastly.toml --run my_app.wasm --addr localhost:12345 --log-stdout --log-stderr
I'd like to get some buy-in from other Viceroy devs before embarking on this significant change, but my proposal is:
-
Introduce
serveandrunsubcommands to the Viceroy CLI -
Keep arguments applicable to both modes in a toplevel struct (probably marked as
globalso they can come before or after the subcommand), and move mode-specific arguments to theserveandrunenums -
Make
servethe default behavior in order to maintain compatibility with drivers of the existing interface like thefastlyCLI.N.B. I am not sure whether this is straightforward to achieve with
clap, so may not be worth the effort if we coordinate sufficiently the CLI to have it perform invocations appropriately for the version of Viceroy it detects.
I know this is a fair amount extra on top of what you've put together here, but I think it'll be a lot easier to onboard new users to this feature if we have more clarity in the interface.
(edit: some examples to illustrate the desired interface)
$ viceroy -C fastly.toml serve my_app.wasm -v2 --addr localhost:12345 --log-stdout --log-stderr
$ viceroy -C fastly.toml run my_app.wasm -v2 -- wasm_arg1 another_wasm_arg
There was a problem hiding this comment.
Based on offline discussion, we'll be:
- Moving ahead with the CLI as-implemented in this PR
- Following up with a PR dedicated to reworking the CLI interface with subcommands
- Marking as experimental and/or omitting the run interface from the changelog, should a Viceroy release be necessary between the two PRs
|
CLI follow-up ticket: #222 |
cli/src/main.rs
Outdated
| @@ -80,7 +79,11 @@ pub async fn main() -> ExitCode { | |||
| pub async fn run_wasm_main(opts: Opts) -> Result<(), anyhow::Error> { | |||
| // Load the wasm module into an execution context | |||
| let ctx = create_execution_context(&opts).await?; | |||
| ctx.run_main(opts.wasm_args()).await | |||
| ctx.run_main( | |||
| opts.input().file_stem().unwrap().to_str().unwrap(), | |||
There was a problem hiding this comment.
I think the second unwrap here could cause us problems since Windows filenames are, iirc, UTF-16. Can you use to_string_lossy instead?
The first unwrap should be okay, because presumably check_module would fail if this argument was a directory. It might be worth saying so explicitly, though.
There was a problem hiding this comment.
@acfoltzer I think you're right that check_module would catch that while parsing CLI args. I added a message here just to make it a bit more clear what is going on though.
There was a problem hiding this comment.
also I'm still fairly new to Rust, so if there's a different way that you would have written this, please let me know :)
There was a problem hiding this comment.
I think this is fine, especially since it'd come up right away when executing a command rather than lying in wait to bring down a long-running system. I could imagine introducing a new type for the validated filename just to have something tracked by the compiler to this point assuring us that the filename exists, but it'd honestly be overkill for this situation.
|
@itsrainy @acfoltzer sorry for adding essentially a support request to this PR but... does this now permits to run tests in wasm for Fastly compute Rust projects? I would be incredibly interested in even the vaguest description of how to use this 🙏 |
|
Hi @yannh, the followup CLI work described in comments here is going to be changing the interface to this functionality, so we're holding off on writing up public documents until then. The PR description @itsrainy wrote above is an accurate description of how to currently run tests for Rust SDK programs. |
This introduces a "run-mode" to Viceroy that will allow us to use it as a test runner. The goal is to allow developers of C@E services to use Viceroy to run unit tests that rely on things provided by Fastly's SDKs. This is based off of a proof-of-concept that @acfoltzer put together a few months ago 🙏
how to use it
To run a wasm program's
_startfunction and then exit, the invocation looks like this:This will set up the context described in
fastly.tomland all the wasm context expected by a C@E service, and then run_start. If you would like to pass arguments to the wasm program, you can specify those after a--on the command line, like so:as a Rust developer, to use this as a test runner you'll need to do the following:
.cargo/config:cargo nextest run:why
cargo-nextestand not justcargo test?The above example would work if executed with cargo test. Where that breaks down is if any tests fail. There is no way to recover from a panic in wasm, so test execution would halt as soon as the first test failure occurs. Because of this, we need each test to be executed in its own wasm instance and have the results aggregated to report overall success/failure. As @acfoltzer pointed out below, this is what cargo-nextest does.