ICU-22781 Uncomment and enable constant denominator tests #3385

younies · 2025-02-10T21:59:15Z

Description:

Re-enable previously commented out tests for constant denominators in units tests for both ICU4C and ICU4J. This includes tests for:

Large power of 10 denominators
Scientific notation denominators
Specific unit denominators

Added notes about potential CLDR-related edge cases for certain unit representations.

Checklist

Required: Issue filed: ICU-22781
Required: The PR title must be prefixed with a JIRA Issue number. Example: "ICU-1234 Fix xyz"
Required: Each commit message must be prefixed with a JIRA Issue number. Example: "ICU-1234 Fix xyz"
Issue accepted (done by Technical Committee after discussion)
Tests included, if applicable
API docs and/or User Guide docs changed or added, if applicable

richgillam

Where's the part where you actually test that formatting does he right thing with this unit?

richgillam · 2025-02-11T00:35:45Z

icu4c/source/test/intltest/units_test.cpp

+        // `portion-per-1e9` is treated as a unit in CLDR,
+        // the unit does not have a constant denominator.
+        // This issue should be addressed in CLDR & ICU.
+        {"portion-per-1000000000", 0},


Is there an outstanding ticket for this? If so, you should probably include the ticket number (or a link) here.

portion-per-1e9 is treated as a precomputed form for translation, not a single unit.

Some units already have 'precomputed' forms, such as kilometer-per-hour; where such units exist, they should be used in preference.

We shouldn't test for the wrong behavior.
That being said, given the schedule, and the fact that we are going fix the problem fully in 48, it's ok for {"portion-per-1000000000", 1000000000}, be present but commented out.

Sure, I have added it back with logKnownIssue so it can be addressed in the next release.

macchiati · 2025-02-11T04:08:52Z

icu4c/source/test/intltest/units_test.cpp

+        // `portion-per-1e9` is treated as a unit in CLDR,
+        // the unit does not have a constant denominator.
+        // This issue should be addressed in CLDR & ICU.
+        {"portion-per-1000000000", 0},


portion-per-1e9 is treated as a precomputed form for translation, not a single unit.

Some units already have 'precomputed' forms, such as kilometer-per-hour; where such units exist, they should be used in preference.

We shouldn't test for the wrong behavior.
That being said, given the schedule, and the fact that we are going fix the problem fully in 48, it's ok for {"portion-per-1000000000", 1000000000}, be present but commented out.

macchiati · 2025-02-11T04:10:57Z

icu4j/main/common_tests/src/test/java/com/ibm/icu/dev/test/format/MeasureUnitTest.java

+                // This issue should be addressed in CLDR & ICU.
+                new ConstantDenominatorTestCase("portion-per-1000000000", 0),
+                new ConstantDenominatorTestCase("portion-per-1e9", 0),
+                new ConstantDenominatorTestCase("portion-per-1E9", 0),


done, I added also a logKnownIssue

mihnita · 2025-02-11T11:06:24Z

icu4c/source/test/intltest/units_test.cpp

@@ -1205,6 +1205,7 @@ void UnitsTest::testUnitsConstantsDenomenator() {
        {"portion-per-7", 7},
        {"portion-per-8", 8},
        {"portion-per-9", 9},


These cases don't exist in the Java tests.
Is there a good reason to have them?
Are they testing something interesting, that is not well covered by other cases?
They they should also be in Java.

If they don't actually exercise anything interesting, we should not have them in C/C++ either.

In general the tests in C++ and Java should be the same, unless there are very good reasons (for example Java testing for all kind of types that don't exist in C++)

now , c++ and java are identical, sorry for this small confusion.

younies · 2025-02-11T14:32:23Z

Where's the part where you actually test that formatting does he right thing with this unit?

@richgillam : for testing the formatting:
#3389
#3390

See unicode-org#3385

jira-pull-request-webhook · 2025-02-11T18:52:08Z

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

richgillam · 2025-02-11T21:49:48Z

Where's the part where you actually test that formatting does he right thing with this unit?

@richgillam : for testing the formatting: #3389 #3390

Thank you.

richgillam

Looks okay to me, but I think you should get approval from somebody who raised objections the last time around.

richgillam · 2025-02-11T21:51:57Z

icu4c/source/test/intltest/units_test.cpp

+        {"portion-per-1000000000000000", 1000000000000000},
+        {"portion-per-10000000000000000", 10000000000000000},
+        {"portion-per-100000000000000000", 100000000000000000},
+        {"portion-per-1000000000000000000", 1000000000000000000},


Same issue as in the other PRs: If these are all expected to fail and are being covered by logKnownIssue() in the code, put some sort of comment next to the actual test case to indicate that (the ticket number you had in the earlier version of these changes works, for example). Same thing further down in here.

An alternative approach would be to add a knownIssueTicketNumber property to your TestCase object-- the test code would look for that and, if it's populated, skip the test and pass it to logKnownIssue.

done now :)

Is line 1219 the only one that's failing? If so, we're good here.

Yes, it is the only one that is failing.

See unicode-org#3385

jira-pull-request-webhook · 2025-02-12T10:08:56Z

Notice: the branch changed across the force-push!

icu4c/source/test/intltest/measfmttest.cpp is different
icu4c/source/test/intltest/units_test.cpp is different
icu4j/main/common_tests/src/test/java/com/ibm/icu/dev/test/format/MeasureUnitTest.java is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

younies · 2025-02-14T00:44:04Z

@macchiati , can you take a look ?

younies requested review from mihnita, sffc and richgillam February 10, 2025 21:59

richgillam reviewed Feb 11, 2025

View reviewed changes

macchiati requested changes Feb 11, 2025

View reviewed changes

mihnita reviewed Feb 11, 2025

View reviewed changes

younies requested review from macchiati, mihnita and richgillam February 11, 2025 14:32

younies added a commit to younies/icu that referenced this pull request Feb 11, 2025

ICU-22781 Uncomment and enable constant denominator tests

573f7e8

See unicode-org#3385

younies force-pushed the enable-constant-tests branch from c18f728 to 573f7e8 Compare February 11, 2025 18:52

richgillam reviewed Feb 11, 2025

View reviewed changes

ICU-22781 Uncomment and enable constant denominator tests

771dad2

See unicode-org#3385

younies force-pushed the enable-constant-tests branch from 573f7e8 to 771dad2 Compare February 12, 2025 10:08

macchiati approved these changes Feb 14, 2025

View reviewed changes

younies merged commit 3c85be1 into unicode-org:main Feb 14, 2025
101 checks passed

younies deleted the enable-constant-tests branch February 14, 2025 09:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ICU-22781 Uncomment and enable constant denominator tests #3385

ICU-22781 Uncomment and enable constant denominator tests #3385

younies commented Feb 10, 2025 •

edited

Loading

richgillam left a comment

richgillam Feb 11, 2025

macchiati Feb 11, 2025

younies Feb 11, 2025

macchiati Feb 11, 2025

macchiati Feb 11, 2025

younies Feb 11, 2025

mihnita Feb 11, 2025

younies Feb 11, 2025

younies commented Feb 11, 2025

jira-pull-request-webhook bot commented Feb 11, 2025

richgillam commented Feb 11, 2025

richgillam left a comment

richgillam Feb 11, 2025

younies Feb 12, 2025

richgillam Feb 12, 2025

younies Feb 12, 2025

jira-pull-request-webhook bot commented Feb 12, 2025

younies commented Feb 14, 2025

ICU-22781 Uncomment and enable constant denominator tests #3385

ICU-22781 Uncomment and enable constant denominator tests #3385

Conversation

younies commented Feb 10, 2025 • edited Loading

Description:

Checklist

richgillam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

younies commented Feb 11, 2025

jira-pull-request-webhook bot commented Feb 11, 2025

richgillam commented Feb 11, 2025

richgillam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jira-pull-request-webhook bot commented Feb 12, 2025

younies commented Feb 14, 2025

younies commented Feb 10, 2025 •

edited

Loading