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 <waynestxia@gmail.com>

* Update README.md

Co-authored-by: Ruihang Xia <waynestxia@gmail.com>

* Update README.md

Co-authored-by: Ruihang Xia <waynestxia@gmail.com>

* Update README.md

Co-authored-by: Ruihang Xia <waynestxia@gmail.com>

---------

Co-authored-by: Ruihang Xia <waynestxia@gmail.com>

.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

.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

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

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};

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()
     }
 

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")]

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;

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<E: EnvController> Runner<E> {
 
     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,46 +152,66 @@ impl<E: EnvController> Runner<E> {
             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 {
             Err(SqlnessError::RunFailed { count: error_count })
         }
     }
 
-    async fn run_single_case(&self, db: &E::DB, path: &PathBuf) -> Result<bool> {
+    /// Return true when this case pass, otherwise false.
+    async fn run_single_case(&self, db: &E::DB, path: &Path) -> Result<bool> {
         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!(
             "Test case {:?} finished, cost: {}ms",
             path.as_os_str(),
             elapsed.as_millis()
         );
-        Ok(is_different)
+
+        Ok(true)
     }
 
     async fn collect_case_paths(&self, env: &str) -> Result<Vec<PathBuf>> {
@@ -229,48 +250,15 @@ impl<E: EnvController> Runner<E> {
         Ok(cases)
     }
 
-    async fn open_output_file<P: AsRef<Path>>(path: P) -> Result<File> {
-        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<P: AsRef<Path>>(&self, path: P) -> Result<bool> {
-        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<String> {
+        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
     }
 }