villebro opened a new pull request, #2819: URL: https://github.com/apache/datafusion-ballista/pull/2819
# Rationale for this change This is the third in a series of cleanup PRs that remove duplicated test code, each one shrinking the code base, after #2791 and #2800. This PR consolidates duplicated scheduler test fixtures and helpers into the scheduler's shared test modules. It does not add, remove or merge any test. A later PR does the same for the remaining crates' tests. The scheduler tests repeated the same fixtures in many modules: the `id`/`gmv` aggregation plan in seven places, the join/aggregate/sort plan in three, the executor registration and status literals in a dozen tests, the scheduler server construction in twelve, four near-identical `JobStateTest` assertions, two pass-through test operators, and in the aqe tests blank `PartitionLocation` literals, planner constructors and a statistics-only leaf node. # Which issue does this PR close? Not related to an issue. The series follows [a review comment on #2765](https://github.com/apache/datafusion-ballista/pull/2765#discussion_r4487993485) pointing out test fixtures copied between test files. # What changes are included in this PR? - `ballista/scheduler/src/test_utils.rs`: `aggregation_logical_plan`, `join_aggregate_sort_logical_plan`, `graph_from_logical`, `test_scheduler_server`, `successful_task_status` and failed-status builders, `drain_tasks`, `await_job_status` and `await_free_vcores`, `executor_registration`, one `PassThroughExec` in place of `FailingPlanRewriteExec` and the aqe `OpaqueExec`, and one `ExplodingTableProvider` with an error and a panic mode. `RejectOne` becomes `RejectingTaskLauncher::only`. - `JobStateTest` checks job status through one `assert_status`. Assertion messages that never interpolated their format arguments are fixed. - aqe tests: `AdaptivePlanner::from_physical_sql`/`from_logical_sql` (the physical path still skips the preparation rules, the logical path runs them), one `Seen`/`Recording`/`Embedder`, `mock_partition_location`, `statistics_exec`, `coalesce_context` and friends. `StatsExec` replaces `MockPartitionedScan` in the `alter_stages` tests; 17 snapshot lines change, and only the operator's display text differs. - `ballista_core::test_utils::tpch::TPCH_TABLES`, a public const next to `tpch_schema`, used by the scheduler test utilities and the `tpch` benchmark instead of two copies of the list. - `SchedulerTest::new_with_launcher` is built from the same code as `new`. It now pins the static planner like `new`; its one caller sets the adaptive planner explicitly to keep its behavior. - The "Test fixtures" section of the contributor guide (`docs/source/contributors-guide/development.md`) gives general rules for where a fixture goes instead of listing each crate's helpers, so it does not need updating when a helper is added or renamed. # Testing - `cargo test -p ballista-scheduler --all-features`: all pass, including `tests/flight_sql.rs` and `tpch_plan_stability`. - `cargo test -p ballista-scheduler --all-features -- --list` is identical before and after (648 tests). - `cargo fmt --all -- --check` and `cargo clippy -p ballista-scheduler -p ballista-core -p ballista-benchmarks --all-targets --all-features -- -D warnings`. # Are there any user-facing changes? No API changes outside `ballista_core::test_utils::tpch::TPCH_TABLES`, a new public constant. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
