Repository: bytecodealliance/wasm-tools
Pull request #1782 (2024-09-11 20:15:58)

Support multi-line run/fail directives in CLI tests
Also add a `readme.wat` test explainer as to what's going on.


Patch:
diff --git a/Cargo.lock b/Cargo.lock
index 95e51ef0..05dc757d 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -1760,6 +1760,7 @@ dependencies = [
  "cpp_demangle",
  "env_logger",
  "gimli 0.30.0",
+ "indexmap 2.4.0",
  "is_executable",
  "libtest-mimic",
  "log",
diff --git a/Cargo.toml b/Cargo.toml
index 45318fb1..a65bf4d5 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -178,6 +178,7 @@ tempfile = "3.1"
 wast = { path = 'crates/wast' }
 pretty_assertions = { workspace = true }
 libtest-mimic = { workspace = true }
+indexmap = { workspace = true }
 
 [[test]]
 name = "cli"
diff --git a/crates/wit-component/src/printing.rs b/crates/wit-component/src/printing.rs
index 769fc04d..49e4058b 100644
--- a/crates/wit-component/src/printing.rs
+++ b/crates/wit-component/src/printing.rs
@@ -5,7 +5,6 @@ use std::mem;
 use wit_parser::*;
 
 // NB: keep in sync with `crates/wit-parser/src/ast/lex.rs`
-const PRINT_SEMICOLONS_DEFAULT: bool = true;
 const PRINT_F32_F64_DEFAULT: bool = false;
 
 /// A utility for printing WebAssembly interface definitions to a string.
@@ -19,7 +18,6 @@ pub struct WitPrinter {
     // Whether to print doc comments.
     emit_docs: bool,
 
-    print_semicolons: bool,
     print_f32_f64: bool,
 }
 
@@ -29,10 +27,6 @@ impl Default for WitPrinter {
             output: Default::default(),
             any_items: false,
             emit_docs: true,
-            print_semicolons: match std::env::var("WIT_REQUIRE_SEMICOLONS") {
-                Ok(s) => s == "1",
-                Err(_) => PRINT_SEMICOLONS_DEFAULT,
-            },
             print_f32_f64: match std::env::var("WIT_REQUIRE_F32_F64") {
                 Ok(s) => s == "1",
                 Err(_) => PRINT_F32_F64_DEFAULT,
@@ -119,9 +113,7 @@ impl WitPrinter {
     }
 
     fn print_semicolon(&mut self) {
-        if self.print_semicolons {
-            self.output.push_str(";");
-        }
+        self.output.push_str(";");
     }
 
     fn new_item(&mut self) {
diff --git a/crates/wit-parser/src/ast.rs b/crates/wit-parser/src/ast.rs
index 37e25046..9d8d7716 100644
--- a/crates/wit-parser/src/ast.rs
+++ b/crates/wit-parser/src/ast.rs
@@ -1624,7 +1624,6 @@ fn eat_id(tokens: &mut Tokenizer<'_>, expected: &str) -> Result<Span> {
 pub struct SourceMap {
     sources: Vec<Source>,
     offset: u32,
-    require_semicolons: Option<bool>,
     require_f32_f64: Option<bool>,
 }
 
@@ -1641,11 +1640,6 @@ impl SourceMap {
         SourceMap::default()
     }
 
-    #[doc(hidden)] // NB: only here for a transitionary period
-    pub fn set_require_semicolons(&mut self, enable: bool) {
-        self.require_semicolons = Some(enable);
-    }
-
     #[doc(hidden)] // NB: only here for a transitionary period
     pub fn set_require_f32_f64(&mut self, enable: bool) {
         self.require_f32_f64 = Some(enable);
@@ -1702,7 +1696,6 @@ impl SourceMap {
                     // passing through the source to get tokenized.
                     &src.contents[..src.contents.len() - 1],
                     src.offset,
-                    self.require_semicolons,
                     self.require_f32_f64,
                 )
                 .with_context(|| format!("failed to tokenize path: {}", src.path.display()))?;
@@ -1872,7 +1865,7 @@ pub enum ParsedUsePath {
 }
 
 pub fn parse_use_path(s: &str) -> Result<ParsedUsePath> {
-    let mut tokens = Tokenizer::new(s, 0, Some(true), None)?;
+    let mut tokens = Tokenizer::new(s, 0, None)?;
     let path = UsePath::parse(&mut tokens)?;
     if tokens.next()?.is_some() {
         bail!("trailing tokens in path specifier");
diff --git a/crates/wit-parser/src/ast/lex.rs b/crates/wit-parser/src/ast/lex.rs
index 93ad6008..76886d85 100644
--- a/crates/wit-parser/src/ast/lex.rs
+++ b/crates/wit-parser/src/ast/lex.rs
@@ -11,7 +11,6 @@ pub struct Tokenizer<'a> {
     input: &'a str,
     span_offset: u32,
     chars: CrlfFold<'a>,
-    require_semicolons: bool,
     require_f32_f64: bool,
 }
 
@@ -117,14 +116,12 @@ pub enum Error {
 }
 
 // NB: keep in sync with `crates/wit-component/src/printing.rs`.
-const REQUIRE_SEMICOLONS_BY_DEFAULT: bool = true;
 const REQUIRE_F32_F64_BY_DEFAULT: bool = false;
 
 impl<'a> Tokenizer<'a> {
     pub fn new(
         input: &'a str,
         span_offset: u32,
-        require_semicolons: Option<bool>,
         require_f32_f64: Option<bool>,
     ) -> Result<Tokenizer<'a>> {
         detect_invalid_input(input)?;
@@ -135,12 +132,6 @@ impl<'a> Tokenizer<'a> {
             chars: CrlfFold {
                 chars: input.char_indices(),
             },
-            require_semicolons: require_semicolons.unwrap_or_else(|| {
-                match std::env::var("WIT_REQUIRE_SEMICOLONS") {
-                    Ok(s) => s == "1",
-                    Err(_) => REQUIRE_SEMICOLONS_BY_DEFAULT,
-                }
-            }),
             require_f32_f64: require_f32_f64.unwrap_or_else(|| {
                 match std::env::var("WIT_REQUIRE_F32_F64") {
                     Ok(s) => s == "1",
@@ -154,11 +145,7 @@ impl<'a> Tokenizer<'a> {
     }
 
     pub fn expect_semicolon(&mut self) -> Result<()> {
-        if self.require_semicolons {
-            self.expect(Token::Semicolon)?;
-        } else {
-            self.eat(Token::Semicolon)?;
-        }
+        self.expect(Token::Semicolon)?;
         Ok(())
     }
 
@@ -668,7 +655,7 @@ fn test_validate_id() {
 #[test]
 fn test_tokenizer() {
     fn collect(s: &str) -> Result<Vec<Token>> {
-        let mut t = Tokenizer::new(s, 0, Some(true), None)?;
+        let mut t = Tokenizer::new(s, 0, None)?;
         let mut tokens = Vec::new();
         while let Some(token) = t.next()? {
             tokens.push(token.1);


Test patch:
diff --git a/tests/cli.rs b/tests/cli.rs
index 96427e12..6ecca33f 100644
--- a/tests/cli.rs
+++ b/tests/cli.rs
@@ -1,31 +1,12 @@
 //! A test suite to test the `wasm-tools` CLI itself.
 //!
-//! This test suite will look for `*.wat` files in the `tests/cli/**` directory,
-//! recursively. Each wat file must have a directive of the form:
-//!
-//!     ;; RUN: ...
-//!
-//! where `...` is a space-separate set of command to pass to the `wasm-tools`
-//! CLI. The `%` argument is replaced with the path to the current file. For
-//! example:
-//!
-//!     ;; RUN: dump %
-//!
-//! would execute `wasm-tools dump the-current-file.wat`. The `cli` directory
-//! additionally contains `*.stdout` and `*.stderr` files to assert the output
-//! of the subcommand. Files are not present if the stdout/stderr are empty.
-//!
-//! This also supports a limited form of piping along the lines of:
-//!
-//!     ;; RUN: strip % | objdump
-//!
-//! where a `|` will execute the first subcommand and pipe its stdout into the
-//! stdin of the next command.
-//!
-//! Use `BLESS=1` in the environment to auto-update expectation files. Be sure
-//! to look at the diff!
+//! This test suite will look for `*.wat` and `*.wit` files in the
+//! `tests/cli/**` directory, recursively. For more information about supported
+//! directives and features of this test suite see the `tests/cli/readme.wat`
+//! file which has an explanatory comment at the top for what's going on.
 
-use anyhow::{anyhow, bail, Context, Result};
+use anyhow::{bail, Context, Result};
+use indexmap::IndexMap;
 use libtest_mimic::{Arguments, Trial};
 use pretty_assertions::StrComparison;
 use std::env;
@@ -59,54 +40,96 @@ fn main() {
     libtest_mimic::run(&args, trials).exit();
 }
 
-fn wasm_tools_exe() -> Command {
-    Command::new(env!("CARGO_BIN_EXE_wasm-tools"))
-}
-
 fn run_test(test: &Path, bless: bool) -> Result<()> {
     let contents = std::fs::read_to_string(test)?;
-    let (line, should_fail) = contents
+
+    let mut directives = contents
         .lines()
-        .filter_map(|l| {
-            let run = l.strip_prefix(";; RUN: ").or(l.strip_prefix("// RUN: "));
-            let fail = l.strip_prefix(";; FAIL: ").or(l.strip_prefix("// FAIL: "));
-            run.map(|l| (l, false)).or(fail.map(|l| (l, true)))
-        })
-        .next()
-        .ok_or_else(|| anyhow!("no line found with `;; RUN: ` directive"))?;
-
-    let mut cmd = wasm_tools_exe();
-    let mut stdin = None;
-    let tempdir = TempDir::new()?;
-    for arg in line.split_whitespace() {
-        let arg = arg.replace("%tmpdir", tempdir.path().to_str().unwrap());
-        if arg == "|" {
-            let output = execute(&mut cmd, stdin.as_deref(), false)?;
-            stdin = Some(output.stdout);
-            cmd = wasm_tools_exe();
-        } else if arg == "%" {
-            cmd.arg(test);
-        } else {
-            cmd.arg(arg);
+        .enumerate()
+        .filter(|(_, l)| !l.is_empty())
+        .filter_map(|(i, l)| {
+            l.strip_prefix("// ")
+                .or(l.strip_prefix(";; "))
+                .map(|l| (i + 1, l))
+        });
+
+    let mut commands = IndexMap::new();
+
+    while let Some((i, line)) = directives.next() {
+        let run = line.strip_prefix("RUN");
+        let fail = line.strip_prefix("FAIL");
+        let (directive, should_fail) = match run.map(|l| (l, false)).or(fail.map(|l| (l, true))) {
+            Some(pair) => pair,
+            None => continue,
+        };
+        let (cmd, name) = match directive.strip_prefix("[") {
+            Some(prefix) => match prefix.find("]:") {
+                Some(i) => (&prefix[i + 2..], &prefix[..i]),
+                None => bail!("line {i}: failed to find `]:` after `[`"),
+            },
+            None => match directive.strip_prefix(":") {
+                Some(cmd) => (cmd, ""),
+                None => bail!("line {i}: failed to find `:` after `RUN` or `FAIL`"),
+            },
+        };
+        let mut cmd = cmd.to_string();
+        while cmd.ends_with("\\") {
+            cmd.pop();
+            match directives.next() {
+                Some((_, line)) => cmd.push_str(line),
+                None => bail!("line {i}: directive ends in `\\` but nothing on next line"),
+            }
+        }
+
+        match commands.insert(name, (cmd, should_fail)) {
+            Some(_) => bail!("line {i}: duplicate directive named {name:?}"),
+            None => {}
         }
     }
 
-    let output = execute(&mut cmd, stdin.as_deref(), should_fail)?;
-    let extension = test.extension().unwrap().to_str().unwrap();
-    assert_output(
-        bless,
-        &output.stdout,
-        &test.with_extension(&format!("{extension}.stdout")),
-        &tempdir,
-    )
-    .context("failed to check stdout expectation (auto-update with BLESS=1)")?;
-    assert_output(
-        bless,
-        &output.stderr,
-        &test.with_extension(&format!("{extension}.stderr")),
-        &tempdir,
-    )
-    .context("failed to check stderr expectation (auto-update with BLESS=1)")?;
+    if commands.is_empty() {
+        bail!("failed to find `// RUN: ...` or `// FAIL: ...` at the top of this file");
+    }
+    let exe = Path::new(env!("CARGO_BIN_EXE_wasm-tools"));
+    let tempdir = TempDir::new_in(exe.parent().unwrap())?;
+    for (name, (line, should_fail)) in commands {
+        let mut cmd = Command::new(exe);
+        let mut stdin = None;
+        for arg in line.split_whitespace() {
+            let arg = arg.replace("%tmpdir", tempdir.path().to_str().unwrap());
+            if arg == "|" {
+                let output = execute(&mut cmd, stdin.as_deref(), false)?;
+                stdin = Some(output.stdout);
+                cmd = Command::new(exe);
+            } else if arg == "%" {
+                cmd.arg(test);
+            } else {
+                cmd.arg(arg);
+            }
+        }
+
+        let output = execute(&mut cmd, stdin.as_deref(), should_fail)?;
+        let extension = test.extension().unwrap().to_str().unwrap();
+        let extension = if name.is_empty() {
+            extension.to_string()
+        } else {
+            format!("{extension}.{name}")
+        };
+        assert_output(
+            bless,
+            &output.stdout,
+            &test.with_extension(&format!("{extension}.stdout")),
+            &tempdir,
+        )
+        .context("failed to check stdout expectation (auto-update with BLESS=1)")?;
+        assert_output(
+            bless,
+            &output.stderr,
+            &test.with_extension(&format!("{extension}.stderr")),
+            &tempdir,
+        )
+        .context("failed to check stderr expectation (auto-update with BLESS=1)")?;
+    }
     Ok(())
 }
 
diff --git a/tests/cli/readme.wat b/tests/cli/readme.wat
new file mode 100644
index 00000000..600aae76
--- /dev/null
+++ b/tests/cli/readme.wat
@@ -0,0 +1,76 @@
+;; This is intended to be a self-documenting test which explains what's
+;; possible in directives for this test suite in `tests/cli/*`. The purpose of
+;; this test suite is to make it as easy as dropping a file in this directory to
+;; test the `wasm-tools` CLI tool and its subcommands. The test file itself is
+;; generally the input to the test and what's being tested will be present in
+;; comments at the top of the file with directives.
+;;
+;; All test directives must come in comments at the start of the file:
+;;
+;; RUN: validate %
+;;
+;; The `RUN` prefix indicates that the specified `wasm-tools` subcommand should
+;; be executed. It's possible to have more than one test in a file by having
+;; named directives such as:
+;;
+;; RUN[validate-again]: validate %
+;;
+;; Directive names must be unique, so using `validate-again` would not be valid.
+;; Additionally you can't use an unprefixed directive more than once so using
+;; `RUN: ...` here again would not be allowed for example.
+;;
+;; You can also use the `FAIL` directive to indicate that the subcommand should
+;; fail rather than succeed.
+;;
+;; FAIL[should-fail]: validate % --features=-simd
+;;
+;; As you can see directives can have comments around them. Directives are
+;; identified as comment lines starting with `RUN` or `FAIL`.
+;;
+;; Within directives there are a few feature. First as you've seen the `%` value
+;; will be substituted with the current filename which means:
+;;
+;; RUN[subst]: validate %
+;;
+;; means to run `wasm-tools validate tests/cli/readme.wat` and test the result
+;; is successful.
+;;
+;; You can additionally use `|` to pipe commands together by feeding the stdout
+;; of the previous command into the stdin of the next command.
+;;
+;; RUN[pipe]: print % | validate
+;;
+;; Note that when piping commands the intermediate commands before the final
+;; one, in this case `print` being the intermediate, must all succeed.
+;;
+;; Tests also assert the stdout/stderr of the command being tested. For example
+;; if printing is tested:
+;;
+;; RUN[print]: print %
+;;
+;; then this tests that `tests/cli/readme.wat.print.stdout` is the result of
+;; `wasm-tools print tests/cli/readme.wat`. Note that this can be tedious to
+;; update so you can use the environment variable `BLESS=1` to automatically
+;; update all test assertions. This can then be reviewed after the test is
+;; passing for accuracy.
+;;
+;; Each test additionally can have a temporary directory available to it which
+;; is accessible with the `%tmpdir` substitution. For example:
+;;
+;; RUN[tmpdir]: print % -o %tmpdir/foo.wat | validate %tmpdir/foo.wat
+;;
+;; Note that temporary directories are persisted across tests in the same file,
+;; but different files all get different temporary directories.
+;;
+;; You can also split commands across multiple lines:
+;;
+;; RUN[multiline]: print % | \
+;;                 validate
+;;
+;; here the `\` character is deleted and the next line is concatenated.
+
+
+;; this is the contents of the test, mostly empty in this case.
+(module
+  (type (func (result v128)))
+)
diff --git a/tests/cli/readme.wat.print.stdout b/tests/cli/readme.wat.print.stdout
new file mode 100644
index 00000000..c8cd2b5e
--- /dev/null
+++ b/tests/cli/readme.wat.print.stdout
@@ -0,0 +1,3 @@
+(module
+  (type (;0;) (func (result v128)))
+)
diff --git a/tests/cli/readme.wat.should-fail.stderr b/tests/cli/readme.wat.should-fail.stderr
new file mode 100644
index 00000000..45ead379
--- /dev/null
+++ b/tests/cli/readme.wat.should-fail.stderr
@@ -0,0 +1,1 @@
+error: SIMD support is not enabled (at offset 0xb)