Create fuzzers for testing correctness of parsing, linting and fixing #4822
Conversation
PR Check ResultsEcosystem✅ ecosystem check detected no changes. BenchmarkLinuxWindows |
|
This looks pretty cool! I'm looking forward to seeing this uncover more bugs — the two you linked already are nice finds. |
Thanks! I hope it gets some good use -- manually sifting through the bugs right now to deduplicate, but hopefully once all the low-hanging fruit issues are out of the way we can integrate it into the standard testing process. 😁 |
|
Also, it should be fairly straightforward to extend the idempotency ideas to #4798 for testing the formatter as well, when this is completed. |
.github/workflows/ci.yaml
Outdated
| - name: "Install Rust toolchain" | ||
| run: rustup show | ||
| - uses: Swatinem/rust-cache@v2 | ||
| - run: cargo install cargo-fuzz |
There was a problem hiding this comment.
Does cargo-bininstall support cargo-fuzz? It could help to speed up the CI step
There was a problem hiding this comment.
I am unfamiliar with bininstall. I will try this 🙂
There was a problem hiding this comment.
Hm, looking at the CI, it seems that none of the other installations in ci.yaml use binstall. Perhaps we should make this a separate PR instead.
There was a problem hiding this comment.
Good point. They probably should. But I understand we want to remove the CI step for now anyway because there would be too many false positives. I recommend either dropping the CI step or adding it in its own PR to resolve the conversation for now.
There was a problem hiding this comment.
We want to keep the build step. In the future, we should add the fuzz runs as another CI step as well.
crates/ruff/src/test.rs
Outdated
| contents: &str, | ||
| path: &Path, | ||
| settings: &Settings, | ||
| max_iterations: usize, |
There was a problem hiding this comment.
Same as for test_path. We should extract the logic into a TestContentsRunner and use it inside of test_contents. This way, it becomes possible for you to use the advanced runner API without having to change all call sites (and reduces the number of arguments).
|
This last commit adds some sugar so I can fuzz locally with libafl (disclosure: this is a fuzzer I am a maintainer of), which is orders of magnitude faster than libfuzzer but has less support. It's not default and shouldn't affect normal users. |
|
It would be glorious to add this to OSS-Fuzz <3 |
|
That's the plan 😉 |
|
Potentially, yes 🙂 Though, this seems really roundabout. Why does this need to be a global/constant? Should this perhaps be a Setting entry? This pattern also appears here: https://github.com/charliermarsh/ruff/blob/1ed5d7e437a1dda0f4c2ddae09f5a7aa7d713bde/crates/ruff/src/linter.rs#L247 |
The settings is what we use in production. I would prefer to keep max iterations local to the testing infrastructure. |
|
I see. This would work in my case, yes. Applying the change! |
…complicated (more precision)
…#4822) Co-authored-by: Micha Reiser <micha@reiser.io>
Summary
This PR introduces multiple fuzzers which test the correctness of Ruff. Namely:
ruff_parse_simple, which attempts to simply crash the parser (though is mainly useful as a utility for generating inputs)ruff_parse_idempotency, which searches for idempotency violations in the parse/unparse utilities of Ruffruff_fix_validity, which checks that fixes applied by Ruff do not introduce syntax violations (usingruff::test::test_snippet)Test Plan
This introduces new tests. I will open PRs with a few of the bugs discovered by the fuzzer and link them to this PR to demonstrate some of the things it is able to find.