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

[coreclr] Block test that should not be run #97248

Merged
merged 2 commits into from
Jan 20, 2024

Conversation

ilonatommy
Copy link
Member

Fixes #97241.
This test was not supposed to be run on coreclr.

@ilonatommy ilonatommy added area-System.Globalization blocking-clean-ci Blocking PR or rolling runs of 'runtime' or 'runtime-extra-platforms' labels Jan 20, 2024
@ilonatommy ilonatommy self-assigned this Jan 20, 2024
@ghost
Copy link

ghost commented Jan 20, 2024

Tagging subscribers to this area: @dotnet/area-system-globalization
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #97241.
This test was not supposed to be run on coreclr.

Author: ilonatommy
Assignees: ilonatommy
Labels:

area-System.Globalization, blocking-clean-ci

Milestone: -

@stephentoub
Copy link
Member

This test was not supposed to be run on coreclr.

Why does "IsIcuGlobalization" rule out coreclr?

@pavelsavara pavelsavara merged commit 22149fa into dotnet:main Jan 20, 2024
111 checks passed
@pavelsavara pavelsavara added the arch-wasm WebAssembly architecture label Jan 20, 2024
@ilonatommy
Copy link
Member Author

ilonatommy commented Jan 20, 2024

This test was not supposed to be run on coreclr.

Why does "IsIcuGlobalization" rule out coreclr?

public static bool IsIcuGlobalization => !IsInvariantGlobalization && (IsHybridGlobalizationOnApplePlatform || ICUVersion > new Version(0, 0, 0, 0));

It seems that coreclr does not load ICU, other tests marked by this property were skipped. This very tests assumes that the platform uses ICU4C hashing algorithm and depends on how ICU behaves (ignores specific characters in hashing). I am not sure what coreclr uses instead of ICU, probably NLS.

tmds pushed a commit to tmds/runtime that referenced this pull request Jan 23, 2024
@stephentoub
Copy link
Member

It seems that coreclr does not load ICU

It does. We use ICU by default on coreclr; you have to opt-out of it to use NLS instead. And I just tried and IsIcuGlobalization does behave as I'd expect and return true when running with coreclr.

If the goal of using IsIcuGlobalization was to avoid running on coreclr, I don't believe this is doing that.

@tarekgh
Copy link
Member

tarekgh commented Jan 24, 2024

At some point @jkotas fixed a bug in tests which used to check ICU version before loading it. #96288.

@jkotas
Copy link
Member

jkotas commented Jan 24, 2024

The test was failing on Win7 and early Win10 that does not have ICU. This change will correctly disable the test on these Windows version.

The test change is correct, just the commit description is not quite right.

@stephentoub
Copy link
Member

The test change is correct, just the commit description is not quite right.

👍

@github-actions github-actions bot locked and limited conversation to collaborators Feb 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-System.Globalization blocking-clean-ci Blocking PR or rolling runs of 'runtime' or 'runtime-extra-platforms'
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test failing: System.Globalization.Tests.CompareInfoHashCodeTests.CheckHashingOfSkippedChars
6 participants