-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Fix and enable Globalization tests that differ based on OS and culture #4625
Changes from all commits
2aa3b48
2a33513
c8dbb89
ac97bd7
d23c8b7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,12 +2,15 @@ | |
// Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||
|
||
using System.Runtime.InteropServices; | ||
using Xunit; | ||
|
||
namespace System.Globalization.Tests | ||
{ | ||
internal static class DateTimeFormatInfoData | ||
{ | ||
private static bool s_isWindows = RuntimeInformation.IsOSPlatform(OSPlatform.Windows); | ||
private static int s_WindowsVersion = GetWindowsVersion(); | ||
private static bool s_isOSX = RuntimeInformation.IsOSPlatform(OSPlatform.OSX); | ||
|
||
public static string GetEraName(CultureInfo cultureInfo) | ||
{ | ||
|
@@ -41,11 +44,144 @@ public static string GetAbbreviatedEraName(CultureInfo cultureInfo) | |
throw GetCultureNotSupportedException(cultureInfo); | ||
} | ||
|
||
private static Exception GetCultureNotSupportedException(CultureInfo cultureInfo) | ||
internal static string[] GetDayNames(CultureInfo cultureInfo) | ||
{ | ||
return new NotSupportedException(string.Format("The culture '{0}' with calendar '{1}' is not supported.", | ||
cultureInfo.Name, | ||
if (string.Equals(cultureInfo.Name, "en-US", StringComparison.OrdinalIgnoreCase)) | ||
{ | ||
return new string[] | ||
{ | ||
"Sunday", | ||
"Monday", | ||
"Tuesday", | ||
"Wednesday", | ||
"Thursday", | ||
"Friday", | ||
"Saturday" | ||
}; | ||
} | ||
if (string.Equals(cultureInfo.Name, "fr-FR", StringComparison.OrdinalIgnoreCase)) | ||
{ | ||
string[] dayNames = new string[] | ||
{ | ||
"dimanche", | ||
"lundi", | ||
"mardi", | ||
"mercredi", | ||
"jeudi", | ||
"vendredi", | ||
"samedi" | ||
}; | ||
|
||
if (s_isOSX) | ||
{ | ||
CapitalizeStrings(dayNames); | ||
} | ||
return dayNames; | ||
} | ||
|
||
throw GetCultureNotSupportedException(cultureInfo); | ||
} | ||
|
||
internal static string[] GetAbbreviatedDayNames(CultureInfo cultureInfo) | ||
{ | ||
if (string.Equals(cultureInfo.Name, "en-US", StringComparison.OrdinalIgnoreCase)) | ||
{ | ||
return new string[] | ||
{ | ||
"Sun", | ||
"Mon", | ||
"Tue", | ||
"Wed", | ||
"Thu", | ||
"Fri", | ||
"Sat" | ||
}; | ||
} | ||
if (string.Equals(cultureInfo.Name, "fr-FR", StringComparison.OrdinalIgnoreCase)) | ||
{ | ||
string[] dayNames = new string[] | ||
{ | ||
"dim.", | ||
"lun.", | ||
"mar.", | ||
"mer.", | ||
"jeu.", | ||
"ven.", | ||
"sam." | ||
}; | ||
|
||
if (s_isOSX) | ||
{ | ||
CapitalizeStrings(dayNames); | ||
} | ||
return dayNames; | ||
} | ||
|
||
throw GetCultureNotSupportedException(cultureInfo); | ||
} | ||
|
||
private static void CapitalizeStrings(string[] strings) | ||
{ | ||
for (int i = 0; i < strings.Length; i++) | ||
{ | ||
strings[i] = strings[i].Substring(0, 1).ToUpper() + strings[i].Substring(1); | ||
} | ||
} | ||
|
||
internal static CalendarWeekRule GetCalendarWeekRule(CultureInfo cultureInfo) | ||
{ | ||
if (string.Equals(cultureInfo.Name, "en-US", StringComparison.OrdinalIgnoreCase)) | ||
{ | ||
return CalendarWeekRule.FirstDay; | ||
} | ||
if (string.Equals(cultureInfo.Name, "br-FR", StringComparison.OrdinalIgnoreCase)) | ||
{ | ||
if (s_isWindows && s_WindowsVersion < 10) | ||
{ | ||
return CalendarWeekRule.FirstFullWeek; | ||
} | ||
else | ||
{ | ||
return CalendarWeekRule.FirstFourDayWeek; | ||
} | ||
} | ||
|
||
throw GetCultureNotSupportedException(cultureInfo); | ||
} | ||
|
||
public static Exception GetCultureNotSupportedException(CultureInfo cultureInfo) | ||
{ | ||
return new NotSupportedException(string.Format("The culture '{0}' with calendar '{1}' is not supported.", | ||
cultureInfo.Name, | ||
cultureInfo.Calendar.GetType().Name)); | ||
} | ||
|
||
public static int GetWindowsVersion() | ||
{ | ||
if (s_isWindows) | ||
{ | ||
RTL_OSVERSIONINFOEX osvi = new RTL_OSVERSIONINFOEX(); | ||
osvi.dwOSVersionInfoSize = (uint)Marshal.SizeOf(osvi); | ||
Assert.Equal(0, RtlGetVersion(out osvi)); | ||
return (int)osvi.dwMajorVersion; | ||
} | ||
|
||
return -1; | ||
} | ||
|
||
[DllImport("ntdll.dll")] | ||
private static extern int RtlGetVersion(out RTL_OSVERSIONINFOEX lpVersionInformation); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting. I didn't know that. I am fine then. |
||
|
||
[StructLayout(LayoutKind.Sequential)] | ||
internal struct RTL_OSVERSIONINFOEX | ||
{ | ||
internal uint dwOSVersionInfoSize; | ||
internal uint dwMajorVersion; | ||
internal uint dwMinorVersion; | ||
internal uint dwBuildNumber; | ||
internal uint dwPlatformId; | ||
[MarshalAs(UnmanagedType.ByValTStr, SizeConst = 128)] | ||
internal string szCSDVersion; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.