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

[release/5.0-rc2] Fix the globalization test. #42226

Merged
merged 2 commits into from
Sep 15, 2020

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Sep 14, 2020

Backport of #42212 to release/5.0-rc2

/cc @tarekgh

Customer Impact

This is a test only change. No product change. So, there is no real customer impact more than developers who build and run our tests against the release branch.
The change here is avoiding the test failure caused by a Windows regression in the new Windows insider builds.

Testing

Running the failed test successfully with no failures.

Risk

None as this is a test change.

The failure is because Windows regresion. The change here is to avoid having the test fail because of that. The regression is tracked by Windows team to fix it.
@ghost
Copy link

ghost commented Sep 14, 2020

Tagging subscribers to this area: @tarekgh, @safern, @krwq
See info in area-owners.md if you want to be subscribed.

@tarekgh tarekgh requested a review from safern September 14, 2020 22:49
@danmoseley
Copy link
Member

OK to merge this test-only stability change as "tell mode"

private static extern int CompareStringEx(string lpLocaleName, uint dwCmpFlags, string lpString1, int cchCount1, string lpString2, int cchCount2, IntPtr lpVersionInformation, IntPtr lpReserved, int lParam);
private const int NORM_LINGUISTIC_CASING = 0x08000000; // use linguistic rules for casing

private static bool WindowsVersionHasTheCompareStringRegression =>
Copy link
Member

Choose a reason for hiding this comment

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

What does "the compare string regression" mean? Will we understand which regression it's referring to in a month or a year?

Copy link
Member

Choose a reason for hiding this comment

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

The code is clear I guess. comparing 2 specific strings returning wrong results. I expect we'll remove this code when Windows fix the regression too. I added this code as I don't know specific versions will have the regression and when it is going to be fixed.

Copy link
Member

Choose a reason for hiding this comment

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

@stephentoub please let me know if you have any concern before I merge this one.

Copy link
Member

Choose a reason for hiding this comment

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

Might be worth a comment, but otherwise, ok.

@tarekgh tarekgh merged commit e24e67a into release/5.0-rc2 Sep 15, 2020
@tarekgh tarekgh deleted the backport/pr-42212-to-release/5.0-rc2 branch September 15, 2020 22:29
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants