diff --git a/.github/workflows/extended.yml b/.github/workflows/extended.yml index 0b8434206e282..28262d34b7010 100644 --- a/.github/workflows/extended.yml +++ b/.github/workflows/extended.yml @@ -84,6 +84,8 @@ jobs: df -h - name: Run tests (excluding doctests) run: cargo xtask ci step test extended + - name: Run spill pool fuzzer with more iterations + run: cargo xtask ci step test spill-pool-fuzz - parallel: - name: Verify Working Directory Clean run: git diff --exit-code diff --git a/datafusion/core/Cargo.toml b/datafusion/core/Cargo.toml index 89f42761535e3..af0002671a2eb 100644 --- a/datafusion/core/Cargo.toml +++ b/datafusion/core/Cargo.toml @@ -198,6 +198,11 @@ ignored = ["datafusion-doc", "datafusion-macros", "dashmap"] [target.'cfg(not(target_os = "windows"))'.dev-dependencies] nix = { version = "0.31.3", features = ["fs"] } +[[test]] +name = "extended_tests" +path = "tests/extended_tests.rs" +required-features = ["extended_tests"] + [[bin]] name = "print_config_docs" path = "src/bin/print_config_docs.rs" diff --git a/datafusion/core/tests/fuzz.rs b/datafusion/core/tests/extended_tests.rs similarity index 78% rename from datafusion/core/tests/fuzz.rs rename to datafusion/core/tests/extended_tests.rs index 9987721318f83..c7e4171ee3e40 100644 --- a/datafusion/core/tests/fuzz.rs +++ b/datafusion/core/tests/extended_tests.rs @@ -15,11 +15,13 @@ // specific language governing permissions and limitations // under the License. -/// Run all tests that are found in the `fuzz_cases` directory. -/// Fuzz tests are slow and gated behind the `extended_tests` feature. -/// Run with: cargo test --features extended_tests -#[cfg(feature = "extended_tests")] +//! Tests too slow to run on every PR: fuzz tests and memory limit validation. +//! This target requires the `extended_tests` feature. +//! +//! Run with: cargo test -p datafusion --test extended_tests --features extended_tests + mod fuzz_cases; +mod memory_limit_validation; #[cfg(test)] #[ctor::ctor(unsafe)] diff --git a/datafusion/core/tests/fuzz_cases/leaf_pushdown_fuzz.rs b/datafusion/core/tests/fuzz_cases/leaf_pushdown_fuzz.rs index 299423edc1edd..13a6605dd9b03 100644 --- a/datafusion/core/tests/fuzz_cases/leaf_pushdown_fuzz.rs +++ b/datafusion/core/tests/fuzz_cases/leaf_pushdown_fuzz.rs @@ -33,7 +33,7 @@ //! ```text //! LEAF_PUSHDOWN_FUZZ_SEED= LEAF_PUSHDOWN_FUZZ_CASES=1 \ //! cargo test --profile ci -p datafusion --features extended_tests \ -//! --test fuzz -- leaf_pushdown --nocapture +//! --test extended_tests -- leaf_pushdown --nocapture //! ``` //! //! `LEAF_PUSHDOWN_FUZZ_CASES` also raises the case count of a normal run. @@ -76,7 +76,7 @@ const INCLUDE_FALSE_BRANCH_UNION_SHAPES: bool = false; /// Cases of the short run. Keep this low: the whole test must stay in a few /// seconds. const DEFAULT_CASES: usize = 250; -/// Cases of the long run, gated behind the `extended_tests` feature. +/// Cases of the long run. const EXTENDED_CASES: usize = 5000; /// Highest share of cases that may fail to plan on both sides. A higher share /// means the generator makes junk. @@ -777,9 +777,6 @@ async fn leaf_pushdown_parquet_fuzz() { run_parquet(env_usize("LEAF_PUSHDOWN_FUZZ_CASES", DEFAULT_CASES / 2)).await; } -/// The whole `fuzz_cases` tree is behind the `extended_tests` feature today. -/// The attribute keeps the long run gated if that changes. -#[cfg(feature = "extended_tests")] #[tokio::test(flavor = "multi_thread")] async fn leaf_pushdown_fuzz_extended() { run_memory(env_usize("LEAF_PUSHDOWN_FUZZ_CASES", EXTENDED_CASES)).await; diff --git a/datafusion/core/tests/memory_limit/mod.rs b/datafusion/core/tests/memory_limit/mod.rs index d8814eaa86307..7239bcf850255 100644 --- a/datafusion/core/tests/memory_limit/mod.rs +++ b/datafusion/core/tests/memory_limit/mod.rs @@ -20,8 +20,6 @@ use std::num::NonZeroUsize; use std::sync::{Arc, LazyLock}; -#[cfg(feature = "extended_tests")] -mod memory_limit_validation; mod nlj_spill_unmatched; mod repartition_mem_limit; mod union_nullable_spill; diff --git a/datafusion/core/tests/memory_limit/memory_limit_validation/mod.rs b/datafusion/core/tests/memory_limit_validation/mod.rs similarity index 100% rename from datafusion/core/tests/memory_limit/memory_limit_validation/mod.rs rename to datafusion/core/tests/memory_limit_validation/mod.rs diff --git a/datafusion/core/tests/memory_limit/memory_limit_validation/nlj_mem_validation.rs b/datafusion/core/tests/memory_limit_validation/nlj_mem_validation.rs similarity index 97% rename from datafusion/core/tests/memory_limit/memory_limit_validation/nlj_mem_validation.rs rename to datafusion/core/tests/memory_limit_validation/nlj_mem_validation.rs index 2b38de04d5cda..9c9a93b7a387c 100644 --- a/datafusion/core/tests/memory_limit/memory_limit_validation/nlj_mem_validation.rs +++ b/datafusion/core/tests/memory_limit_validation/nlj_mem_validation.rs @@ -19,7 +19,7 @@ use datafusion::prelude::SessionConfig; -use crate::memory_limit::memory_limit_validation::utils; +use crate::memory_limit_validation::utils; #[test] fn nlj_no_mem_limit_runner() { diff --git a/datafusion/core/tests/memory_limit/memory_limit_validation/smj_mem_validation.rs b/datafusion/core/tests/memory_limit_validation/smj_mem_validation.rs similarity index 98% rename from datafusion/core/tests/memory_limit/memory_limit_validation/smj_mem_validation.rs rename to datafusion/core/tests/memory_limit_validation/smj_mem_validation.rs index 3af642fffe101..4a3711ac38561 100644 --- a/datafusion/core/tests/memory_limit/memory_limit_validation/smj_mem_validation.rs +++ b/datafusion/core/tests/memory_limit_validation/smj_mem_validation.rs @@ -21,7 +21,7 @@ use datafusion::prelude::SessionConfig; -use crate::memory_limit::memory_limit_validation::utils; +use crate::memory_limit_validation::utils; /// Ensures the planner selected a sort-merge join. const SMJ_OPERATOR_NAME: &str = "SortMergeJoinExec"; diff --git a/datafusion/core/tests/memory_limit/memory_limit_validation/sort_mem_validation.rs b/datafusion/core/tests/memory_limit_validation/sort_mem_validation.rs similarity index 99% rename from datafusion/core/tests/memory_limit/memory_limit_validation/sort_mem_validation.rs rename to datafusion/core/tests/memory_limit_validation/sort_mem_validation.rs index 689915f4aa9db..01a8afa1c7bfd 100644 --- a/datafusion/core/tests/memory_limit/memory_limit_validation/sort_mem_validation.rs +++ b/datafusion/core/tests/memory_limit_validation/sort_mem_validation.rs @@ -22,7 +22,7 @@ //! - Test runners that spawn individual test processes //! - Test cases that contain the actual validation logic -use crate::memory_limit::memory_limit_validation::utils; +use crate::memory_limit_validation::utils; // =========================================================================== // Test runners: diff --git a/datafusion/core/tests/memory_limit/memory_limit_validation/utils.rs b/datafusion/core/tests/memory_limit_validation/utils.rs similarity index 97% rename from datafusion/core/tests/memory_limit/memory_limit_validation/utils.rs rename to datafusion/core/tests/memory_limit_validation/utils.rs index aae30418b25f9..ef3fcccdc5adf 100644 --- a/datafusion/core/tests/memory_limit/memory_limit_validation/utils.rs +++ b/datafusion/core/tests/memory_limit_validation/utils.rs @@ -97,7 +97,7 @@ where /// environment variable set. Re-invokes the current test binary directly, /// avoiding cargo overhead and recompilation. pub fn spawn_test_process(module: &str, test: &str) { - let test_path = format!("memory_limit::memory_limit_validation::{module}::{test}"); + let test_path = format!("memory_limit_validation::{module}::{test}"); let exe = std::env::current_exe().expect("Failed to get test binary path"); let output = Command::new(exe) .arg(&test_path) @@ -114,6 +114,11 @@ pub fn spawn_test_process(module: &str, test: &str) { "Test '{test}' failed with status: {}\nstdout:\n{stdout}\nstderr:\n{stderr}", output.status, ); + // A path that matches no test passes without running anything. + assert!( + stdout.contains("running 1 test"), + "Test path '{test_path}' did not select exactly one test\nstdout:\n{stdout}", + ); } fn operator_spill_count(plan: &dyn ExecutionPlan, operator_name: &str) -> usize { diff --git a/xtask/src/ci_steps.rs b/xtask/src/ci_steps.rs index 6ee4d73edfb20..a3dc597fe450d 100644 --- a/xtask/src/ci_steps.rs +++ b/xtask/src/ci_steps.rs @@ -30,6 +30,8 @@ type StepRunner = fn(&StepContext, &[String]) -> StepResult; const CI_COMMAND: &str = "cargo xtask ci step"; const CI_SHORTCUT: &str = "cargo ci-step"; const TPCH_DATA_DIR: &str = "datafusion/sqllogictest/test_files/tpch/data"; +/// The `datafusion-physical-plan` unit test that the `spill-pool-fuzz` step runs. +const SPILL_POOL_FUZZ_TEST: &str = "spill::spill_pool::tests::spill_pool_scenario_fuzz"; /// Metadata and implementation for one CI step. /// @@ -55,7 +57,7 @@ static CI_STEPS: &[StepInfo] = &[ }, StepInfo { command: "test", - help_usage: "test [--explain]", + help_usage: "test [--explain]", help_examples: &["test cli", "test workspace"], help_description: "Run a CI test suite", error_message: "Cargo test step failed", @@ -397,8 +399,8 @@ impl StepContext { } } "extended" => { - // Run only the tests that the `workspace` step does not. The - // merge queue runs that step on the same commit. + // Tests behind the `extended_tests` feature. The `workspace` + // step runs everything else. command .args([ "test", @@ -406,27 +408,29 @@ impl StepContext { "ci", "-p", "datafusion", - "-p", - "datafusion-physical-plan", - "--lib", "--test", - "fuzz", - "--test", - "core_integration", + "extended_tests", "--features", "avro,backtrace,extended_tests,parquet_encryption", + ]) + .env("RUST_BACKTRACE", "1"); + } + "spill-pool-fuzz" => { + // Run more random scenarios of the spill pool fuzzer than the + // `workspace` step does (about a minute). + command + .args([ + "test", + "--profile", + "ci", + "-p", + "datafusion-physical-plan", + "--lib", "--", - // Tests behind the `extended_tests` feature. A new test - // behind it needs a filter here too, otherwise it never - // runs in CI. - "fuzz_cases::", - "memory_limit_validation::", - // Runs with more iterations, see below. - "spill_pool_scenario_fuzz", + "--exact", + SPILL_POOL_FUZZ_TEST, ]) .env("RUST_BACKTRACE", "1") - // Run more random scenarios of the spill pool fuzzer than the - // default suite does (about a minute). .env("DATAFUSION_SPILL_POOL_FUZZ_ITERATIONS", "1000"); } "hash-collisions" => { @@ -657,7 +661,7 @@ fn required_env(name: &str) -> Result { mod tests { use super::*; - const TEST_USAGE: &str = "usage: cargo xtask ci step test [--explain]"; + const TEST_USAGE: &str = "usage: cargo xtask ci step test [--explain]"; fn args(args: &[&str]) -> Vec { args.iter().map(|arg| (*arg).to_string()).collect() @@ -706,6 +710,56 @@ mod tests { assert_eq!(error, TEST_USAGE); } + /// A filter that matches no test passes without running anything, so a + /// renamed or moved spill pool fuzzer would silently stop running in CI. + #[test] + fn spill_pool_fuzz_test_exists() { + let (module, test) = SPILL_POOL_FUZZ_TEST + .rsplit_once("::tests::") + .expect("test path has a `tests` module"); + let file = Path::new(env!("CARGO_MANIFEST_DIR")) + .join("../datafusion/physical-plan/src") + .join(format!("{}.rs", module.replace("::", "/"))); + let source = std::fs::read_to_string(&file) + .unwrap_or_else(|e| panic!("cannot read {}: {e}", file.display())); + assert!( + source.contains("mod tests {") && source.contains(&format!("fn {test}(")), + "{} has no test `{test}` in `mod tests`; update SPILL_POOL_FUZZ_TEST", + file.display() + ); + } + + /// The `extended` step runs only the `extended_tests` test target, which + /// requires the `extended_tests` feature. A test gated on the feature with a + /// `cfg` in any other target would never run in CI. + #[test] + fn extended_tests_feature_only_gates_whole_targets() { + fn visit(dir: &Path, found: &mut Vec) { + for entry in std::fs::read_dir(dir).unwrap() { + let path = entry.unwrap().path(); + if path.is_dir() { + visit(&path, found); + } else if path.extension() == Some(OsStr::new("rs")) + && std::fs::read_to_string(&path) + .unwrap() + .contains("feature = \"extended_tests\"") + { + found.push(path); + } + } + } + let mut found = vec![]; + visit( + &Path::new(env!("CARGO_MANIFEST_DIR")).join("../datafusion"), + &mut found, + ); + assert!( + found.is_empty(), + "put tests behind `extended_tests` in the `extended_tests` test target \ + (see datafusion/core/Cargo.toml), not behind a `cfg` in {found:?}" + ); + } + #[test] fn unknown_test_variant_is_rejected() { let error = context() @@ -936,17 +990,27 @@ cargo test \ expected: |actual| { insta::assert_snapshot!(actual, @r" cd /workspace && \ -DATAFUSION_SPILL_POOL_FUZZ_ITERATIONS=1000 \ RUST_BACKTRACE=1 \ cargo test \ --profile ci \ -p datafusion \ +--test extended_tests \ +--features avro,backtrace,extended_tests,parquet_encryption +") + }, + }, + Cmd { + command: &["test", "spill-pool-fuzz"], + expected: |actual| { + insta::assert_snapshot!(actual, @r" +cd /workspace && \ +DATAFUSION_SPILL_POOL_FUZZ_ITERATIONS=1000 \ +RUST_BACKTRACE=1 \ +cargo test \ +--profile ci \ -p datafusion-physical-plan \ --lib \ ---test fuzz \ ---test core_integration \ ---features avro,backtrace,extended_tests,parquet_encryption \ --- fuzz_cases:: memory_limit_validation:: spill_pool_scenario_fuzz +-- --exact spill::spill_pool::tests::spill_pool_scenario_fuzz ") }, },