React to string comparison changing on .NET Core

[jaredpar]

The default sort order for char / string changed on .NET Core. This
impacted a number of our tests which weren't explicitly using ordinal to
compare strings. The fix here makes our tests run consistently across the
various versions of .NET Core and .NET Framework

dotnet/runtime#43956

[jaredpar]

@dotnet/roslyn-compiler PTAL this updates our code to account for at least part of the string comparison changes in net5.0

[jcouv]

LGTM Thanks (iteration 1)

[RikkiGibson]

I have a feeling this relates to #46608

[jaredpar]

I have a feeling this relates to #46608

Agree. There is a very strong chance that those failures are at least partially related to this.

[RikkiGibson]

@jaredpar @sharwell This happened again because we started using a non-ordinal string comparison here:

roslyn/src/Workspaces/CSharpTest/OrganizeImports/OrganizeUsingsTests.cs

Line 40 in 3edf5a4

Assert.Equal(final.ReplaceLineEndings(endOfLine ?? Environment.NewLine), newRoot.ToFullString());

Is there something we can do to prevent this from cropping up in the future? It seems like it's caused by a difference between the runtimes used to run tests in CI vs locally?

[RikkiGibson]

I currently get the following test failures when running -testCoreClr locally:

✘ Microsoft.CodeAnalysis.CSharp.Workspaces.UnitTests.OrganizeImports.OrganizeUsingsTests.CaseSensitivity29ms
Error:
Assert.Equal() Failure
                ↓ (pos 6)
Expected: using ア;\r\nusing ｱ;\r\nusing あ;\r\nusing アア;\r\nusing ···
Actual:   using あ;\r\nusing ｱ;\r\nusing ああ;\r\nusing あｱ;\r\nusing···
                ↑ (pos 6)

Stack trace:
   at Microsoft.CodeAnalysis.CSharp.Workspaces.UnitTests.OrganizeImports.OrganizeUsingsTests.CheckAsync(String initial, String final, Boolean placeSystemNamespaceFirst, Boolean separateImportGroups, String endOfLine) in C:\Users\rikki\src\roslyn\src\Workspaces\CSharpTest\OrganizeImports\OrganizeUsingsTests.cs:line 40
   at Microsoft.CodeAnalysis.CSharp.Workspaces.UnitTests.OrganizeImports.OrganizeUsingsTests.CaseSensitivity2() in C:\Users\rikki\src\roslyn\src\Workspaces\CSharpTest\OrganizeImports\OrganizeUsingsTests.cs:line 1122
--- End of stack trace from previous location ---

✘ Microsoft.CodeAnalysis.CSharp.Workspaces.UnitTests.OrganizeImports.OrganizeUsingsTests.CaseSensitivity18ms
Error:
Assert.Equal() Failure
                                     ↓ (pos 305)
Expected: ···;\r\nusing CC;\r\nusing ア;\r\nusing ｱ;\r\nusing あ;\r\nusing アア;\r\nusing ···
Actual:   ···;\r\nusing CC;\r\nusing あ;\r\nusing ｱ;\r\nusing ああ;\r\nusing あｱ;\r\nusing···
                                     ↑ (pos 305)

Stack trace:
   at Microsoft.CodeAnalysis.CSharp.Workspaces.UnitTests.OrganizeImports.OrganizeUsingsTests.CheckAsync(String initial, String final, Boolean placeSystemNamespaceFirst, Boolean separateImportGroups, String endOfLine) in C:\Users\rikki\src\roslyn\src\Workspaces\CSharpTest\OrganizeImports\OrganizeUsingsTests.cs:line 40
   at Microsoft.CodeAnalysis.CSharp.Workspaces.UnitTests.OrganizeImports.OrganizeUsingsTests.CaseSensitivity1() in C:\Users\rikki\src\roslyn\src\Workspaces\CSharpTest\OrganizeImports\OrganizeUsingsTests.cs:line 1087
--- End of stack trace from previous location ---

Actually, passing StringComparer.Ordinal to the mentioned Assert.Equal call above doesn't seem to change the results, so I'm not exactly certain what's going wrong here.

[AlekseyTs]

It looks like there is a special comment in the failing tests about a case mapping between the characters that cause the diff. Could it be caused by changes to case mapping rather than to changes to string comparison? Perhaps even unintentional changes.

[AlekseyTs]

BTW, it looks like we already have an issue that tracks the test failure - #58834.

[jaredpar]

Is there something we can do to prevent this from cropping up in the future? It seems like it's caused by a difference between the runtimes used to run tests in CI vs locally?

Unfortunately no. The runtime took this breaking change and there is nothing for us to do but adapt. There are no analyzers to spot this, it's just find the failure and fix.

[RikkiGibson]

I'm curious what specifically causes us to use a different runtime in CI versus locally. We always use the same SDK to run tests, so why should there be a difference?