Add regression tests for locale default hour cycle (Fixes #594)#7652
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive regression tests for issue #594, ensuring that datetime formatting correctly falls back to locale-specific default hour cycles based on CLDR region data when no explicit -u-hc- preference is provided.
Changes:
- Added datagen test to validate that
preferred_hour_cycle()correctly infers hour cycles from CLDR data - Added runtime test to verify that datetime formatting uses the correct default hour cycle for various locales
- Included tests for explicit
-u-hc-overrides and midnight edge cases
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| provider/source/src/datetime/semantic_skeletons/tests.rs | Adds test_preferred_hour_cycle_by_locale() to validate datagen pipeline correctly infers locale-specific hour cycles from CLDR patterns |
| components/datetime/tests/simple_test.rs | Adds test_locale_default_hour_cycle() to verify runtime formatting behavior with default and explicit hour cycle preferences |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (locale_str, expected_hour_cycle) in cases { | ||
| let locale: Locale = locale_str.parse().unwrap(); | ||
| let prefs = DateTimeFormatterPreferences::from(&locale); | ||
| let formatter = FixedCalendarDateTimeFormatter::<icu_calendar::Gregorian, _>::try_new( |
There was a problem hiding this comment.
Consider importing Gregorian from icu_calendar at the top of the file and using the shorter Gregorian instead of the fully qualified icu_calendar::Gregorian. This would be consistent with other test files like resolved_components.rs that import and use Gregorian directly.
sffc
left a comment
There was a problem hiding this comment.
I think we have tests for this already, but adding additional tests specifically for the components::Bag behavior as integration tests is harmless and directly fixes the issue.
Thanks for the review |
|
Please fix the merge conflict |
4d3bd27 to
3621767
Compare
|
Please fix clippy. Thanks |
| .unwrap(); | ||
| let formatted = formatter.format(&datetime); | ||
| let resolved_pattern = formatted.pattern(); | ||
| let bag = components::Bag::from(&resolved_pattern); |
There was a problem hiding this comment.
Observation: the main thing you are testing in this PR is the behavior of components::Bag::hour_cycle.
There is one small test that does this, test_length_time
We also have test_hour_cycle_selection which tests the hour cycle resolution through DateTimeFormatterPreferences but not all the way through to components::Bag
So I lean toward believing that this test is a value-add, but we have this behavior mostly already tested elsewhere.
I would be more confident if you moved the test into resolved_components.rs and described it as being a more thorough test of components::Bag::hour_cycle.
sffc
left a comment
There was a problem hiding this comment.
(@afrdbaig7 please address my comment from above)
|
sffc
left a comment
There was a problem hiding this comment.
Thanks, please fix the fmt error and then we can merge
Ok i will fix it and push the update shortly |
f70e40e to
463dbc5
Compare
463dbc5 to
4f7319f
Compare
|
@sffc Thanks for your help I’ve addressed your comment. |
Fixes #594
This PR adds regression tests to make sure datetime formatting falls back
to the correct default hour cycle based on CLDR region data when no
explicit -u-hc- preference is provided.
I added both runtime and datagen tests so that the behavior is covered
at formatting time as well as at the data level.
Runtime tests (simple_test.rs):
Datagen test (semantic_skeletons/tests.rs):
CoarseHourCycle:
Note: The runtime tests require
feature = "experimental"forcomponents::Bagaccess.These tests should help prevent regressions and make the expected
locale-based hour cycle behavior more clearly covered.
Changelog
Add regression tests for
components::Bag::hour_cycleresolution across locales.