Add fuzz target for icu_calendar #7051
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. |
Manishearth
force-pushed
the
fuzzer
branch
4 times, most recently
from
28b7ab0 to
5362450
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds a fuzz testing target for the icu_calendar component to help identify potential bugs through automated testing with random inputs. The fuzzer focuses on testing date construction from various field combinations across different calendar systems.
- Adds a new fuzzer that tests date construction with arbitrary inputs including different calendar types, month interpretations, and overflow handling strategies
- Creates the necessary infrastructure files for fuzzing including Cargo.toml, .gitignore, and LICENSE
- Integrates the fuzzer into the main workspace
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| components/calendar/fuzz/fuzz_targets/construction.rs | Main fuzzer implementation testing date construction with arbitrary inputs |
| components/calendar/fuzz/Cargo.toml | Package configuration for the fuzzer with required dependencies |
| components/calendar/fuzz/LICENSE | Unicode license file for the fuzzer directory |
| components/calendar/fuzz/.gitignore | Git ignore patterns for fuzzer artifacts and build outputs |
| Cargo.toml | Workspace member addition for the new fuzzer package |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
Thank you for adding this fuzz target for icu_calendar. It's a great step towards improving the robustness of the calendar APIs. I've left a couple of suggestions to improve the implementation by using more idiomatic Rust patterns, which should make the fuzzer easier to maintain and extend in the future.
Manishearth
force-pushed
the
fuzzer
branch
6 times, most recently
from
b43cbf7 to
01972b5
Compare
Manishearth
force-pushed
the
fuzzer
branch
2 times, most recently
from
ef9ff4a to
325aa38
Compare
| // HijriSimulatedMecca, | ||
| HijriTabularTypeIIThursday, | ||
| HijriUmmAlQura, | ||
| Iso, |
There was a problem hiding this comment.
observation: it's weird that AnyCalendar doesn't have Julian, which is an actual working calendar with use cases, but it does have HijriSimulatedMecca which is not. wasn't the idea that AnyCalendar covers all calendars in the crate?
anyway, Julian is not covered by this code because of this. it would be nice to cover it, but that would require a rewrite in terms of concrete types. at least leave a comment here that it's uncovered.
There was a problem hiding this comment.
Isn't the idea that AnyCalendar covers all calendars in the crate?
It's more "all calendars beyond a certain bar", where that bar was informed by CLDR (but we can add more if we want).
The idea is that adding new calendars to the crate is a much lower bar (though we still want there to be user need)
There was a problem hiding this comment.
I'm just noting that the bar is not applied uniformly, given that simulated Hijri is not a CLDR calendar, we don't know of any use cases for it, and it's buggy.
| let mut fields = DateFields::default(); | ||
| // Temporal only cares about validity in ±270k. We generously test a bit outside of that. | ||
| // We should error on these dates instead, or otherwise handle them: https://github.com/unicode-org/icu4x/issues/7049 | ||
| fields.extended_year = Some(data.year % 1_000_000); |
There was a problem hiding this comment.
nit: not a fan of the mod operation, it modifies the probability distribution in ways I don't want to think about. can you just return and use the next input?
There was a problem hiding this comment.
So my experience with fuzzing is it's better to use whatever information you have somehow. The distribution isn't too important because the fuzzer looks at codepaths and will try and find inputs that hit certain codepaths.
Change-Id: I6a6a696459ba2f142563f69803acf3da433d5c1d
|
I definitely plan to keep iterating on this, but what we have now is a good start. |
2 participants
I talked to @sffc about adding
Arbitraryimpls toicu_calendar, he wanted me to try landing this without that first.As we add more fuzzers we might want to share these impls either with a shared framework file.
We also can just have one fuzzer that does many things. I plan to add more to this fuzzer, e.g. it should attempt both Constrain and Reject, and it should also attempt to hit MissingFieldsStrategy code. It coudl also test arithmetic once we have it.
#7049