Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[apple][HybridGlobalization] Enable more System.Globalization.Tests for Apple Hybrid-Globalization #100240

Closed
matouskozak opened this issue Mar 25, 2024 · 9 comments
Assignees
Milestone

Comments

@matouskozak
Copy link
Member

There are a lot of tests in System.Globalization.Tests that are only enabled for Hybrid-Globalization on Browser. For example:

[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsHybridGlobalizationOnBrowser))]
[MemberData(nameof(LongDatePattern_Get_TestData_HybridGlobalization))]
public void LongDatePattern_Get_ReturnsExpected_HybridGlobalization(DateTimeFormatInfo format, string expected)
{
Assert.Equal(expected, format.LongDatePattern);
}

Some of these tests could be used to verify correct functionality of Apple Hybrid-Globalization.


cc: @mkhamoyan

Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-globalization
See info in area-owners.md if you want to be subscribed.

Copy link
Contributor

Tagging subscribers to 'os-ios': @steveisok, @akoeplinger, @kotlarmilos
See info in area-owners.md if you want to be subscribed.

@mkhamoyan
Copy link
Contributor

I wonder why are these test cases runtime/src/libraries/System.Runtime/tests/System.Globalization.Tests/DateTimeFormatInfo/DateTimeFormatInfoLongDatePattern.cs enabled for only hybrid mode on browser, maybe they are specific to browser hybrid mode flow @ilonatommy ?
If they are general let's enable for all platforms for both using ICU or hybrid mode.

@ilonatommy
Copy link
Member

ilonatommy commented Mar 26, 2024

If they are general let's enable for all platforms for both using ICU or hybrid mode.

They are not specific to hybrid but testing ICU on this is not necessary, in ICU if we loaded the data and it's available for one specific locale, then we can assume all the remaining locales are correct.
In hybrid it's not the case, when for one locale JS can have a correct value, for another one it might return nonsense, especially for DatePatterns where we perform regexing and string operations + JS reply might differ between versions of hosts. A good example is this PR: #90881 where culture-info.ts required changes for ja-JP only and it was caught by these tests. Running them on ICU is in my opinion a waste of testing machine time.

Edit:
For apple I would ask myself a question: is it possible that only few locales are broken while en-US (or some other EFIGS locale) that we most frequently test by default returns correct value? If it is, then enabling them on apple makes sense. Otherwise, one locale per API for apple.

@mkhamoyan
Copy link
Contributor

They are not specific to hybrid but testing ICU on this is not necessary, in ICU if we loaded the data and it's available for one specific locale, then we can assume all the remaining locales are correct. In hybrid it's not the case, when for one locale JS can have a correct value, for another one it might return nonsense, especially for DatePatterns where we perform regexing and string operations + JS reply might differ between versions of hosts. A good example is this PR: #90881 where culture-info.ts required changes for ja-JP only and it was caught by these tests. Running them on ICU is in my opinion a waste of testing machine time.

Edit: For apple I would ask myself a question: is it possible that only few locales are broken while en-US (or some other EFIGS locale) that we most frequently test by default returns correct value? If it is, then enabling them on apple makes sense. Otherwise, one locale per API for apple.

For Apple there should not be broken locales (Apple also uses ICU libs). IMHO running same suite of tests that we were validating ICU should be enough for Apple hybrid mode.

@matouskozak
Copy link
Member Author

matouskozak commented Mar 26, 2024

They are not specific to hybrid but testing ICU on this is not necessary, in ICU if we loaded the data and it's available for one specific locale, then we can assume all the remaining locales are correct. In hybrid it's not the case, when for one locale JS can have a correct value, for another one it might return nonsense, especially for DatePatterns where we perform regexing and string operations + JS reply might differ between versions of hosts. A good example is this PR: #90881 where culture-info.ts required changes for ja-JP only and it was caught by these tests. Running them on ICU is in my opinion a waste of testing machine time.
Edit: For apple I would ask myself a question: is it possible that only few locales are broken while en-US (or some other EFIGS locale) that we most frequently test by default returns correct value? If it is, then enabling them on apple makes sense. Otherwise, one locale per API for apple.

For Apple there should not be broken locales (Apple also uses ICU libs). IMHO running same suite of tests that we were validating ICU should be enough for Apple hybrid mode.

I think that for Apple, we won't have a problem that locale is missing or is broken. However, for ICU Globalization we have to normalize the retrieved values from ICU using NormalizeDatePattern

private static string NormalizeDatePattern(string input)

and I believe the same might be needed for Apple hybrid mode (or we should verify that it is not needed at least).

@ilonatommy
Copy link
Member

ilonatommy commented Mar 26, 2024

@tarekgh, we are trying to understand what was the motivation for skipping some types of tests for globalization methods when developing the libs. E.g. LongDatePattern that was not tested even though we are performing normalization on our side in the libs.
What's more, we are trying to figure out what is the motivation for testing only specific locales. E.g. en-US, fr-FR (


but skipping the rest and how did we choose which ones are worth testing?

@tarekgh
Copy link
Member

tarekgh commented Mar 26, 2024

@ilonatommy we are trying to make the tests stable. The globalization data is changing almost every release of ICU/Windows. If we test all cultures that will make the test unstable. So, we choose subset of the cultures that are stable enough to test against.

@matouskozak
Copy link
Member Author

Completed in #102464

@github-actions github-actions bot locked and limited conversation to collaborators Aug 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants