Skip to content

React to string comparison changing on .NET Core#56011

Merged
jaredpar merged 1 commit intodotnet:mainfrom
jaredpar:strings
Aug 30, 2021
Merged

React to string comparison changing on .NET Core#56011
jaredpar merged 1 commit intodotnet:mainfrom
jaredpar:strings

Conversation

@jaredpar
Copy link
Copy Markdown
Member

@jaredpar jaredpar commented Aug 30, 2021 •
edited
Loading

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
React to string comparison changing on .NET Core
2aaf0eb 
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.

dotnet/runtime#43956
@ghost ghost added the Area-Compilers label Aug 30, 2021
@jaredpar jaredpar marked this pull request as ready for review August 30, 2021 18:42
@jaredpar jaredpar requested a review from a team as a code owner August 30, 2021 18:42
@jaredpar
Copy link
Copy Markdown
Member Author

jaredpar commented Aug 30, 2021

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

jcouv
jcouv approved these changes Aug 30, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 1)

@jcouv jcouv added the Test Test failures in roslyn-CI label Aug 30, 2021
@RikkiGibson
Copy link
Copy Markdown
Member

RikkiGibson commented Aug 30, 2021

I have a feeling this relates to #46608

@jaredpar
Copy link
Copy Markdown
Member Author

jaredpar commented Aug 30, 2021

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.

@jaredpar jaredpar merged commit 4d99cda into dotnet:main Aug 30, 2021
@ghost ghost added this to the Next milestone Aug 30, 2021
@jaredpar jaredpar deleted the strings branch August 30, 2021 20:20
@dibarbet dibarbet modified the milestones: Next, 17.0.P4 Aug 31, 2021
@RikkiGibson RikkiGibson mentioned this pull request Oct 11, 2021
Closed
@RikkiGibson
Copy link
Copy Markdown
Member

RikkiGibson commented Feb 25, 2022

@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
Copy link
Copy Markdown
Member

RikkiGibson commented Feb 25, 2022

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
Copy link
Copy Markdown
Contributor

AlekseyTs commented Feb 25, 2022 •
edited
Loading

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
Copy link
Copy Markdown
Contributor

AlekseyTs commented Feb 25, 2022

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

@jaredpar
Copy link
Copy Markdown
Member Author

jaredpar commented Feb 25, 2022

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
Copy link
Copy Markdown
Member

RikkiGibson commented Feb 25, 2022

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RikkiGibson RikkiGibson RikkiGibson approved these changes

@jcouv jcouv jcouv approved these changes

Assignees

No one assigned

Labels

Area-Compilers Test Test failures in roslyn-CI

Projects

None yet

Milestone

17.0.P4

Development

Successfully merging this pull request may close these issues.

5 participants

@jaredpar @RikkiGibson @AlekseyTs @jcouv @dibarbet