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

Disable useUserOverride in CultureContext #21990

Merged
merged 3 commits into from
Sep 13, 2017

Conversation

khyperia
Copy link
Contributor

@khyperia khyperia commented Sep 8, 2017

This is a test-only change.

CultureInfo has this documentation: The user might choose to override some of the values associated with the current culture of Windows through the regional and language options portion of Control Panel. For example, the user might choose to display the date in a different format or to use a currency other than the default for the culture. If the culture identifier associated with name matches the culture identifier of the current Windows culture, this constructor creates a CultureInfo object that uses those overrides, including user settings for the properties of the DateTimeFormatInfo instance returned by the DateTimeFormat property, and the properties of the NumberFormatInfo instance returned by the NumberFormat property.

This was causing one test on my machine to fail:

Microsoft.CodeAnalysis.CSharp.ExpressionEvaluator.UnitTests.ExpansionTests.Primitives

Assert.Equal() Failure\r\n           ↓ (pos 1)\r\nExpected: {1/1/0001 12:00:00 AM}\r\nActual:   {0001-01-01 00:00:00}\r\n           ↑ (pos 1)

   at Microsoft.CodeAnalysis.ExpressionEvaluator.ResultProviderTestBase.Verify(DkmEvaluationResult actual, DkmEvaluationResult expected)
   at Microsoft.CodeAnalysis.CSharp.ExpressionEvaluator.UnitTests.ExpansionTests.Primitives() in C:\c\roslyn\src\ExpressionEvaluator\CSharp\Test\ResultProvider\ExpansionTests.cs:line 62

The fix is to explicitly override this behavior, by passing useUserOverride = false in a different constructor.

Behavior testing in a console app:

Console.WriteLine(new DateTime());
CultureInfo.CurrentCulture = new CultureInfo("en-US");
Console.WriteLine(new DateTime());
CultureInfo.CurrentCulture = new CultureInfo("en-US", false);
Console.WriteLine(new DateTime());

produces

0001-01-01 00:00:00
0001-01-01 00:00:00
1/1/0001 12:00:00 AM

I don't know what team should review this, so I've just added roslyn-compiler. Feel free to add a area-* and the correct reviewer if there is a more appropriate team.

@khyperia khyperia added the Test Test failures in roslyn-CI label Sep 8, 2017
@khyperia khyperia requested a review from a team September 8, 2017 17:09
@khyperia
Copy link
Contributor Author

khyperia commented Sep 8, 2017

... never mind, apparently that constructor is not available in netstandard1.3, which is what we are compiling for in TestUtilities.csproj.

😢

@khyperia khyperia force-pushed the cultureinfo_override branch from a028bda to 9dda4f0 Compare September 8, 2017 17:29
@@ -476,7 +476,7 @@ void M()
}
}
";
using (new CultureContext("en-US"))
using (new CultureContext(new CultureInfo("en-US", false)))
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

false [](start = 63, length = 5)

Consider using named argument. #Closed

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@TyOverby
Copy link
Contributor

TyOverby commented Sep 8, 2017

LGTM

Regex used:

rg 'new CultureInfo' --files-with-matches | xargs sed -Ei 's/new CultureInfo\("("?[^"]*"?)"(, false)?\)/new CultureInfo("\1", useUserOverride: false)/'
rg 'New CultureInfo' --files-with-matches | xargs sed -Ei 's/New CultureInfo\("("?[^"]*"?)"(, False)?\)/New CultureInfo("\1", useUserOverride:=False)/'
@khyperia
Copy link
Contributor Author

Pushed another commit that both takes into account @AlekseyTs's feedback to use named arguments, as well as doing a regex search for all CultureInfo constructors and fixing those too (but only in tests - there are a couple uses in non-test code, but I left those alone)

@khyperia
Copy link
Contributor Author

retest windows_release_vs-integration_prtest please

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@khyperia khyperia merged commit 5b636eb into dotnet:master Sep 13, 2017
@khyperia khyperia deleted the cultureinfo_override branch September 13, 2017 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-already-signed Test Test failures in roslyn-CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants