Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OrganizeImportsTests involving non-English characters are failing locally. #58834

Closed
Youssef1313 opened this issue Jan 13, 2022 · 7 comments
Closed
Assignees
Labels
Area-IDE Test Test failures in roslyn-CI
Milestone

Comments

@Youssef1313
Copy link
Member

Youssef1313 commented Jan 13, 2022

Version Used:

main

Steps to Reproduce:

Run Build.cmd -testCoreClr locally.

Actual Behavior:

image

Expected Behavior:

No test failure. The code path for comparing appears to be using InvariantCulture, which per documentation:

invariant culture data is stable over time and across installed cultures and cannot be customized by users. This makes the invariant culture particularly useful for operations that require culture-independent results, such as formatting and parsing operations that persist formatted data, or sorting and ordering operations that require that data be displayed in a fixed order regardless of culture.

StackOverflow question around this area for reference.

The comparison, per my understanding, happens in:

https://github.com/dotnet/roslyn/blob/main/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Utilities/TokenComparer.cs

https://github.com/dotnet/roslyn/blob/main/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/VisualBasic/Utilities/TokenComparer.vb

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jan 13, 2022
@jinujoseph jinujoseph added Test Test failures in roslyn-CI and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Jan 20, 2022
@jinujoseph jinujoseph added this to the 17.2.P1 milestone Jan 20, 2022
@Cosifne Cosifne modified the milestones: 17.2.P1, 17.2 Feb 15, 2022
@Cosifne
Copy link
Member

Cosifne commented Feb 15, 2022

I play with this test and the key problem is the comparing of three Japanese characters.
The test is assuming ア < ア < あ
I don't know Japanese but from my testing, the current comparison result is
Enable Kana + Ignore Width : あ = ア < ア

And I can't find a combination to let あ be the largest.
If enable Kana then あ <ア, diable Kana then あ = ア
If with Width then あ < ア, ignore width then あ = ア

@CyrusNajmabadi
Copy link
Member

@dtivel I believe you wrote these test cases like 15 years ago right :) Any recollection on the concerns here?

@dtivel
Copy link

dtivel commented Feb 16, 2022

I don't remember anything about this test. What does test history look like? Has the test been flaky for the past 15 years or did it suddenly start failing now?

@CyrusNajmabadi
Copy link
Member

I don't remember anything about this test. What does test history look like?

This predates roslyn by like 10+ years :D it was back when i added "sort imports" back in the wee VS2005 days and we worked on it together :)

@Cosifne
Copy link
Member

Cosifne commented Feb 16, 2022

I don't remember anything about this test. What does test history look like? Has the test been flaky for the past 15 years or did it suddenly start failing now?

It's been reported to fail since Jan. But I am not sure it is starting to fail since which commit.
I feel it would be good to review the test to see if it still makes sense after 10 years.
If yes, we could talk to some cultural experts to see how to achieve it

@RikkiGibson
Copy link
Contributor

It's possible that #57365 introduced the failure during local runs. It is probably related to a different runtime being used when running coreclr tests in CI versus locally.

@jinujoseph jinujoseph modified the milestones: 17.2, 17.3 May 5, 2022
@Cosifne
Copy link
Member

Cosifne commented Sep 12, 2022

Fixed in this PR:
#63853

@Cosifne Cosifne closed this as completed Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE Test Test failures in roslyn-CI
Projects
None yet
Development

No branches or pull requests

7 participants