Merge pull request #1893 from influxdata/crepererum/fix_query_tests_rebuild

test: don't rebuild `query_tests` all the time

Cargo.lock

@@ -3057,6 +3057,7 @@ dependencies = [
  "query",
  "server",
  "snafu",
+ "tempfile",
  "test_helpers",
  "tokio",
 ]

query_tests/Cargo.toml

@@ -22,5 +22,6 @@ internal_types = { path = "../internal_types" }
 metrics = { path = "../metrics" }
 object_store = { path = "../object_store" }
 snafu = "0.6.3"
+tempfile = "3.1.0"
 test_helpers = { path = "../test_helpers" }
 tokio = { version = "1.0", features = ["macros", "time"] }

query_tests/build.rs

@@ -1,4 +1,4 @@
-//! Finds all .sql files in cases/ and creates corresponding entries in src/cases.rs
+//! Finds all .sql files in `cases/in/` and creates corresponding entries in src/cases.rs
 //! native Rust types.
 
 use std::path::{Path, PathBuf};
@@ -9,7 +9,7 @@ type Result<T, E = Error> = std::result::Result<T, E>;
 fn main() -> Result<()> {
     // crate root
     let root = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
-    let cases = root.join("cases");
+    let cases = root.join("cases").join("in");
 
     let sql_files = find_sql_files(&cases);
 
@@ -19,9 +19,7 @@ fn main() -> Result<()> {
     // Now create the generated sql file
     let output_content = make_cases_rs(&sql_files).join("\n");
     let output_file = root.join("src").join("cases.rs");
-    std::fs::write(&output_file, output_content)
-        .map_err(|e| format!("Error writing to {:?}: {}", output_file, e))
-        .unwrap();
+    write_if_changed(&output_file, &output_content);
 
     Ok(())
 }
@@ -64,7 +62,7 @@ use crate::runner::Runner;"#
 #[tokio::test]
 // Tests from {:?},
 async fn test_cases_{}() {{
-    let input_path = Path::new("cases").join("{}");
+    let input_path = Path::new("cases").join("in").join("{}");
     let mut runner = Runner::new();
     runner
         .run(input_path)
@@ -80,3 +78,24 @@ async fn test_cases_{}() {{
 
     output_lines
 }
+
+/// Write content to file if it is different to the current content.
+///
+/// This prevents us from touching the file and modifying the `mtime` so that Cargo will re-compile the output `.rs`
+/// file every time. Also see [`rust-lang/cargo#6529`](https://github.com/rust-lang/cargo/issues/6529).
+fn write_if_changed(path: &Path, content: &str) {
+    let changed = if !path.exists() {
+        true
+    } else {
+        match std::fs::read_to_string(path) {
+            Ok(old_content) => old_content != content,
+            Err(_) => true,
+        }
+    };
+
+    if changed {
+        std::fs::write(path, content)
+            .map_err(|e| format!("Error writing to {:?}: {}", path, e))
+            .unwrap();
+    }
+}

query_tests/src/README.md

@@ -15,7 +15,7 @@ The tests in `src/runner` are driven somewhat more dynamically based on input fi
 # Cookbook: Adding a new Test
 
 How do you make a new test:
-1. Add a new file .sql to the `cases` directory
+1. Add a new file .sql to the `cases/in` directory
 2. Run the tests `` cargo test -p query_tests`
 3. You will get a failure message that contains examples of how to update the files
 
@@ -23,9 +23,9 @@ How do you make a new test:
 ## Example output
 Possibly helpful commands:
   # See diff
-  diff -du "/Users/alamb/Software/influxdb_iox/query_tests/cases/pushdown.expected" "/Users/alamb/Software/influxdb_iox/query_tests/cases/pushdown.out"
+  diff -du "/Users/alamb/Software/influxdb_iox/query_tests/cases/in/pushdown.expected" "/Users/alamb/Software/influxdb_iox/query_tests/cases/out/pushdown.out"
   # Update expected
-  cp -f "/Users/alamb/Software/influxdb_iox/query_tests/cases/pushdown.out" "/Users/alamb/Software/influxdb_iox/query_tests/cases/pushdown.expected"
+  cp -f "/Users/alamb/Software/influxdb_iox/query_tests/cases/in/pushdown.out" "/Users/alamb/Software/influxdb_iox/query_tests/cases/out/pushdown.expected"
 
 # Cookbook: Adding a new test scenaroo
 

query_tests/src/cases.rs

@@ -7,7 +7,7 @@ use crate::runner::Runner;
 #[tokio::test]
 // Tests from "duplicates.sql",
 async fn test_cases_duplicates_sql() {
-    let input_path = Path::new("cases").join("duplicates.sql");
+    let input_path = Path::new("cases").join("in").join("duplicates.sql");
     let mut runner = Runner::new();
     runner
         .run(input_path)
@@ -21,7 +21,7 @@ async fn test_cases_duplicates_sql() {
 #[tokio::test]
 // Tests from "pushdown.sql",
 async fn test_cases_pushdown_sql() {
-    let input_path = Path::new("cases").join("pushdown.sql");
+    let input_path = Path::new("cases").join("in").join("pushdown.sql");
     let mut runner = Runner::new();
     runner
         .run(input_path)

query_tests/src/runner.rs

@@ -5,7 +5,7 @@ mod setup;
 
 use arrow::record_batch::RecordBatch;
 use query::{exec::ExecutorType, frontend::sql::SqlQueryPlanner};
-use snafu::{ResultExt, Snafu};
+use snafu::{OptionExt, ResultExt, Snafu};
 use std::{
     io::BufWriter,
     io::Write,
@@ -78,6 +78,12 @@ pub enum Error {
 
     #[snafu(display("IO inner while flushing buffer: {}", source))]
     FlushingBuffer { source: std::io::Error },
+
+    #[snafu(display("Input path has no file stem: '{:?}'", path))]
+    NoFileStem { path: PathBuf },
+
+    #[snafu(display("Input path has no parent?!: '{:?}'", path))]
+    NoParent { path: PathBuf },
 }
 pub type Result<T, E = Error> = std::result::Result<T, E>;
 
@@ -136,15 +142,13 @@ impl<W: Write> Runner<W> {
 
     /// Run the test case of the specified `input_path`
     ///
-    /// Produces output at `input_path`.out
+    /// Produces output at `../out/<input_path>.out`
     ///
-    /// Compares it to an expected result at `input_path`.expected
-    ///
-    /// Returns Ok on success, or Err() on failure
+    /// Compares it to an expected result at `<input_path>.expected`
     pub async fn run(&mut self, input_path: impl Into<PathBuf>) -> Result<()> {
         let input_path = input_path.into();
         // create output and expected output
-        let output_path = input_path.with_extension("out");
+        let output_path = make_output_path(&input_path)?;
         let expected_path = input_path.with_extension("expected");
 
         writeln!(self.log, "Running case in {:?}", input_path)?;
@@ -274,6 +278,25 @@ impl<W: Write> Runner<W> {
     }
 }
 
+/// Return output path for input path.
+fn make_output_path(input: &Path) -> Result<PathBuf> {
+    let stem = input.file_stem().context(NoFileStem { path: input })?;
+
+    // go two levels up (from file to dir, from dir to parent dir)
+    let parent = input.parent().context(NoParent { path: input })?;
+    let parent = parent.parent().context(NoParent { path: parent })?;
+    let mut out = parent.to_path_buf();
+
+    // go one level down (from parent dir to out-dir)
+    out.push("out");
+
+    // set file name and ext
+    out.set_file_name(stem);
+    out.set_extension("out");
+
+    Ok(out)
+}
+
 /// Return the absolute path to `path`, regardless of if it exists or
 /// not on the local filesystem
 fn make_absolute(path: &Path) -> PathBuf {
@@ -284,7 +307,7 @@ fn make_absolute(path: &Path) -> PathBuf {
 
 #[cfg(test)]
 mod test {
-    use test_helpers::assert_contains;
+    use test_helpers::{assert_contains, tmp_dir};
 
     use super::*;
 
@@ -307,15 +330,15 @@ SELECT * from disk;
 
     #[tokio::test]
     async fn runner_positive() {
-        let input_file = test_helpers::make_temp_file(TEST_INPUT);
-        let output_path = input_file.path().with_extension("out");
-        let expected_path = input_file.path().with_extension("expected");
+        let (_tmp_dir, input_file) = make_in_file(TEST_INPUT);
+        let output_path = make_output_path(&input_file).unwrap();
+        let expected_path = input_file.with_extension("expected");
 
         // write expected output
         std::fs::write(&expected_path, EXPECTED_OUTPUT).unwrap();
 
         let mut runner = Runner::new_with_writer(vec![]);
-        let runner_results = runner.run(&input_file.path()).await;
+        let runner_results = runner.run(&input_file).await;
 
         // ensure that the generated output and expected output match
         let output_contents = read_file(&output_path);
@@ -340,15 +363,15 @@ SELECT * from disk;
 
     #[tokio::test]
     async fn runner_negative() {
-        let input_file = test_helpers::make_temp_file(TEST_INPUT);
-        let output_path = input_file.path().with_extension("out");
-        let expected_path = input_file.path().with_extension("expected");
+        let (_tmp_dir, input_file) = make_in_file(TEST_INPUT);
+        let output_path = make_output_path(&input_file).unwrap();
+        let expected_path = input_file.with_extension("expected");
 
         // write incorrect expected output
         std::fs::write(&expected_path, "this is not correct").unwrap();
 
         let mut runner = Runner::new_with_writer(vec![]);
-        let runner_results = runner.run(&input_file.path()).await;
+        let runner_results = runner.run(&input_file).await;
 
         // ensure that the generated output and expected output match
         let output_contents = read_file(&output_path);
@@ -374,6 +397,18 @@ SELECT * from disk;
         assert_contains!(&runner_log, "Setup: TwoMeasurements");
     }
 
+    fn make_in_file<C: AsRef<[u8]>>(contents: C) -> (tempfile::TempDir, PathBuf) {
+        let dir = tmp_dir().expect("create temp dir");
+        let in_dir = dir.path().join("in");
+        std::fs::create_dir(&in_dir).expect("create in-dir");
+
+        let mut file = in_dir;
+        file.set_file_name("foo.sql");
+
+        std::fs::write(&file, contents).expect("writing data to temp file");
+        (dir, file)
+    }
+
     fn read_file(path: &Path) -> String {
         let output_contents = std::fs::read(path).expect("Can read file");
         String::from_utf8(output_contents).expect("utf8")