Repository navigation
ci: stop re-running the workspace test suite in the extended tests job - #26649
Merged
Merged
Conversation
alamb
approved these changes
Oct 23, 2026
alamb
left a comment
alamb
left a comment
Contributor
There was a problem hiding this comment.
Thanks @jayzhan211 -- I think this is an improvement
I have an idea how to further simplify for your consideration too
| "avro,json,backtrace,extended_tests,recursive_protection,parquet_encryption", | ||
| "avro,backtrace,extended_tests,parquet_encryption", | ||
| "--", | ||
| // Tests behind the `extended_tests` feature. A new test |
Contributor
There was a problem hiding this comment.
But I worry about this list and the names of the tests getting out of sync.
Maybe as a follow on PR we could move all these "extended" tests into their own binary rather than having to have a feature flag and a list here
cargo test --test core_integration_extendedOr something like that 🤔
Contributor
|
good, thanks :) |
Contributor
Author
2010YOUY01
pushed a commit
to 2010YOUY01/arrow-datafusion
that referenced
this pull request
Oct 26, 2026
…pache#26694) ## Which issue does this PR close? - None. Follow-up to the review of apache#26649. ## Rationale for this change After apache#26649, the `extended` CI step picks its tests with name filters (`fuzz_cases::`, `memory_limit_validation::`, `spill_pool_scenario_fuzz`). If a test behind the `extended_tests` feature lives somewhere the filters do not match, or a filtered test is renamed, the filter matches nothing, the step still passes, and the test silently stops running in CI. ## What changes are included in this PR? - The tests behind `extended_tests` now live in one test target, `extended_tests`, which requires the feature as a whole (`required-features = ["extended_tests"]` in `Cargo.toml`). It replaces the `fuzz` target and holds: - `fuzz_cases` (unchanged contents and test paths). - `memory_limit_validation`, which moves out of `core_integration`. The three `#[cfg(feature = "extended_tests")]` lines in the source are gone. - The `extended` step is now `cargo test -p datafusion --test extended_tests --features ...`, with no filters. A new test in the target runs without touching the step, and renaming the target makes cargo fail. - The spill pool fuzzer (1000 iterations) moves to its own step, `spill-pool-fuzz`, run as a second step of the same job. It cannot share the `extended` step, because a test filter applies to every test binary in the run. It still selects its test by path. - Two xtask tests guard what cargo cannot check. xtask is a workspace member, so they run on every PR: - `spill_pool_fuzz_test_exists` fails if the path the `spill-pool-fuzz` step uses no longer names a test. - `extended_tests_feature_only_gates_whole_targets` fails if any source file gates code on `extended_tests` with a `cfg`, since such a test would never run in CI. - The memory validation runner re-runs each test in a child process by its path. It now also fails if that path selects no test, which would otherwise pass. Moving the tests changes those paths, so this checks the move too. ## What is the testing strategy for this PR? - `cargo xtask ci step test extended` runs the same tests as before: 134 fuzz tests, and 21 plus 1 already ignored memory validation tests. - `cargo xtask ci step test spill-pool-fuzz` runs exactly 1 test. - `cargo test -p datafusion --test extended_tests` without the feature fails with "requires the features: `extended_tests`", and `--tests` without the feature skips the target, so the `workspace` step no longer builds it. - The two new xtask tests fail when the spill pool fuzzer is renamed, when its file is moved, and when a `#[cfg(feature = "extended_tests")]` is added to `core_integration.rs`. The memory validation runner fails when its test path prefix is wrong. - The `spill-pool-fuzz` step only compiles the `datafusion-physical-plan` unit test binary, which the `extended` step also built before this PR. Fuzz and memory validation tests now run in one process. Each memory validation test measures the RSS of its own child process, so the fuzz tests running next to it do not change what it measures. ## Are there any user-facing changes? No.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
The
extended_testsjob runs for every merge queue entry (778 times in the last 30 days, on 32-CPU runners), and most of what it runs is a second copy of the workspace test suite. Thecargo test (amd64)job already runs that suite on the same commit in the merge queue.In a recent merge queue run (39907001762) the job spent 555s running tests:
tests/fuzz.rs(only in this job)cargo test (amd64))cargo test (amd64), except the 1000-iteration spill pool fuzz)cargo test (amd64))What changes are included in this PR?
The
extendedxtask step now runs only the tests that theworkspacestep does not:memory_limit_validation, which are behind theextended_testsfeaturespill_pool_scenario_fuzz, which this job runs with 1000 iterationsIt builds only
datafusionanddatafusion-physical-plantest targets and uses test name filters to skip their other tests. The job name and required check are unchanged.Nothing that ran before is lost:
extended_testsis only defined indatafusionand only used bytests/fuzz.rsandtests/memory_limit/mod.rs.workspacestep runs every other test with a superset of the features.recursive_protectionis a default feature, andjsononly applies to the proto crates, which theworkspacestep tests withjson.ffi_example_table_provider, which theworkspacestep excludes, has no tests.Tradeoff
The step now lists the tests it runs by name. If a new test is put behind the
extended_testsfeature outsidefuzz_casesormemory_limit_validation, it also needs a filter in this step, otherwise it never runs in CI. A comment next to the filters says this.What is the testing strategy for this PR?
The xtask snapshot test is updated. Running
cargo xtask ci step test extendedlocally runs exactly the expected 156 tests and filters out the rest:datafusionlibcore_integrationfuzzdatafusion-physical-planlibWithout the filters, the same targets run 4675 tests, and all pass.
Based on the run above, this should cut the job's test time from about 555s to about 325s, plus a smaller build. That is roughly 4 minutes per merge queue run.
Are there any user-facing changes?
No.