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

[lib][tests] Globalization comparison tests cleanup #86305

Closed

Conversation

ilonatommy
Copy link
Member

@ilonatommy ilonatommy commented May 16, 2023

Initiated by #85965 (comment).
In the original globalization comparison tests we used to have test cases with multiple comparison options. Actually never all 3 declared options were relevant which obscured the test readibility. This PR reduces the number of options passed to minimum.

@ghost
Copy link

ghost commented May 16, 2023

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

Issue Details

Initiated by #85965 (comment).
In the original globalization comparison tests we used to have comparison test cases with multiple options. Actually never all 3 declared options were relevant which obscured the test readibility. This PR reduces the number of options passed to minimum.

Author: ilonatommy
Assignees: ilonatommy
Labels:

area-System.Globalization

Milestone: -

@ilonatommy ilonatommy changed the title Cleanup. [lib][tests] Globalization comparison tests cleanup May 16, 2023
@tarekgh tarekgh added the arch-wasm WebAssembly architecture label May 16, 2023
@ghost
Copy link

ghost commented May 16, 2023

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

Initiated by #85965 (comment).
In the original globalization comparison tests we used to have test cases with multiple comparison options. Actually never all 3 declared options were relevant which obscured the test readibility. This PR reduces the number of options passed to minimum.

Author: ilonatommy
Assignees: ilonatommy
Labels:

arch-wasm, area-System.Globalization

Milestone: -

@mkhamoyan
Copy link
Contributor

/azp run runtime-ioslike

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mkhamoyan
Copy link
Contributor

Looks good to me. Let's wait for tests to run on iOS.

@ilonatommy
Copy link
Member Author

ios failures are unrelated

@mkhamoyan
Copy link
Contributor

@ilonatommy there is also supportedCmpOptions which looks like same property. As in my current PR #85965 I am touching a lot of lines after this PR got merged there will be need to check a lot of lines one by one. So my suggestion is either I can do these refactoring in my current PR, either let's do this after my PR got merged?

@ilonatommy
Copy link
Member Author

So my suggestion is either I can do these refactoring in my current PR, either let's do this after my PR got merged?

If it won't be problematic then doing it in your PR is fine.

@mkhamoyan
Copy link
Contributor

Thanks, I will do then.

mkhamoyan added a commit that referenced this pull request May 17, 2023
Copy link
Member

@mdh1418 mdh1418 left a comment

Choose a reason for hiding this comment

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

Thanks for going in and finding the minimum options!

@ilonatommy
Copy link
Member Author

This will get in with #85965. We checked that CI is passing with the changes but decided that it's less confusing for Meri to incorporate the changes directly in her PR.

@ilonatommy ilonatommy closed this May 17, 2023
mkhamoyan added a commit that referenced this pull request May 24, 2023
Implemented CompareStringNative for OSX platforms
Added changes done by @ilonatommy in #86305
@ghost ghost locked as resolved and limited conversation to collaborators Jun 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants