Partially fix localization issues in tests#24460
Partially fix localization issues in tests#24460pdelvo wants to merge 5 commits intodotnet:dev15.7.xfrom
Conversation
sharwell
left a comment
There was a problem hiding this comment.
Marked as request changes for the question marked with ❓ .
| <InternalsVisibleTo Include="Microsoft.CodeAnalysis.CompilerServer" /> | ||
| <InternalsVisibleTo Include="VBCSCompiler" /> | ||
| <InternalsVisibleTo Include="VBCSCompilerPortable" /> | ||
| <InternalsVisibleToTest Include="Roslyn.Compilers.CompilerServer.UnitTests" /> |
There was a problem hiding this comment.
@dotnet/roslyn-compiler Do we need any special approval for IVT for test projects?
|
|
||
| string innerString = string.Format(CSharpResources.ERR_CantOpenFileWrite, libDll.Path, ""); | ||
|
|
||
| // Remove the last ' or " |
There was a problem hiding this comment.
💡 Can you expand this comment to explain why removal is necessary?
| Diagnostic(ErrorCode.ERR_InvalidDebugInformationFormat).WithArguments("-1").WithLocation(1, 1), | ||
| // error CS2041: Invalid output name: Name cannot start with whitespace. | ||
| Diagnostic(ErrorCode.ERR_InvalidOutputName).WithArguments(CodeAnalysisResources.NameCannotStartWithWhitespace).WithLocation(1, 1), | ||
| Diagnostic(ErrorCode.ERR_InvalidOutputName).WithArguments("Name cannot start with whitespace.").WithLocation(1, 1), |
There was a problem hiding this comment.
This is the same error as this one. @AlekseyTs suggestion there was to hardcode the string as @pdelvo did.
|
|
||
| Assert.Equal(result.ExitCode, 1); | ||
| Assert.Equal(@"test.vb(1) : error BC30037: Character is not valid. | ||
| Assert.Equal($@"test.vb(1) : error BC30037: {ReplaceNonASCIICharacters(VBResources.ERR_IllegalChar)} |
There was a problem hiding this comment.
❔ Why is the call to ReplaceNonASCIICharacters necessary?
➡️ The use of ERR_IllegalChar is intended to avoid the need to write non-ASCII characters to the output. Ironically, this does not work as expected due to localization.
There was a problem hiding this comment.
The problem is that the error message contains non ascii characters ("Ungültiges Zeichen."). So because the pipe is set to ASCII the "ü" is replaced by "?".
The desireable output for german would probably be "Ungueltiges Zeichen." (replacing ü by ue) or maybe use a different word without them, maybe "Unerlaubtes Zeichen", but I dont think you can do this for every language. For languages with a completely different character set (e.g. chinese) this test becomes completely useless.
| <InternalsVisibleTo Include="Microsoft.CodeAnalysis.CompilerServer" /> | ||
| <InternalsVisibleTo Include="VBCSCompiler" /> | ||
| <InternalsVisibleTo Include="VBCSCompilerPortable" /> | ||
| <InternalsVisibleToTest Include="Roslyn.Compilers.CompilerServer.UnitTests"/> |
There was a problem hiding this comment.
@dotnet/roslyn-compiler Here is a second IVT for tests to confirm.
|
IVT to test still needs to be reviewed but is a okay generally. IVT to non-test is what we're really concerned about tohugh. |
AlekseyTs
left a comment
There was a problem hiding this comment.
All compiler tests should request diagnostic messages in english, there should be no need for getting localized resource in tests for the purpose of verifying diagnostics.
|
@pdelvo I'm working with Aleksey now on figuring out the specific expectations so we aren't getting blocked on these. |
|
|
||
| var outWriter = new StringWriter(CultureInfo.InvariantCulture); | ||
| int exitCode = new MockCSharpCompiler(null, dir.Path, new[] { "/target:library", libSrc.Path }).Run(outWriter); | ||
| Assert.Contains($"error CS2012: Cannot open '{libDll.Path}' for writing", outWriter.ToString()); |
There was a problem hiding this comment.
Assert.Contains($"error CS2012: Cannot open '{libDll.Path}' for writing", outWriter.ToString()); [](start = 12, length = 96)
Please add /preferreduilang:en switch instead, see how other tests do this.
|
|
||
| var outWriter = new StringWriter(CultureInfo.InvariantCulture); | ||
| int exitCode = new MockCSharpCompiler(null, dir.Path, new[] { "/target:library", libSrc.Path }).Run(outWriter); | ||
| Assert.Contains($"error CS2012: Cannot open '{libDll.Path}' for writing", outWriter.ToString()); |
There was a problem hiding this comment.
Assert.Contains($"error CS2012: Cannot open '{libDll.Path}' for writing", outWriter.ToString()); [](start = 12, length = 96)
Same suggestion
| var refDll = Path.Combine(dir.Path, Path.Combine("ref", "a.dll")); | ||
| Assert.False(File.Exists(refDll)); | ||
|
|
||
| Assert.Equal("a.cs(1,39): error CS0103: The name 'error' does not exist in the current context", outWriter.ToString().Trim()); |
There was a problem hiding this comment.
Assert.Equal("a.cs(1,39): error CS0103: The name 'error' does not exist in the current context", outWriter.ToString().Trim()); [](start = 12, length = 126)
Same suggestion
| Assert.True(mItem1.GetAttributes().IsEmpty); | ||
| Assert.Equal("error CS8128: Member 'Item1' was not found on type 'ValueTuple<T1, T2>' from assembly 'comp, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null'.", | ||
| Assert.Equal($"error CS8128: {string.Format(CSharpResources.ERR_PredefinedTypeMemberNotFoundInAssembly, "Item1", "ValueTuple<T1, T2>", "comp, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null")}", | ||
| mItem1.GetUseSiteDiagnostic().ToString()); |
There was a problem hiding this comment.
ToString [](start = 55, length = 8)
Please call overload that takes IFormatProvider instead and pass english culture, we have a helper that we use in tests to get that culture EnsureEnglishUICulture.PreferredOrNull.
| // (9,11): error CS1503: Argument 1: cannot convert from '(int, <null>)' to 'int' | ||
| // M((1, null)); | ||
| Diagnostic(ErrorCode.ERR_BadArgType, "(1, null)").WithArguments("1", "(int, <null>)", "int").WithLocation(9, 11)); | ||
| Diagnostic(ErrorCode.ERR_BadArgType, "(1, null)").WithArguments("1", $"(int, {MessageID.IDS_NULL.Localize()})", "int").WithLocation(9, 11)); |
There was a problem hiding this comment.
Diagnostic(ErrorCode.ERR_BadArgType, "(1, null)").WithArguments("1", $"(int, {MessageID.IDS_NULL.Localize()})", "int").WithLocation(9, 11)); [](start = 16, length = 140)
I believe this failure is caused by a real bug in compiler and changing the test will only hide it. See #24603
| // (13,64): error CS0117: '(string, (int, (<null>, int)))' does not contain a definition for 'M' | ||
| // System.Console.WriteLine(("qq", (Alice: 1, (null, 3))).M()); | ||
| Diagnostic(ErrorCode.ERR_NoSuchMember, "M").WithArguments("(string, (int, (<null>, int)))", "M").WithLocation(13, 64), | ||
| Diagnostic(ErrorCode.ERR_NoSuchMember, "M").WithArguments($"(string, (int, ({MessageID.IDS_NULL.Localize() }, int)))", "M").WithLocation(13, 64), |
There was a problem hiding this comment.
Diagnostic(ErrorCode.ERR_NoSuchMember, "M").WithArguments($"(string, (int, ({MessageID.IDS_NULL.Localize() }, int)))", "M").WithLocation(13, 64), [](start = 16, length = 145)
Same comment
| // (15,49): error CS0117: '(string, lambda expression)' does not contain a definition for 'GetAwaiter' | ||
| // System.Console.WriteLine(("qq", ()=>1 ).GetAwaiter()); | ||
| Diagnostic(ErrorCode.ERR_NoSuchMember, "GetAwaiter").WithArguments("(string, lambda expression)", "GetAwaiter").WithLocation(15, 49) | ||
| Diagnostic(ErrorCode.ERR_NoSuchMember, "GetAwaiter").WithArguments($"(string, {MessageID.IDS_Lambda.Localize()})", "GetAwaiter").WithLocation(15, 49) |
There was a problem hiding this comment.
Diagnostic(ErrorCode.ERR_NoSuchMember, "GetAwaiter").WithArguments($"(string, {MessageID.IDS_Lambda.Localize()})", "GetAwaiter").WithLocation(15, 49) [](start = 16, length = 149)
Same comment
| // (19,44): error CS0117: '(int, <null>)' does not contain a definition for 'M1' | ||
| // System.Console.WriteLine((1, null).M1()); | ||
| Diagnostic(ErrorCode.ERR_NoSuchMember, "M1").WithArguments("(int, <null>)", "M1"), | ||
| Diagnostic(ErrorCode.ERR_NoSuchMember, "M1").WithArguments($"(int, {MessageID.IDS_NULL.Localize()})", "M1"), |
There was a problem hiding this comment.
Diagnostic(ErrorCode.ERR_NoSuchMember, "M1").WithArguments($"(int, {MessageID.IDS_NULL.Localize()})", "M1"), [](start = 16, length = 108)
Same comment
|
|
||
| var result = compilation.Emit(new MemoryStream(), options: new EmitOptions(outputNameOverride: "x\0x")); | ||
| result.Diagnostics.Verify( | ||
| result.Diagnostics.Verify(fallbackToErrorCodeOnlyForNonEnglish: true, |
There was a problem hiding this comment.
Verify [](start = 31, length = 6)
VerifyWithFallbackToErrorCodeOnlyForNonEnglish? Could you please explain what happens with this test in its original form?
| // a.csx(2,27): error CS1504: Source file 'b.csx' could not be opened -- Could not find file. | ||
| // #load "b.csx"; | ||
| Diagnostic(ErrorCode.ERR_NoSourceFile, @"""b.csx""").WithArguments("b.csx", "Could not find file.").WithLocation(2, 27)); | ||
| Diagnostic(ErrorCode.ERR_NoSourceFile, @"""b.csx""").WithArguments("b.csx", CSharpResources.CouldNotFindFile).WithLocation(2, 27)); |
There was a problem hiding this comment.
CSharpResources.CouldNotFindFile [](start = 92, length = 32)
Is this string coming from exception? Please confirm that and, if that is the case, wrap the body of the test with using (new EnsureEnglishUICulture())
There was a problem hiding this comment.
If the string is not coming from exception we need to take a closer look at what is going on here.
In reply to: 168914982 [](ancestors = 168914982)
| <name>Test</name> | ||
| </assembly> | ||
| <members> | ||
| <!-- Badly formed XML comment ignored for member ""T:C1"" --> |
There was a problem hiding this comment.
[](start = 8, length = 61)
Please change UI culture for the duration of this function instead. using (new EnsureEnglishUICulture())
| <!-- Badly formed XML comment ignored for member ""T:Partial"" --> | ||
| <!-- Badly formed XML comment ignored for member ""T:Parse"" --> | ||
| <!-- Badly formed XML comment ignored for member ""T:Diagnose"" --> | ||
| {string.Format(CSharpResources.IDS_XMLIGNORED, "T:Partial")} |
There was a problem hiding this comment.
{string.Format(CSharpResources.IDS_XMLIGNORED, "T:Partial")} [](start = 8, length = 60)
Please change UI culture for the duration of this function instead. using (new EnsureEnglishUICulture())
| var main = compilation.GetTypeByMetadataName("Test").GetMember<MethodSymbol>("Main"); | ||
|
|
||
| Assert.Equal(@"<!-- Badly formed XML comment ignored for member ""M:Test.Main"" -->", main.GetDocumentationCommentXml().Trim()); | ||
| Assert.Equal(string.Format(CSharpResources.IDS_XMLIGNORED, "M:Test.Main"), main.GetDocumentationCommentXml().Trim()); |
There was a problem hiding this comment.
Assert.Equal(string.Format(CSharpResources.IDS_XMLIGNORED, "M:Test.Main"), main.GetDocumentationCommentXml().Trim()); [](start = 12, length = 117)
Please change UI culture for the duration of this function instead. using (new EnsureEnglishUICulture())
| var result = RunCommandLineCompiler(CSharpCompilerClientExecutable, $"/shared:{serverData.PipeName} /nologo hello.cs", _tempDirectory, files, redirectEncoding: Encoding.ASCII, shouldRunOnServer: false); | ||
| Assert.Equal(result.ExitCode, 1); | ||
| Assert.Equal("hello.cs(1,1): error CS1056: Unexpected character '?'", result.Output.Trim()); | ||
| Assert.Equal($"hello.cs(1,1): error CS1056: {string.Format(CSharpResources.ERR_UnexpectedCharacter, "?")}", result.Output.Trim()); |
There was a problem hiding this comment.
Assert.Equal($"hello.cs(1,1): error CS1056: {string.Format(CSharpResources.ERR_UnexpectedCharacter, "?")}", result.Output.Trim()); [](start = 16, length = 130)
Please add /preferreduilang:en switch instead.
There was a problem hiding this comment.
It looks like this is applicable to all other issues in this file
In reply to: 168915207 [](ancestors = 168915207)
| Assert.Equal( | ||
| $"{vb}(3) : error BC30451: 'Bad' is not declared. It may be inaccessible due to its protection level. | ||
| $"{vb}(3) : error BC30451: {String.Format(VBResources.ERR_NameNotDeclared1, "Bad")} | ||
|
|
There was a problem hiding this comment.
Please add /preferreduilang:en switch instead
|
Done with review pass (iteration 5) |
|
The PR wasn't updated in a while. Marking as "not for review" for now so it doesn't show up in our queries. |
|
Closing as these issues were all cleaned up in #25874. Thanks for submitting this PR. It was helpful in convincing us this was a real problem that we needed to fix and add testing to ensure it stayed fix going forward. |
Here are changes to 5 different test projects that were failing on my german machine because of localization issues.
I had to add an Internals visible to to make resources visible to the compiler server tests.
#23837