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.

Sign up for GitHub

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-22781 Uncomment and enable constant denominator tests #3385

Merged
merged 1 commit into from
Feb 14, 2025
+107 −67
Merged

ICU-22781 Uncomment and enable constant denominator tests #3385

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions icu4c/source/test/intltest/measfmttest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6116,9 +6116,15 @@ void MeasureFormatTest::TestInvalidIdentifiers() {
"meter-per-1000-1000",
"meter-per-1000-second-1000-kilometer",
"per-1000-and-per-1000",
"meter-per-100-100-kilometer", // Failing ICU-23045
};

for (const auto& input : inputs) {
if (uprv_strcmp(input, "meter-per-100-100-kilometer") == 0) {
logKnownIssue("ICU-23045", "Incorrect constant denominator for certain unit identifiers "
"leads to incorrect unit identifiers.");
continue;
}
status.setScope(input);
MeasureUnit::forIdentifier(input, status);
status.expectErrorAndReset(U_ILLEGAL_ARGUMENT_ERROR);
Expand Down
61 changes: 33 additions & 28 deletions icu4c/source/test/intltest/units_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1192,6 +1192,7 @@ void UnitsTest::testUnitsConstantsDenomenator() {
} testCases[]{
{"meter-per-1000", 1000},
{"liter-per-1000-kiloliter", 1000},
{"meter-per-100-kilometer", 100}, // Failing: ICU-23045
{"liter-per-kilometer", 0},
{"second-per-1000-minute", 1000},
{"gram-per-1000-kilogram", 1000},
Expand All @@ -1205,6 +1206,7 @@ void UnitsTest::testUnitsConstantsDenomenator() {
{"portion-per-7", 7},
{"portion-per-8", 8},
{"portion-per-9", 9},
Copy link
Contributor

Choose a reason for hiding this comment

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

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++)

Copy link
Member Author

Choose a reason for hiding this comment

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

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


// Test for constant denominators that are powers of 10
{"portion-per-10", 10},
{"portion-per-100", 100},
Expand All @@ -1214,16 +1216,18 @@ void UnitsTest::testUnitsConstantsDenomenator() {
{"portion-per-1000000", 1000000},
{"portion-per-10000000", 10000000},
{"portion-per-100000000", 100000000},
// ICU-22781: {"portion-per-1000000000", 1000000000},
// ICU-22781: {"portion-per-10000000000", 10000000000},
// ICU-22781: {"portion-per-100000000000", 100000000000},
// ICU-22781: {"portion-per-1000000000000", 1000000000000},
// ICU-22781: {"portion-per-10000000000000", 10000000000000},
// ICU-22781: {"portion-per-100000000000000", 100000000000000},
// ICU-22781: {"portion-per-1000000000000000", 1000000000000000},
// ICU-22781: {"portion-per-10000000000000000", 10000000000000000},
// ICU-22781: {"portion-per-100000000000000000", 100000000000000000},
// ICU-22781: {"portion-per-1000000000000000000", 1000000000000000000},
{"portion-per-1000000000", 1000000000}, // Failing: ICU-23045
{"portion-per-10000000000", 10000000000},
{"portion-per-100000000000", 100000000000},
{"portion-per-1000000000000", 1000000000000},
{"portion-per-10000000000000", 10000000000000},
{"portion-per-100000000000000", 100000000000000},
{"portion-per-1000000000000000", 1000000000000000},
{"portion-per-10000000000000000", 10000000000000000},
{"portion-per-100000000000000000", 100000000000000000},
{"portion-per-1000000000000000000", 1000000000000000000},
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

done now :)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is the only one that is failing.

{"portion-per-1e3-kilometer", 1000},

// Test for constant denominators that are represented as scientific notation
// numbers.
{"portion-per-1e1", 10},
Expand All @@ -1238,32 +1242,33 @@ void UnitsTest::testUnitsConstantsDenomenator() {
{"portion-per-1E5", 100000},
{"portion-per-1e6", 1000000},
{"portion-per-1E6", 1000000},
// ICU-22781: {"portion-per-1e10", 10000000000},
// ICU-22781: {"portion-per-1E10", 10000000000},
// ICU-22781: {"portion-per-1e18", 1000000000000000000},
// ICU-22781: {"portion-per-1E18", 1000000000000000000},
{"portion-per-1e9", 1000000000}, // Failing: ICU-23045
{"portion-per-1E9", 1000000000}, // Failing: ICU-23045
{"portion-per-1e10", 10000000000},
{"portion-per-1E10", 10000000000},
{"portion-per-1e18", 1000000000000000000},
{"portion-per-1E18", 1000000000000000000},

// Test for constant denominators that are randomly selected.
// ICU-22781: {"liter-per-12345-kilometer", 12345},
// ICU-22781: {"per-1000-kilometer", 1000},
// ICU-22781: {"liter-per-1000-kiloliter", 1000},
{"liter-per-12345-kilometer", 12345},
{"per-1000-kilometer", 1000},
{"liter-per-1000-kiloliter", 1000},

// Test for constant denominators that give 0.
{"meter", 0},
{"meter-per-second", 0},
{"meter-per-square-second", 0},
// NOTE: The following constant denominator should be 0. However, since
// `100-kilometer` is treated as a unit in CLDR,
// the unit does not have a constant denominator.
// This issue should be addressed in CLDR.
{"meter-per-100-kilometer", 0},
// NOTE: the following CLDR identifier should be invalid, but because
// `100-kilometer` is considered a unit in CLDR,
// one `100` will be considered as a unit constant denominator and the other
// `100` will be considered part of the unit.
// This issue should be addressed in CLDR.
{"meter-per-100-100-kilometer", 100},
};

for (const auto &testCase : testCases) {
if (uprv_strcmp(testCase.source, "portion-per-1000000000") == 0 ||
uprv_strcmp(testCase.source, "portion-per-1e9") == 0 ||
uprv_strcmp(testCase.source, "portion-per-1E9") == 0 ||
uprv_strcmp(testCase.source, "meter-per-100-kilometer") == 0) {
logKnownIssue("ICU-23045", "Incorrect constant denominator for certain unit identifiers");
continue;
}

MeasureUnit unit = MeasureUnit::forIdentifier(testCase.source, status);
if (status.errIfFailureAndReset("forIdentifier(\"%s\")", testCase.source)) {
continue;
Expand Down
107 changes: 68 additions & 39 deletions icu4j/main/common_tests/src/test/java/com/ibm/icu/dev/test/format/MeasureUnitTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1330,12 +1330,22 @@ class ConstantDenominatorTestCase {
List<ConstantDenominatorTestCase> testCases = Arrays.asList(
new ConstantDenominatorTestCase("meter-per-1000", 1000),
new ConstantDenominatorTestCase("liter-per-1000-kiloliter", 1000),
new ConstantDenominatorTestCase("meter-per-100-kilometer", 100),
new ConstantDenominatorTestCase("liter-per-kilometer", 0),
new ConstantDenominatorTestCase("second-per-1000-minute", 1000),
new ConstantDenominatorTestCase("gram-per-1000-kilogram", 1000),
new ConstantDenominatorTestCase("meter-per-100", 100),
// Test for constant denominators that are powers of 10
new ConstantDenominatorTestCase("meter-per-100", 100), // Failing ICU-23045
new ConstantDenominatorTestCase("portion-per-1", 1),
new ConstantDenominatorTestCase("portion-per-2", 2),
new ConstantDenominatorTestCase("portion-per-3", 3),
new ConstantDenominatorTestCase("portion-per-4", 4),
new ConstantDenominatorTestCase("portion-per-5", 5),
new ConstantDenominatorTestCase("portion-per-6", 6),
new ConstantDenominatorTestCase("portion-per-7", 7),
new ConstantDenominatorTestCase("portion-per-8", 8),
new ConstantDenominatorTestCase("portion-per-9", 9),

// Test for constant denominators that are powers of 10
new ConstantDenominatorTestCase("portion-per-10", 10),
new ConstantDenominatorTestCase("portion-per-100", 100),
new ConstantDenominatorTestCase("portion-per-1000", 1000),
Expand All @@ -1344,46 +1354,58 @@ class ConstantDenominatorTestCase {
new ConstantDenominatorTestCase("portion-per-1000000", 1000000),
new ConstantDenominatorTestCase("portion-per-10000000", 10000000),
new ConstantDenominatorTestCase("portion-per-100000000", 100000000),
// ICU-22781: new ConstantDenominatorTestCase("portion-per-1000000000", 1000000000),
// ICU-22781: new ConstantDenominatorTestCase("portion-per-10000000000", 10000000000L),
// ICU-22781: new ConstantDenominatorTestCase("portion-per-100000000000", 100000000000L),
// ICU-22781: new ConstantDenominatorTestCase("portion-per-1000000000000", 1000000000000L),
// ICU-22781: new ConstantDenominatorTestCase("portion-per-10000000000000", 10000000000000L),
// ICU-22781: new ConstantDenominatorTestCase("portion-per-100000000000000", 100000000000000L),
// ICU-22781: new ConstantDenominatorTestCase("portion-per-1000000000000000", 1000000000000000L),
// ICU-22781: new ConstantDenominatorTestCase("portion-per-10000000000000000", 10000000000000000L),
// ICU-22781: new ConstantDenominatorTestCase("portion-per-100000000000000000", 100000000000000000L),
// ICU-22781: new ConstantDenominatorTestCase("portion-per-1000000000000000000", 1000000000000000000L),
// Test for constant denominators that are represented as scientific notation
// numbers.
// ICU-22781: new ConstantDenominatorTestCase("portion-per-1e9", 1000000000L),
// ICU-22781: new ConstantDenominatorTestCase("portion-per-1E9", 1000000000L),
// ICU-22781: new ConstantDenominatorTestCase("portion-per-10e9", 10000000000L),
// ICU-22781: new ConstantDenominatorTestCase("portion-per-10E9", 10000000000L),
// ICU-22781: new ConstantDenominatorTestCase("portion-per-1e10", 10000000000L),
// ICU-22781: new ConstantDenominatorTestCase("portion-per-1E10", 10000000000L),
// ICU-22781: new ConstantDenominatorTestCase("portion-per-1e3-kilometer", 1000),
// Test for constant denominators that are randomely selected.
// ICU-22781: new ConstantDenominatorTestCase("liter-per-12345-kilometer", 12345),
// ICU-22781: new ConstantDenominatorTestCase("per-1000-kilometer", 1000),
// ICU-22781: new ConstantDenominatorTestCase("liter-per-1000-kiloliter", 1000),
// Test for constant denominators that gives 0.
new ConstantDenominatorTestCase("meter", 0),
new ConstantDenominatorTestCase("portion-per-1000000000", 1000000000), // Failing ICU-23045
new ConstantDenominatorTestCase("portion-per-10000000000", 10000000000L),
new ConstantDenominatorTestCase("portion-per-100000000000", 100000000000L),
new ConstantDenominatorTestCase("portion-per-1000000000000", 1000000000000L),
new ConstantDenominatorTestCase("portion-per-10000000000000", 10000000000000L),
new ConstantDenominatorTestCase("portion-per-100000000000000", 100000000000000L),
new ConstantDenominatorTestCase("portion-per-1000000000000000", 1000000000000000L),
new ConstantDenominatorTestCase("portion-per-10000000000000000", 10000000000000000L),
new ConstantDenominatorTestCase("portion-per-100000000000000000", 100000000000000000L),
new ConstantDenominatorTestCase("portion-per-1000000000000000000", 1000000000000000000L),
new ConstantDenominatorTestCase("portion-per-1e3-kilometer", 1000),

// Test for constant denominators that are represented as scientific notation numbers.
new ConstantDenominatorTestCase("portion-per-1e1", 10),
new ConstantDenominatorTestCase("portion-per-1E1", 10),
new ConstantDenominatorTestCase("portion-per-1e2", 100),
new ConstantDenominatorTestCase("portion-per-1E2", 100),
new ConstantDenominatorTestCase("portion-per-1e3", 1000),
new ConstantDenominatorTestCase("portion-per-1E3", 1000),
new ConstantDenominatorTestCase("portion-per-1e4", 10000),
new ConstantDenominatorTestCase("portion-per-1E4", 10000),
new ConstantDenominatorTestCase("portion-per-1e5", 100000),
new ConstantDenominatorTestCase("portion-per-1E5", 100000),
new ConstantDenominatorTestCase("portion-per-1e6", 1000000),
new ConstantDenominatorTestCase("portion-per-1E6", 1000000),
new ConstantDenominatorTestCase("portion-per-1e9", 1000000000), // Failing ICU-23045
new ConstantDenominatorTestCase("portion-per-1E9", 1000000000), // Failing ICU-23045
new ConstantDenominatorTestCase("portion-per-1e10", 10000000000L),
new ConstantDenominatorTestCase("portion-per-1E10", 10000000000L),
new ConstantDenominatorTestCase("portion-per-1e18", 1000000000000000000L),
new ConstantDenominatorTestCase("portion-per-1E18", 1000000000000000000L),

// Test for constant denominators that are randomly selected.
new ConstantDenominatorTestCase("liter-per-12345-kilometer", 12345),
new ConstantDenominatorTestCase("per-1000-kilometer", 1000),
new ConstantDenominatorTestCase("liter-per-1000-kiloliter", 1000),

// Test for constant denominators that give 0.
new ConstantDenominatorTestCase("meter", 0),
new ConstantDenominatorTestCase("meter-per-second", 0),
new ConstantDenominatorTestCase("meter-per-square-second", 0),
// NOTE: The following constant denominator should be 0. However, since
// `100-kilometer` is treated as a unit in CLDR,
// the unit does not have a constant denominator.
// This issue should be addressed in CLDR.
new ConstantDenominatorTestCase("meter-per-100-kilometer", 0),
// NOTE: the following CLDR identifier should be invalid, but because
// `100-kilometer` is considered a unit in CLDR,
// one `100` will be considered as a unit constant denominator and the other
// `100` will be considered part of the unit.
// This issue should be addressed in CLDR.
new ConstantDenominatorTestCase("meter-per-100-100-kilometer", 100));
new ConstantDenominatorTestCase("meter-per-square-second", 0));

for (ConstantDenominatorTestCase testCase : testCases) {
switch (testCase.identifier) {
case "portion-per-1000000000":
case "portion-per-1e9":
case "portion-per-1E9":
case "meter-per-100-kilometer":
logKnownIssue("ICU-23045", "Incorrect constant denominator for certain unit identifiers");
continue;
}

MeasureUnit unit = MeasureUnit.forIdentifier(testCase.identifier);
assertEquals("Constant denominator for " + testCase.identifier, testCase.expectedConstantDenominator,
unit.getConstantDenominator());
Expand Down Expand Up @@ -1443,9 +1465,16 @@ public void TestInvalidIdentifiers() {
"meter-per-1000-second-1000-kilometer",
"per-1000-and-per-1000",
"liter-per-kilometer-100",
"meter-per-100-100-kilometer", // Failing ICU-23045
};

for (String input : inputs) {
if (input.equals("meter-per-100-100-kilometer")) {
logKnownIssue("ICU-23045", "Incorrect constant denominator for certain unit identifiers " +
"leads to incorrect unit identifiers.");
continue;
}

try {
MeasureUnit.forIdentifier(input);
Assert.fail("An IllegalArgumentException must be thrown");
Expand Down