From 38030ac6c46922aaab8f54568f4b817adad7a261 Mon Sep 17 00:00:00 2001 From: xxchan Date: Thu, 11 Apr 2024 16:52:02 +0800 Subject: [PATCH] feat: support matching stdout for system (#206) --- CHANGELOG.md | 17 +++ Cargo.lock | 6 +- Cargo.toml | 2 +- sqllogictest-bin/Cargo.toml | 4 +- sqllogictest-bin/src/engines.rs | 5 +- sqllogictest-engines/Cargo.toml | 2 +- sqllogictest-engines/src/external.rs | 6 +- sqllogictest-engines/src/postgres/extended.rs | 6 +- sqllogictest-engines/src/postgres/simple.rs | 6 +- sqllogictest/src/parser.rs | 36 ++++-- sqllogictest/src/runner.rs | 118 +++++++++++++++--- tests/system_command/system_command.rs | 16 ++- tests/system_command/system_command.slt | 29 +++++ .../system_command/system_command_fail_2.slt | 5 + 14 files changed, 215 insertions(+), 43 deletions(-) create mode 100644 tests/system_command/system_command_fail_2.slt diff --git a/CHANGELOG.md b/CHANGELOG.md index 907de43..ac20304 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,23 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased +## [0.20.0] - 2024-04-08 + +* Show stdout, stderr when `system` command fails. +* Support matching stdout for `system` + ``` + system ok + echo "Hello, world!" + ---- + Hello, world! + ``` + Currently, only exact match is supported. Besides, the output cannot contain more than one blank lines in between. The record ends with two consecutive blank lines. + + Some minor **Breaking changes**: + - Add field `stdout` to `parser::Record::System` and `runner::RecordOutput::System`, and mark them as `#[non_exhaustive]`. + - Change trait method `AsyncDB::run_command`'s return type from `std::process::ExitStatus` to `std::process::Output`. + + ## [0.19.1] - 2024-01-04 * parser: `include` now returns error if no file is matched. diff --git a/Cargo.lock b/Cargo.lock index 8e6cbe6..cfa41c5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1403,7 +1403,7 @@ dependencies = [ [[package]] name = "sqllogictest" -version = "0.19.1" +version = "0.20.0" dependencies = [ "async-trait", "educe", @@ -1426,7 +1426,7 @@ dependencies = [ [[package]] name = "sqllogictest-bin" -version = "0.19.1" +version = "0.20.0" dependencies = [ "anyhow", "async-trait", @@ -1447,7 +1447,7 @@ dependencies = [ [[package]] name = "sqllogictest-engines" -version = "0.19.1" +version = "0.20.0" dependencies = [ "async-trait", "bytes", diff --git a/Cargo.toml b/Cargo.toml index 826e541..f7ed8f0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -3,7 +3,7 @@ resolver = "2" members = ["sqllogictest", "sqllogictest-bin", "sqllogictest-engines", "tests"] [workspace.package] -version = "0.19.1" +version = "0.20.0" edition = "2021" homepage = "https://github.com/risinglightdb/sqllogictest-rs" keywords = ["sql", "database", "parser", "cli"] diff --git a/sqllogictest-bin/Cargo.toml b/sqllogictest-bin/Cargo.toml index f602c29..fbca6eb 100644 --- a/sqllogictest-bin/Cargo.toml +++ b/sqllogictest-bin/Cargo.toml @@ -24,8 +24,8 @@ glob = "0.3" itertools = "0.11" quick-junit = { version = "0.3" } rand = "0.8" -sqllogictest = { path = "../sqllogictest", version = "0.19" } -sqllogictest-engines = { path = "../sqllogictest-engines", version = "0.19" } +sqllogictest = { path = "../sqllogictest", version = "0.20" } +sqllogictest-engines = { path = "../sqllogictest-engines", version = "0.20" } tokio = { version = "1", features = [ "rt", "rt-multi-thread", diff --git a/sqllogictest-bin/src/engines.rs b/sqllogictest-bin/src/engines.rs index d178f55..748e816 100644 --- a/sqllogictest-bin/src/engines.rs +++ b/sqllogictest-bin/src/engines.rs @@ -1,5 +1,4 @@ use std::fmt::Display; -use std::process::ExitStatus; use std::time::Duration; use async_trait::async_trait; @@ -130,7 +129,7 @@ impl AsyncDB for Engines { tokio::time::sleep(dur).await } - async fn run_command(command: std::process::Command) -> std::io::Result { - Command::from(command).status().await + async fn run_command(command: std::process::Command) -> std::io::Result { + Command::from(command).output().await } } diff --git a/sqllogictest-engines/Cargo.toml b/sqllogictest-engines/Cargo.toml index 3c8d26a..670e0e9 100644 --- a/sqllogictest-engines/Cargo.toml +++ b/sqllogictest-engines/Cargo.toml @@ -19,7 +19,7 @@ postgres-types = { version = "0.2.5", features = ["derive", "with-chrono-0_4"] } rust_decimal = { version = "1.30.0", features = ["tokio-pg"] } serde = { version = "1", features = ["derive"] } serde_json = "1" -sqllogictest = { path = "../sqllogictest", version = "0.19" } +sqllogictest = { path = "../sqllogictest", version = "0.20" } thiserror = "1" tokio = { version = "1", features = [ "rt", diff --git a/sqllogictest-engines/src/external.rs b/sqllogictest-engines/src/external.rs index 64a5238..1b9aad9 100644 --- a/sqllogictest-engines/src/external.rs +++ b/sqllogictest-engines/src/external.rs @@ -1,6 +1,6 @@ use std::io; use std::marker::PhantomData; -use std::process::{ExitStatus, Stdio}; +use std::process::Stdio; use std::time::Duration; use async_trait::async_trait; @@ -121,8 +121,8 @@ impl AsyncDB for ExternalDriver { tokio::time::sleep(dur).await } - async fn run_command(command: std::process::Command) -> std::io::Result { - Command::from(command).status().await + async fn run_command(command: std::process::Command) -> std::io::Result { + Command::from(command).output().await } } diff --git a/sqllogictest-engines/src/postgres/extended.rs b/sqllogictest-engines/src/postgres/extended.rs index cef6beb..debba9b 100644 --- a/sqllogictest-engines/src/postgres/extended.rs +++ b/sqllogictest-engines/src/postgres/extended.rs @@ -1,5 +1,5 @@ use std::fmt::Write; -use std::process::{Command, ExitStatus}; +use std::process::Command; use std::time::Duration; use async_trait::async_trait; @@ -319,7 +319,7 @@ impl sqllogictest::AsyncDB for Postgres { tokio::time::sleep(dur).await } - async fn run_command(command: Command) -> std::io::Result { - tokio::process::Command::from(command).status().await + async fn run_command(command: Command) -> std::io::Result { + tokio::process::Command::from(command).output().await } } diff --git a/sqllogictest-engines/src/postgres/simple.rs b/sqllogictest-engines/src/postgres/simple.rs index 690a7ad..7d6e9c2 100644 --- a/sqllogictest-engines/src/postgres/simple.rs +++ b/sqllogictest-engines/src/postgres/simple.rs @@ -1,4 +1,4 @@ -use std::process::{Command, ExitStatus}; +use std::process::Command; use std::time::Duration; use async_trait::async_trait; @@ -66,7 +66,7 @@ impl sqllogictest::AsyncDB for Postgres { tokio::time::sleep(dur).await } - async fn run_command(command: Command) -> std::io::Result { - tokio::process::Command::from(command).status().await + async fn run_command(command: Command) -> std::io::Result { + tokio::process::Command::from(command).output().await } } diff --git a/sqllogictest/src/parser.rs b/sqllogictest/src/parser.rs index c0021f2..e1d33bd 100644 --- a/sqllogictest/src/parser.rs +++ b/sqllogictest/src/parser.rs @@ -136,11 +136,13 @@ pub enum Record { }, /// A system command is an external command that is to be executed by the shell. Currently it /// must succeed and the output is ignored. + #[non_exhaustive] System { loc: Location, conditions: Vec, /// The external command. command: String, + stdout: Option, }, /// A sleep period. Sleep { @@ -266,8 +268,13 @@ impl std::fmt::Display for Record { loc: _, conditions: _, command, + stdout, } => { - writeln!(f, "system ok\n{command}") + writeln!(f, "system ok\n{command}")?; + if let Some(stdout) = stdout { + writeln!(f, "----\n{}\n", stdout.trim())?; + } + Ok(()) } Record::Sleep { loc: _, duration } => { write!(f, "sleep {}", humantime::format_duration(*duration)) @@ -754,7 +761,7 @@ fn parse_inner(loc: &Location, script: &str) -> Result QueryExpect::empty_results(), }; - // The SQL for the query is found on second an subsequent lines of the record + // The SQL for the query is found on second and subsequent lines of the record // up to first line of the form "----" or until the end of the record. let (sql, has_result) = parse_lines(&mut lines, &loc, Some(RESULTS_DELIMITER))?; if has_result { @@ -789,11 +796,19 @@ fn parse_inner(loc: &Location, script: &str) -> Result { // TODO: we don't support asserting error message for system command - let (command, _) = parse_lines(&mut lines, &loc, None)?; + // The command is found on second and subsequent lines of the record + // up to first line of the form "----" or until the end of the record. + let (command, has_result) = parse_lines(&mut lines, &loc, Some(RESULTS_DELIMITER))?; + let stdout = if has_result { + Some(parse_multiple_result(&mut lines)) + } else { + None + }; records.push(Record::System { loc, conditions: std::mem::take(&mut conditions), command, + stdout, }); } ["control", res @ ..] => match res { @@ -897,10 +912,10 @@ fn parse_lines<'a>( Ok((out, found_delimiter)) } -/// Parse multiline error message under `----`. -fn parse_multiline_error<'a>( +/// Parse multiline output under `----`. +fn parse_multiple_result<'a>( lines: &mut Peekable>, -) -> ExpectedError { +) -> String { let mut results = String::new(); while let Some((_, line)) = lines.next() { @@ -913,7 +928,14 @@ fn parse_multiline_error<'a>( results.push('\n'); } - ExpectedError::Multiline(results.trim().to_string()) + results.trim().to_string() +} + +/// Parse multiline error message under `----`. +fn parse_multiline_error<'a>( + lines: &mut Peekable>, +) -> ExpectedError { + ExpectedError::Multiline(parse_multiple_result(lines)) } #[cfg(test)] diff --git a/sqllogictest/src/runner.rs b/sqllogictest/src/runner.rs index b26ce4f..6b02818 100644 --- a/sqllogictest/src/runner.rs +++ b/sqllogictest/src/runner.rs @@ -36,7 +36,9 @@ pub enum RecordOutput { count: u64, error: Option, }, + #[non_exhaustive] System { + stdout: Option, error: Option, }, } @@ -82,11 +84,11 @@ pub trait AsyncDB { /// [`Runner`] calls this function to run a system command. /// - /// The default implementation is `std::process::Command::status`, which is universial to any + /// The default implementation is `std::process::Command::output`, which is universial to any /// async runtime but would block the current thread. If you are running in tokio runtime, you - /// should override this by `tokio::process::Command::status`. - async fn run_command(mut command: Command) -> std::io::Result { - command.status() + /// should override this by `tokio::process::Command::output`. + async fn run_command(mut command: Command) -> std::io::Result { + command.output() } } @@ -255,6 +257,15 @@ pub enum TestErrorKind { }, #[error("system command failed: {err}\n[CMD] {command}")] SystemFail { command: String, err: AnyError }, + #[error( + "system command stdout mismatch:\n[command] {command}\n[Diff] (-expected|+actual)\n{}", + TextDiff::from_lines(.expected_stdout, .actual_stdout).iter_all_changes().format_with("\n", |diff, f| format_diff(&diff, f, false)) + )] + SystemStdoutMismatch { + command: String, + expected_stdout: String, + actual_stdout: String, + }, // Remember to also update [`TestErrorKindDisplay`] if this message is changed. #[error("{kind} is expected to fail with error:\n\t{expected_err}\nbut got error:\n\t{err}\n[SQL] {sql}")] ErrorMismatch { @@ -359,6 +370,19 @@ impl<'a> Display for TestErrorKindDisplay<'a> { format_column_diff(expected, actual, true) ) } + TestErrorKind::SystemStdoutMismatch { + command, + expected_stdout, + actual_stdout, + } => { + write!( + f, + "system command stdout mismatch:\n[command] {command}\n[Diff] (-expected|+actual)\n{}", + TextDiff::from_lines(expected_stdout, actual_stdout) + .iter_all_changes() + .format_with("\n", |diff, f|{ format_diff(&diff, f, true)}) + ) + } _ => write!(f, "{}", self.error), } } @@ -461,6 +485,7 @@ pub fn default_column_validator(_: &Vec, _: &Vec) -> bool { /// The strict validator checks: /// - the number of columns is as expected /// - each column has the same type as expected +#[allow(clippy::ptr_arg)] pub fn strict_column_validator(actual: &Vec, expected: &Vec) -> bool { actual.len() == expected.len() && !actual @@ -592,17 +617,23 @@ impl> Runner { conditions, command, loc: _, + stdout: expected_stdout, } => { let command = match self.may_substitute(command) { Ok(command) => command, - Err(error) => return RecordOutput::System { error: Some(error) }, + Err(error) => { + return RecordOutput::System { + stdout: None, + error: Some(error), + } + } }; if should_skip(&self.labels, "", &conditions) { return RecordOutput::Nothing; } - let cmd = if cfg!(target_os = "windows") { + let mut cmd = if cfg!(target_os = "windows") { let mut cmd = std::process::Command::new("cmd"); cmd.arg("/C").arg(command); cmd @@ -611,19 +642,39 @@ impl> Runner { cmd.arg("-c").arg(command); cmd }; + cmd.stdout(std::process::Stdio::piped()); + cmd.stderr(std::process::Stdio::piped()); let result = D::run_command(cmd).await; - #[derive(thiserror::Error, Debug)] - #[error("process exited unsuccessfully: {0}")] // message from unstable `ExitStatusError` - struct SystemError(ExitStatus); + #[error( + "process exited unsuccessfully: {status}\nstdout: {stdout}\nstderr: {stderr}" + )] + struct SystemError { + status: ExitStatus, + stdout: String, + stderr: String, + } + let mut stdout = None; let error: Option = match result { - Ok(status) if status.success() => None, - Ok(status) => Some(Arc::new(SystemError(status))), + Ok(output) => { + if output.status.success() { + if expected_stdout.is_some() { + stdout = Some(String::from_utf8_lossy(&output.stdout).to_string()); + } + None + } else { + Some(Arc::new(SystemError { + status: output.status, + stdout: String::from_utf8_lossy(&output.stdout).to_string(), + stderr: String::from_utf8_lossy(&output.stderr).to_string(), + })) + } + } Err(e) => Some(Arc::new(e)), }; - RecordOutput::System { error } + RecordOutput::System { error, stdout } } Record::Query { conditions, @@ -909,12 +960,31 @@ impl> Runner { loc, conditions: _, command, + stdout: expected_stdout, + }, + RecordOutput::System { + error, + stdout: actual_stdout, }, - RecordOutput::System { error }, ) => { if let Some(err) = error { return Err(TestErrorKind::SystemFail { command, err }.at(loc)); } + match (expected_stdout, actual_stdout) { + (None, _) => {} + (Some(expected_stdout), actual_stdout) => { + let actual_stdout = actual_stdout.unwrap_or_default(); + // TODO: support newlines contained in expected_stdout + if expected_stdout != actual_stdout.trim() { + return Err(TestErrorKind::SystemStdoutMismatch { + command, + expected_stdout, + actual_stdout, + } + .at(loc)); + } + } + } } _ => unreachable!(), } @@ -1058,8 +1128,11 @@ impl> Runner { /// Substitute the input SQL or command with [`Substitution`], if enabled by `control /// substitution`. fn may_substitute(&self, input: String) -> Result { + #[derive(thiserror::Error, Debug)] + #[error("substitution failed: {0}")] + struct SubstError(subst::Error); if let Some(substitution) = &self.substitution { - subst::substitute(&input, substitution).map_err(|e| Arc::new(e) as AnyError) + subst::substitute(&input, substitution).map_err(|e| Arc::new(SubstError(e)) as AnyError) } else { Ok(input) } @@ -1388,6 +1461,23 @@ pub fn update_record_with_output( }) } }, + ( + Record::System { + loc, + conditions, + command, + stdout: _, + }, + RecordOutput::System { + stdout: actual_stdout, + error: _, + }, + ) => Some(Record::System { + loc, + conditions, + command, + stdout: actual_stdout.clone(), + }), // No update possible, return the original record _ => None, diff --git a/tests/system_command/system_command.rs b/tests/system_command/system_command.rs index 6d191a3..6b86c98 100644 --- a/tests/system_command/system_command.rs +++ b/tests/system_command/system_command.rs @@ -43,9 +43,10 @@ impl sqllogictest::DB for FakeDB { fn test() { let mut tester = sqllogictest::Runner::new(|| async { Ok(FakeDB) }); - tester - .run_file("./system_command/system_command.slt") - .unwrap(); + if let Err(e) = tester.run_file("./system_command/system_command.slt") { + println!("{}", e.display(true)); + panic!(); + } } #[test] @@ -57,4 +58,13 @@ fn test_fail() { .unwrap_err(); assert!(err.to_string().contains("system command failed"), "{err}"); + + let err = tester + .run_file("./system_command/system_command_fail_2.slt") + .unwrap_err(); + + assert!( + err.to_string().contains("system command stdout mismatch"), + "{err}" + ); } diff --git a/tests/system_command/system_command.slt b/tests/system_command/system_command.slt index afd70d9..161c5ef 100644 --- a/tests/system_command/system_command.slt +++ b/tests/system_command/system_command.slt @@ -15,3 +15,32 @@ query T select read("${__TEST_DIR__}/test.txt") ---- 1919810 + +# Note: it ends with 2 newlines +system ok +echo "114514" +---- +114514 + + +# Note: when substitution is on, we need to escape the backslash +# Note: 1 blank line in the middle is ok, but not 2 +system ok +echo "114\\n\\n514" +---- +114 + +514 + + +system ok +cat <