Fix and enable Globalization tests that differ based on OS and culture #4625
Conversation
| } | ||
|
|
||
| [DllImport("ntdll.dll")] | ||
| private static extern int RtlGetVersion(out RTL_OSVERSIONINFOEX lpVersionInformation); |
There was a problem hiding this comment.
I think this API intended to be kernel mode and it is not nice to call it from the user mode even if it is working. I would suggest you use IsWindowsVersionOrGreater instead
There was a problem hiding this comment.
This is the function that the new OSDescription API is going to use, according to #4334.
If the function will work for the product, then it should be OK to call in the tests, right?
There was a problem hiding this comment.
Interesting. I didn't know that. I am fine then.
|
beside my comment, everything else LGTM |
| [Theory] | ||
| [InlineData("en-US")] | ||
| [InlineData("br-FR")] | ||
| public void TestLocale(string localeName) |
There was a problem hiding this comment.
While I appreciate the complexities of br-FR's calendar rule, I'm wondering if we couldn't help clarity by having some more fixed points called out in something like
[Theory]
[InlineData("en-US", CalendarWeekRule.FirstDay)]
[InlineData("fr-FR", CalendarWeekRule.SomeConst)]
[InlineData("ja-JP", CalendarWeekRule.SomeConst)]
public void VerifyCalendarWeekRule(string localeName, CalendarWeekRule expected)
{
...just so that we can get breadth-at-a-glance.
Then have br-FR and maybe a couple other complex cases in another test method group.
There was a problem hiding this comment.
@steveharter added this test, so he can comment as he wants.
IMO, adding more cultures won't really help increase the coverage since we are getting this data from ICU. We just need to test that we are getting the values correctly from ICU. Also, adding more cultures increases the chance that we will get conflicting values between all the differing systems we are testing on.
I think the two cultures are sufficient for testing coverage, so the question becomes, should I split the test back out into 2 separate Facts instead of using a Theory.
Fix and enable Globalization tests that differ based on OS and culture
There were a few tests that were disabled and/or had TODO comments in them to clean their handling of culture and OS differences. I've cleaned them up following the established pattern in other tests.
Fix #3243
@ellismg @tarekgh