From 2cc78f315f8632dafd6e67e9c9a9fd27c7647dad Mon Sep 17 00:00:00 2001 From: Jiacai Liu Date: Fri, 3 Feb 2023 15:26:49 +0800 Subject: [PATCH] refactor: implement new compare logic (#32) * refactor compare logic * update docs * fix clippy * rename diff_cases to failed_cases * seek to start after truncate * Update README.md Co-authored-by: Ruihang Xia * Update README.md Co-authored-by: Ruihang Xia * Update README.md Co-authored-by: Ruihang Xia * Update README.md Co-authored-by: Ruihang Xia --------- Co-authored-by: Ruihang Xia --- .github/workflows/ci.yml | 1 - .gitignore | 5 -- README.md | 15 +++--- examples/bad.rs | 2 +- src/config.rs | 16 ++---- src/error.rs | 2 +- src/lib.rs | 3 +- src/runner.rs | 110 +++++++++++++++++---------------------- 8 files changed, 65 insertions(+), 89 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1b728ef..f58f506 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -46,7 +46,6 @@ jobs: restore-keys: | debug-${{ runner.os }}-${{ hashFiles('rust-toolchain') }}- debug-${{ runner.os }}- - debug-${{ runner.os }} - name: Install cargo-sort run: | cargo install cargo-sort diff --git a/.gitignore b/.gitignore index d47cc97..96ef6c0 100644 --- a/.gitignore +++ b/.gitignore @@ -1,7 +1,2 @@ /target -# Ignore this output file, since the bad example will always fail. -# For normal case, output file should not be ignored, and it acts as -# a signal that our testcase need to be fixed. -examples/bad-case/simple/select.output -# Ignoring cargo.lock file Cargo.lock diff --git a/README.md b/README.md index 5215436..7a99df2 100644 --- a/README.md +++ b/README.md @@ -20,7 +20,7 @@ examples/ ├── basic-case # Testcase root directory │ └── simple # One environment │ ├── config.toml # Config file for current environment -│ ├── select.result # Expected result file +│ ├── select.result # Output result file │ └── select.sql # Input SQL testcase ├── basic.rs # Entrypoint of this example @@ -32,14 +32,15 @@ cargo run --example basic ``` It will do following things: 1. Collect all environments(first-level directory) under `basic-case`. -2. Run testcases(`.sql` files) under environment one after one. - 1. Write temporary result to `{testcase}.output` - 2. Compare `{testcase}.output` with `{testcase}.result` using `diff` +2. Run tests(`.sql` files) under environment one after one. + 1. Before execution it will read `{testcase}.result`(create one if not exists) to memory for compare. + 2. During execution it will collect query response and write to `{testcase}.result` + 3. After execution it will compare the generated `{testcase}.result` with previous one, **PASS** when they are the same, and **FAIL** otherwise. 3. Report result. -Our target is to keep `*.result` file up to date, when `*.output` is equals to its corresponding result, the runner will delete it after executed. - -When there is any diffs, the runner will keep `*.output` for later investigation. +Usually `result` files should be tracked in git, whenever there are failed tests, users should +1. Update `result` to latest version(e.g. `git add`) if the newer result is right, or +2. Restore `result` back to original version (e.g. `git checkout`), troubleshoot bugs in database implementation, and run tests again Below is the output of this example: ```bash diff --git a/examples/bad.rs b/examples/bad.rs index 0f42e26..72c7a58 100644 --- a/examples/bad.rs +++ b/examples/bad.rs @@ -2,7 +2,7 @@ //! A demo designed to run failed. //! -//! When there is any diff between ${testcase}.output and ${testcase}.result, +//! When there is any diff between old and new result, //! Users must resolve the diff, and keep the result file up to date. use std::{fmt::Display, fs::File, path::Path}; diff --git a/src/config.rs b/src/config.rs index 6f8d6c4..6666141 100644 --- a/src/config.rs +++ b/src/config.rs @@ -13,14 +13,10 @@ pub struct Config { #[builder(default = "Config::default_test_case_extension()")] #[serde(default = "Config::default_test_case_extension")] pub test_case_extension: String, - /// Default value: `output` - #[builder(default = "Config::default_output_result_extension()")] - #[serde(default = "Config::default_output_result_extension")] - pub output_result_extension: String, /// Default value: `result` - #[builder(default = "Config::default_expect_result_extension()")] - #[serde(default = "Config::default_expect_result_extension")] - pub expect_result_extension: String, + #[builder(default = "Config::default_result_extension()")] + #[serde(default = "Config::default_result_extension")] + pub result_extension: String, /// Default value: `-- SQLNESS` #[builder(default = "Config::default_interceptor_prefix()")] #[serde(default = "Config::default_interceptor_prefix")] @@ -44,11 +40,7 @@ impl Config { "sql".to_string() } - fn default_output_result_extension() -> String { - "output".to_string() - } - - fn default_expect_result_extension() -> String { + fn default_result_extension() -> String { "result".to_string() } diff --git a/src/error.rs b/src/error.rs index 266c628..25efe66 100644 --- a/src/error.rs +++ b/src/error.rs @@ -21,7 +21,7 @@ pub enum SqlnessError { #[error("IO operation failed, source error: {0}")] IO(#[from] std::io::Error), - #[error("Cannot parse the output/result file. Not valid UTF-8 encoding")] + #[error("Cannot parse the result file. Not valid UTF-8 encoding")] ReadResult(#[from] std::string::FromUtf8Error), #[error("Run failed. {count} cases can't pass")] diff --git a/src/lib.rs b/src/lib.rs index 9783650..655172c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -34,6 +34,7 @@ //! │ ├── config.toml //! │ ├── dml //! │ │ └── basic.sql +//! │ │ └── basic.result //! │ ├── ddl //! │ └── system //! └── remote @@ -51,7 +52,7 @@ //! Note that only the first layer of sub-directory is special (for distinguash //! different environments). All deeper layers are treated as the same. E.g., //! both `sqlness/local/dml/basic.sql` and `sqlness/local/dml/another-dir/basic.sql` -//! will be run under the `local` in the same pass. +//! will be run under the `local` env in the same pass. mod case; mod config; diff --git a/src/runner.rs b/src/runner.rs index 59e4eb3..1d2d777 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -1,12 +1,13 @@ // Copyright 2022 CeresDB Project Authors. Licensed under Apache-2.0. +use std::io::{Cursor, SeekFrom}; use std::path::{Path, PathBuf}; use std::str::FromStr; use prettydiff::basic::{DiffOp, SliceChangeset}; use prettydiff::diff_lines; -use tokio::fs::{read_dir, remove_file, File, OpenOptions}; -use tokio::io::{AsyncReadExt, AsyncWriteExt}; +use tokio::fs::{read_dir, File, OpenOptions}; +use tokio::io::{AsyncReadExt, AsyncSeekExt, AsyncWriteExt}; use tokio::time::Instant; use walkdir::WalkDir; @@ -124,15 +125,15 @@ impl Runner { async fn run_env(&self, env: &str, db: &E::DB) -> Result<()> { let case_paths = self.collect_case_paths(env).await?; - let mut diff_cases = vec![]; + let mut failed_cases = vec![]; let mut errors = vec![]; let start = Instant::now(); for path in case_paths { - let case_result = self.run_single_case(db, &path).await; + let is_success = self.run_single_case(db, &path).await; let case_name = path.as_os_str().to_str().unwrap().to_owned(); - match case_result { - Ok(true) => diff_cases.push(case_name), - Ok(false) => {} + match is_success { + Ok(false) => failed_cases.push(case_name), + Ok(true) => {} Err(e) => { if self.config.fail_fast { println!("Case {case_name} failed with error {e:?}"); @@ -151,17 +152,17 @@ impl Runner { start.elapsed().as_millis() ); - let mut error_count = 0; - if !diff_cases.is_empty() { - println!("Different cases:"); - println!("{diff_cases:#?}"); - error_count += diff_cases.len(); + if !failed_cases.is_empty() { + println!("Failed cases:"); + println!("{failed_cases:#?}"); } + if !errors.is_empty() { println!("Error cases:"); println!("{errors:#?}"); - error_count += errors.len(); } + + let error_count = failed_cases.len() + errors.len(); if error_count == 0 { Ok(()) } else { @@ -169,20 +170,39 @@ impl Runner { } } - async fn run_single_case(&self, db: &E::DB, path: &PathBuf) -> Result { + /// Return true when this case pass, otherwise false. + async fn run_single_case(&self, db: &E::DB, path: &Path) -> Result { let case_path = path.with_extension(&self.config.test_case_extension); - let case = TestCase::from_file(case_path, &self.config).await?; - let output_path = path.with_extension(&self.config.output_result_extension); - let mut output_file = Self::open_output_file(&output_path).await?; + let case = TestCase::from_file(&case_path, &self.config).await?; + let result_path = path.with_extension(&self.config.result_extension); + let mut result_file = OpenOptions::new() + .create(true) + .write(true) + .read(true) + .open(&result_path) + .await?; + + // Read old result out for compare later + let mut old_result = String::new(); + result_file.read_to_string(&mut old_result).await?; + // Execute testcase + let mut new_result = Cursor::new(Vec::new()); let timer = Instant::now(); - case.execute(db, &mut output_file).await?; + case.execute(db, &mut new_result).await?; let elapsed = timer.elapsed(); - output_file.flush().await?; - let is_different = self.compare(&path).await?; - if !is_different { - remove_file(output_path).await?; + // Truncate and write new result back + result_file.set_len(0).await?; + result_file.seek(SeekFrom::Start(0)).await?; + result_file.write_all(new_result.get_ref()).await?; + + // Compare old and new result + let new_result = String::from_utf8(new_result.into_inner()).expect("not utf8 string"); + if let Some(diff) = self.compare(&old_result, &new_result) { + println!("Result unexpected, path:{case_path:?}"); + println!("{diff}"); + return Ok(false); } println!( @@ -190,7 +210,8 @@ impl Runner { path.as_os_str(), elapsed.as_millis() ); - Ok(is_different) + + Ok(true) } async fn collect_case_paths(&self, env: &str) -> Result> { @@ -229,48 +250,15 @@ impl Runner { Ok(cases) } - async fn open_output_file>(path: P) -> Result { - Ok(OpenOptions::default() - .create(true) - .truncate(true) - .write(true) - .open(&path) - .await?) - } - - /// Compare files' diff, return true if two files are different - async fn compare>(&self, path: P) -> Result { - let mut result_lines = vec![]; - File::open( - path.as_ref() - .with_extension(&self.config.expect_result_extension), - ) - .await? - .read_to_end(&mut result_lines) - .await?; - let result_lines = String::from_utf8(result_lines)?; - - let mut output_lines = vec![]; - File::open( - path.as_ref() - .with_extension(&self.config.output_result_extension), - ) - .await? - .read_to_end(&mut output_lines) - .await?; - let output_lines = String::from_utf8(output_lines)?; - - let diff = diff_lines(&result_lines, &output_lines); + /// Compare result, return None if them are the same, else return diff changes + fn compare(&self, expected: &str, actual: &str) -> Option { + let diff = diff_lines(expected, actual); let diff = diff.diff(); let is_different = diff.iter().any(|d| !matches!(d, DiffOp::Equal(_))); if is_different { - println!( - "Result unexpected, path:{}", - path.as_ref().to_string_lossy() - ); - println!("{}", SliceChangeset { diff }); + return Some(format!("{}", SliceChangeset { diff })); } - Ok(is_different) + None } }