Skip to content

Fixing compiler test localization issues#25772

Merged
jaredpar merged 11 commits intodotnet:features/spanish-queuefrom
jaredpar:fix-loc
Mar 28, 2018
Merged

Fixing compiler test localization issues#25772
jaredpar merged 11 commits intodotnet:features/spanish-queuefrom
jaredpar:fix-loc

Conversation

@jaredpar
Copy link
Copy Markdown
Member

@jaredpar jaredpar commented Mar 27, 2018

This fixes the localization issues we had in our tests as revealed by our Spanish run.

@jaredpar jaredpar added Area-Compilers PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Mar 27, 2018
@jaredpar jaredpar requested a review from a team as a code owner March 27, 2018 21:10
@jaredpar jaredpar changed the title WIP: Fixing compiler test localization issues Fixing compiler test localization issues Mar 27, 2018
@jaredpar
Copy link
Copy Markdown
Member Author

CC @dotnet/roslyn-compiler for review

@AlekseyTs
Copy link
Copy Markdown
Contributor

@jaredpar The PR is still marked for personal review

</errors>
CompilationUtils.AssertTheseDeclarationDiagnostics(compilation1, expectedErrors1)

Using New EnsureEnglishUICulture
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Mar 27, 2018

Choose a reason for hiding this comment

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

Using New EnsureEnglishUICulture [](start = 8, length = 32)

The fact that you have to do this looks suspicious. I would expect AssertTheseDeclarationDiagnostics to take care of this #Closed

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The latter part of the message is controlled by the resource string CodeAnalysisResources.PEImageDoesntContainManagedMetadata. It's not added as a diagnostic argument but as the text of an exception. From PEModule.cs

throw new BadImageFormatException(CodeAnalysisResources.PEImageDoesntContainManagedMetadata);

Given it's an exception message I wouldn't expect the AssertTheseDeclarationDiagnostics method to be able to control it. Am I missing something?


In reply to: 177594598 [](ancestors = 177594598)

Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Mar 28, 2018

Choose a reason for hiding this comment

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

Scratch that, this is an thrown exception case


In reply to: 177594598 [](ancestors = 177594598)

@jaredpar jaredpar removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Mar 27, 2018
bool shouldRunOnServer = true)
{
var arguments = new List<string>(argumentsSingle.Split(' '));
arguments.Add("/preferreduilang:en");
Copy link
Copy Markdown
Member

@jasonmalinowski jasonmalinowski Mar 27, 2018

Choose a reason for hiding this comment

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

It might be worth adding a comment here to the test that is validating that localization does work, just so somebody isn't thinking we never test that. #Resolved

@jaredpar jaredpar added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Mar 27, 2018
Copy link
Copy Markdown
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 (iteration 10)

''' Late-bound calls with ByRef return values not supported.
''' </summary>
<Fact()>
<ConditionalFact(GetType(IsEnglishLocal))>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

<ConditionalFact(GetType(IsEnglishLocal))> [](start = 8, length = 42)

Instead of doing this, we can change/restore CurrentCulture in the Main method of comp2. We should have examples of that.

Copy link
Copy Markdown
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 (iteration 11)

@jaredpar jaredpar removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Mar 28, 2018
@jaredpar
Copy link
Copy Markdown
Member Author

Flagged compiler test are now passing.

@jaredpar jaredpar merged commit ecec9d4 into dotnet:features/spanish-queue Mar 28, 2018
@jaredpar jaredpar deleted the fix-loc branch March 28, 2018 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants