Localization test failures 1 by MaStr11 · Pull Request #24407 · dotnet/roslyn

MaStr11 · 2018-01-23T21:55:41Z

Fix for #23837.

This is the first of a series of PRs meant to resolve the unit test failures caused by missing localizations.

This PR includes test related to csi.exe and interactive scripting:

Microsoft.CodeAnalysis.CSharp.Scripting.UnitTests

Done. Tested local (de-DE) and CI (en-US).

Microsoft.CodeAnalysis.CSharp.Scripting.Desktop.UnitTests

Done. Tested local (de-DE) and CI (en-US).

Microsoft.CodeAnalysis.VisualBasic.Scripting.UnitTests

Done. Tested local (de-DE) and CI (en-US).

Roslyn.InteractiveHost.UnitTests

Done. Tested local (de-DE) and CI (en-US).

MaStr11 · 2018-01-23T23:13:34Z

src/Scripting/CSharpTest.Desktop/CsiTests.cs

 > 
-", result.Output);
+";
+            expected = expected.Replace((char)0x2013, (char)0x002d); // EN DASH -> HYPHEN-MINUS


csi.exe outputs the en dash in Microsoft (R) Visual C# - interaktive Compilerversion 42.42.42.42424 as hyphen-minus. There might be a more reliable way to fix this.

💭 Seems weird to have an en dash in a resource string.

@MaStr11 Please add a more detailed comment explaining why the replacement is necessary. Something like:

The German translation (and possibly others) contains an en dash (0x2013), but csi.exe outputs it as a hyphen-minus (0x002d). We need to fix up the expected string before we can compare it to the actual output.

…g for VB but I corrected the corresponding test in C#. Changes in C# are partially reverted (test is sipped again) and VB is fixed.

sharwell · 2018-01-23T23:33:11Z

src/Scripting/CSharpTest.Desktop/CsiTests.cs

 > 
-", result.Output);
+";
+            expected = expected.Replace((char)0x2013, (char)0x002d); // EN DASH -> HYPHEN-MINUS


💭 Seems weird to have an en dash in a resource string.

sharwell · 2018-01-23T23:33:34Z

src/Scripting/CSharpTest.Desktop/CsiTests.cs

-Microsoft (R) Visual C# Interactive Compiler version {s_compilerVersion}
-Copyright (C) Microsoft Corporation. All rights reserved.
+            var expected = $@"
+{ string.Format(CSharpScriptingResources.LogoLine1, s_compilerVersion) }


💡 No spaces inside the braces for interpolated strings.

@MaStr11 I should be able to clean this up in the future for all of this repository using automated tools. This feedback is more for future work.

…ests.

sharwell · 2018-01-24T18:37:34Z

TODO: Uncomment //[UseCulture("en-US")] once #24398 is merged

#24398 is now merged.

…nTestFailures1

sharwell · 2018-01-24T18:59:17Z

@MaStr11 Can you update the original comment to replace the TODO text with the current status?

MaStr11 · 2018-01-24T18:59:55Z

#24398 is now merged.

I gave it a try and I can confirm that it indeed works as intended. Ironically it didn't help in this particular case because the test did a process.Start and captured the output from the running csi.exe. So the culture of the test itself changes nothing as this is always tied to the culture of the machine. I fixed it by using the string resource (as it should anyway).

sharwell · 2018-01-24T20:37:06Z

Requesting reviews from @dotnet/roslyn-compiler and @dotnet/roslyn-interactive

jaredpar

Compiler changes look good.

tmat · 2018-01-25T19:08:47Z

src/Scripting/CSharpTest/CommandLineRunnerTests.cs

+            var logoOutput = $@"{ string.Format(CSharpScriptingResources.LogoLine1, s_compilerVersion) }
+{ CSharpScriptingResources.LogoLine2}
+
+{ ScriptingResources.HelpPrompt}";


Could you extract to a static field (similar to s_compilerVersion) and then use the field in all test cases?

MaStr11 · 2018-01-25T19:13:44Z

Sure I could. But there are cases where you want your tests to run in a particular culture using the [UseCulture] attribute. So there is a benefit in keeping the tests self contained.

tmat · 2018-01-25T19:16:52Z

src/Scripting/VisualBasicTest/CommandLineRunnerTests.vb

+VBScriptingResources.LogoLine2 + "

-Type ""#help"" for more information.
+" + ScriptingResources.HelpPrompt + "


Extract to a static field

tmat · 2018-01-25T19:16:55Z

🕐

tmat · 2018-01-25T19:29:49Z

There are perhaps a few of such tests that use a different culture. They would not use this field then. Majority tests do not care about the "logo" lines and it would make them more readable. Also authoring a new test would be easier.

sharwell · 2018-01-25T20:08:53Z

💭 I tend to prefer self-contained tests, and don't see the issue with having the string inlined in multiple tests. However, /src/Scripting/ is not one of the paths where I'm in the code owner group so we tend to defer to the preference of the code owner in these cases.

tmat · 2018-01-25T20:24:30Z

@sharwell The tests are already not self-contained anyways -- they use s_compilerVersion and other helpers.

MaStr11 · 2018-01-25T20:37:46Z

@tmat I'm currently changing the tests. I also facing a problem with the tests that change the Culture during the test (It seems the culture is persisted. So half of the tests run in de-DE while the other run in en-GB). I will need some more time to fix this.

…that swallowed the assert before.

…ulture2 that swallowed the assert before." This reverts commit f3270f6.

…o restore the culture at the end of the test.

MaStr11 · 2018-01-25T22:09:10Z

I'm done with the changes now (Sorry for the messy commit history).

sharwell · 2018-01-25T23:19:18Z

src/Scripting/VisualBasicTest/CommandLineRunnerTests.vb

-
-" + ScriptingResources.HelpPrompt
-            Dim runner = CreateRunner(args:={}, input:="Imports System.Globalization
+            ' Save the current thread culture as it is changed in the test.


❓ The UseCulture attribute should restore the culture at the end of the test. Is it possible to use it here?

There are two differences.

UseCulture sets Current thread.Culture while this test sets CultureInfo.DefaultThreadCulture.

UseCulture also sets the culture at the beginning of the test

I think it is better the way it is but haven't tried UseCulture.

…e1 to use three different cultures. Test shows unexpected results.

…UICulture1 to use three different cultures. Test shows unexpected results." This reverts commit 8e05074.

MaStr11 · 2018-01-26T11:12:35Z

The UseCulture attribute should restore the culture at the end of the test. Is it possible to use it here?

@sharwell I gave it a try in commit 8e05074. I changed UseCultureAttribute to also set DefaultThreadCurrentUICulture. But doing so caused all kinds of unpredictable side-effects (It seemed to me that the UseCultureAttribute.After method is not able to revert to the previous system state by setting DefaultThreadCurrentUICulture. I just gave up after a while. The version of UseCulture pushed in 8e05074 had some bugs, but even the corrected version showed the same results.) and so I reverted the change.

@tmat I found two problems with vbi.exe during my tests:

Inputing >and pressing enter gives a NRE in SyntaxFactory.IsPartOfLinqQueryNotFollowedByNewLine (This error is re-produceable in other circumstances too, but that is the easiest one). Is this a known bug or should I file an issue?
Is it expected that ? New System.DateTime always outputs #1/1/0001 12:00:00 AM# no matter what DefaultThreadCurrentUICulture is set?

MaStr11 · 2018-01-26T16:09:38Z

src/Compilers/CSharp/Portable/CSharpCodeAnalysis.csproj

    <InternalsVisibleToTest Include="Microsoft.CodeAnalysis.Scripting.Destkop.UnitTests" />
    <InternalsVisibleToTest Include="Microsoft.CodeAnalysis.CSharp.Scripting.UnitTests" />
    <InternalsVisibleToTest Include="Microsoft.CodeAnalysis.CSharp.Scripting.Desktop.UnitTests" />
+    <InternalsVisibleToTest Include="Roslyn.InteractiveHost.UnitTests" />


@sharwell Since you asked for approval for InternalsVisibleToTest in #24460 you may also want approval for this one.

👍 This one should be covered under the umbrella of the review from @jaredpar.

sharwell · 2018-01-29T15:58:02Z

@tmat Can you answer the two final questions from @MaStr11 in the last comment above?

AlekseyTs · 2018-03-06T19:35:46Z

@sharwell This PR was merged without getting required sign-offs from the compiler team and it doesn't deal with localization issues in the manner that other compiler tests do. I think it should be reverted and different fixes should be made.

sharwell · 2018-03-07T02:28:47Z

@AlekseyTs This pull request did not touch compiler code.

AlekseyTs · 2018-03-07T03:44:45Z

This PR made changes to CSharpCodeAnalysis.csproj, it also touched Scripting and Csi tests, even though they are not under Compilers folder, they used to be owned by Compiler team at some point. @jaredpar To clarify the current ownership for the tests.

Fixed Microsoft.CodeAnalysis.CSharp.Scripting.UnitTests.dll

a3f8221

MaStr11 requested a review from a team as a code owner January 23, 2018 21:55

Fixes for Microsoft.CodeAnalysis.CSharp.Scripting.Desktop.UnitTests

27fe33a

MaStr11 commented Jan 23, 2018

View reviewed changes

MaStr11 added 2 commits January 24, 2018 10:12

Fixes for Microsoft.CodeAnalysis.VisualBasic.Scripting.UnitTests

4e03e48

Fixes for Roslyn.InteractiveHost.UnitTests

7abb2a7

MaStr11 requested a review from a team as a code owner January 24, 2018 10:08

MaStr11 added 2 commits January 24, 2018 11:45

Fixed regression in TestDisplayResultsWithCurrentUICulture

c6a5c7b

Follow up to TestDisplayResultsWithCurrentUICulture : Test was failin…

377de8e

…g for VB but I corrected the corresponding test in C#. Changes in C# are partially reverted (test is sipped again) and VB is fixed.

MaStr11 changed the title ~~Localization test failures 1 [WIP]~~ Localization test failures 1 Jan 24, 2018

This was referenced Jan 24, 2018

Localization test failures 2 #24424

Merged

Running Test.cmd on a non english machine causes lots of unit tests to fail #23837

Open

Localization test failures 3 #24426

Merged

sharwell approved these changes Jan 24, 2018

View reviewed changes

Added comment explaining the dash <-> minus replacement for csi.exe t…

a8826a2

…ests.

MaStr11 added 2 commits January 24, 2018 19:39

Merge remote-tracking branch 'upstream/dev15.6.x' into FixLocalizatio…

104b4b9

…nTestFailures1

Fixed LineNumber_Information_On_Exception without [UseCulture]

f7338cd

jaredpar reviewed Jan 25, 2018

View reviewed changes

sharwell self-assigned this Jan 25, 2018

sharwell added this to the 15.6 milestone Jan 25, 2018

tmat reviewed Jan 25, 2018

View reviewed changes

MaStr11 added 4 commits January 25, 2018 21:47

Centralized compiler logo and help prompt output.

549626b

Changed try-finally block in TestDisplayResultsWithCurrentUICulture2 …

f3270f6

…that swallowed the assert before.

Revert "Changed try-finally block in TestDisplayResultsWithCurrentUIC…

c5e7ba4

…ulture2 that swallowed the assert before." This reverts commit f3270f6.

Also wrapped TestDisplayResultsWithCurrentUICulture1 in try-finally t…

bba566c

…o restore the culture at the end of the test.

sharwell reviewed Jan 25, 2018

View reviewed changes

MaStr11 added 2 commits January 26, 2018 11:22

Test failure: Changed VB script TestDisplayResultsWithCurrentUICultur…

8e05074

…e1 to use three different cultures. Test shows unexpected results.

Revert "Test failure: Changed VB script TestDisplayResultsWithCurrent…

7dcf050

…UICulture1 to use three different cultures. Test shows unexpected results." This reverts commit 8e05074.

sharwell modified the milestones: 15.6, 15.7 Jan 26, 2018

sharwell changed the base branch from dev15.6.x to dev15.7.x January 26, 2018 15:56

MaStr11 commented Jan 26, 2018

View reviewed changes

sharwell mentioned this pull request Jan 28, 2018

Formatter should remove spaces inside interpolated string braces #24498

Closed

MaStr11 mentioned this pull request Jan 30, 2018

Localization test failures 4 #24539

Closed

sharwell changed the base branch from dev15.7.x to dev15.7.x-vs-deps February 14, 2018 16:09

sharwell changed the base branch from dev15.7.x-vs-deps to dev15.7.x February 14, 2018 16:09

sharwell merged commit 6bfea1a into dotnet:dev15.7.x Feb 14, 2018

MaStr11 deleted the FixLocalizationTestFailures1 branch March 2, 2018 15:17

AlekseyTs self-assigned this Mar 6, 2018

Conversation

MaStr11 commented Jan 23, 2018 • edited by sharwell Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Microsoft.CodeAnalysis.CSharp.Scripting.UnitTests

Microsoft.CodeAnalysis.CSharp.Scripting.Desktop.UnitTests

Microsoft.CodeAnalysis.VisualBasic.Scripting.UnitTests

Roslyn.InteractiveHost.UnitTests

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell commented Jan 24, 2018

Uh oh!

sharwell commented Jan 24, 2018

Uh oh!

MaStr11 commented Jan 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sharwell commented Jan 24, 2018

Uh oh!

jaredpar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaStr11 commented Jan 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmat commented Jan 25, 2018

Uh oh!

tmat commented Jan 25, 2018

Uh oh!

sharwell commented Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tmat commented Jan 25, 2018

Uh oh!

MaStr11 commented Jan 25, 2018

Uh oh!

MaStr11 commented Jan 25, 2018

Uh oh!

sharwell Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaStr11 commented Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell commented Jan 29, 2018

Uh oh!

AlekseyTs commented Mar 6, 2018

Uh oh!

sharwell commented Mar 7, 2018

Uh oh!

AlekseyTs commented Mar 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

MaStr11 commented Jan 23, 2018 •

edited by sharwell

Loading

MaStr11 commented Jan 24, 2018 •

edited

Loading

sharwell commented Jan 25, 2018 •

edited

Loading

sharwell Jan 25, 2018 •

edited

Loading

MaStr11 commented Jan 26, 2018 •

edited

Loading