Repository navigation
test: Add a shared framework for reproducible fuzz tests - #27048
2010YOUY01 wants to merge 9 commits into
Conversation
| let suites = [Suite::new( | ||
| &spill_pool, | ||
| RunConfig { | ||
| run_seed: Some(0), |
There was a problem hiding this comment.
This is the existing fuzz config, probably we can use rotating seed (timestamp), and time limit instead of 1000 cases.
github-actions
Bot
added
development-process
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
The basic idea looks pretty good to me -- thank you @2010YOUY01
What I don't understand is the new crate structure and then the need to partially expose some of the existing testing code from the physical expr crate
| - name: Run fuzzers with more iterations | ||
| - name: Run fuzz tests | ||
| run: cargo xtask ci step test fuzz | ||
| # Migrate it to the above unified fuzz runner over time |
There was a problem hiding this comment.
Can you please file a ticket that explains the migration plan and has a checklist of tests to run and link it here? So we have the context in the future?
|
|
||
| mod suites; | ||
|
|
||
| /// The single registration point for project-wide fuzz campaigns. |
There was a problem hiding this comment.
what is a fuzz campaign? I am not familar with the campaign terminology
| # specific language governing permissions and limitations | ||
| # under the License. | ||
|
|
||
| # A private crate providing a single entry point for all fuzz tests. |
There was a problem hiding this comment.
why did you make a new crate? Why not just a module in the existing datafusion crate?
There was a problem hiding this comment.
see the longer comment below
| use datafusion_common::instant::Instant; | ||
| use datafusion_common::{Result, exec_err}; | ||
|
|
||
| /// A component's fuzz experiment, run by the centralized driver. |
There was a problem hiding this comment.
it may help to add a link here to the centralized driver
|
@alamb THanks for the review!
Exposing some testing util was intended to keep the PR small, but this make the structure confusing. This is fixed in dd6f892: the idea is the fuzzing implementation all lives in RE another major concern for this PR:
I think this structure is clearer. Otherwise, integration tests would include regular, extended (slow), and fuzzing tests, making it awkward to maintain or run only a subset of them. Since splitting them doesn't seem to introduce any other issues, I went with the clearer structure. |
Important
Fork CI setup: For pull requests from a fork to
main, enable workflows in your fork's Actions tab before you push.See the fork CI setup guide.
Which issue does this PR close?
Rationale for this change
Fuzz tests should follow these conventions:
the driver can run on a best-effort basis within a budget such as one minute.
Most of the existing implementations don't follow all of these principles, or implemented it with unnecessary complexity:
Refactoring to a shared trait can prevent these issues in the future.
This PR introduces a trait to enforce these conventions and migrates one fuzz unit test to use it. Other existing fuzzers remain available for later migration.
Demo
What changes are included in this PR?
Read the key changes in the following order; the remaining changes are minor.
FuzzTestintest-utils/src/fuzz.rs: a trait that ensures all fuzz testsfollow the intended conventions.
Suiteindatafusion/fuzz/tests/fuzz.rs: a single entry point for all fuzzunit tests in DataFusion.
SpillPoolFuzzerindatafusion/fuzz/tests/suites/spill_pool.rs: the first unittest migrated to the centralized
FuzzTesttrait and unified runner.The key API is introduced in step 1. After implementing several fuzz tests in datafusion, I came up with this design to capture their common patterns. I also asked Codex to check whether all existing fuzz tests can be migrated to this trait.
Step 2: using a unified test entry point make it easier to configure fuzzing time in the future. For example it's possible to deploy a long-running background fuzzing job.
What is the testing strategy for this PR?
Are there any user-facing changes?