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

ICU-22152 Uncommented a bunch of commented-out test cases in ULocaleC… #2883

Merged

Conversation

richgillam
Copy link
Contributor

@richgillam richgillam commented Mar 13, 2024

…ollationTest.TestNameList() and made them pass again.

Checklist
  • Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22152
  • Required: The PR title must be prefixed with a JIRA Issue number.
  • Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
  • Required: Each commit message must be prefixed with a JIRA Issue number.
  • Issue accepted (done by Technical Committee after discussion)
  • Tests included, if applicable
  • API docs and/or User Guide docs changed or added, if applicable

…ollationTest.TestNameList() and made them pass again.
// {"Français (cyrillique, Canada)", "Français (cyrillique, Canada)", "fr_Cyrl_CA", "fr_Cyrl_CA"},
// },
{{"fr-Cyrl-BE", "fr-Cyrl-CA"},
{"Français (cyrillique, Belgique)", "French (Cyrillic, Belgium)", "fr_Cyrl_BE", "fr_Cyrl_BE"},
Copy link
Member

Choose a reason for hiding this comment

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

optional:

This hardcodes the English result, and I see that you are changing the test code to temporarily hardcode the default language to be English. This should work.

I wonder if it's worth it. A simpler approach would be to change the expected values that fall back to the default locale to null or "" and have the test code below simply ignore them.

On the other hand, you have done this and you could simply merge it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hardcoding the English results and forcing the default locale to be English seemed like the path of least resistance, so I took it. TBH, I didn't think about using an empty string (or a null) as an indicator to skip the test altogether. I kind of like that, but then you're not testing the "in self" case at all, whereas forcing the default locale to be English does test that logic. On the other hand, getting English in these cases feels really wrong...

I'm inclined to just merge this and call it a day until some other problem comes up or somebody suggests a better change to the underlying code. It sounds like you don't object to that, but I'll leave this PR open until at least the end of the day just in case you change your mind or somebody else weighs in.

Copy link
Member

Choose a reason for hiding this comment

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

I'm inclined to just merge this and call it a day

+1

@richgillam richgillam merged commit 281d8ef into unicode-org:main Mar 19, 2024
15 checks passed
@richgillam richgillam deleted the ICU-22152-collation-displayname-tests branch March 19, 2024 00:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants