From 86b9cdad274a880de129d24a39fa5e3544753694 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Thu, 30 Apr 2020 12:54:11 +0200 Subject: [PATCH 01/10] Create assertEquals for double with a delta parameter. --- icu4c/source/test/intltest/intltest.cpp | 27 +++++++++++++++++++++++- icu4c/source/test/intltest/intltest.h | 28 +++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/icu4c/source/test/intltest/intltest.cpp b/icu4c/source/test/intltest/intltest.cpp index 993222b2448..d222d46d883 100644 --- a/icu4c/source/test/intltest/intltest.cpp +++ b/icu4c/source/test/intltest/intltest.cpp @@ -2026,7 +2026,8 @@ UBool IntlTest::assertEquals(const char* message, double expected, double actual) { bool bothNaN = std::isnan(expected) && std::isnan(actual); - if (expected != actual && !bothNaN) { + bool bothInf = std::isinf(expected) && std::isinf(actual); + if (expected != actual && !bothNaN && !bothInf) { errln((UnicodeString)"FAIL: " + message + "; got " + actual + "; expected " + expected); @@ -2040,6 +2041,24 @@ UBool IntlTest::assertEquals(const char* message, return TRUE; } +UBool IntlTest::assertEquals(const char* message, + double expected, + double actual, + double delta) { + bool bothNaN = std::isnan(expected) && std::isnan(actual); + bool bothInf = std::isinf(expected) && std::isinf(actual); + if (abs(expected - actual) > delta && !bothNaN && !bothInf) { + errln((UnicodeString)("FAIL: ") + message + "; got " + actual + "; expected " + expected + + "; acceptable delta " + delta); + return FALSE; + } +#ifdef VERBOSE_ASSERTIONS + else { + logln((UnicodeString)("Ok: ") + message + "; got " + actual); + } +#endif + return TRUE; +} UBool IntlTest::assertEquals(const char* message, UBool expected, @@ -2249,6 +2268,12 @@ UBool IntlTest::assertEquals(const UnicodeString& message, double actual) { return assertEquals(extractToAssertBuf(message), expected, actual); } +UBool IntlTest::assertEquals(const UnicodeString& message, + double expected, + double actual, + double delta) { + return assertEquals(extractToAssertBuf(message), expected, actual, delta); +} UBool IntlTest::assertEquals(const UnicodeString& message, UErrorCode expected, UErrorCode actual) { diff --git a/icu4c/source/test/intltest/intltest.h b/icu4c/source/test/intltest/intltest.h index 6fb2e9ff460..b3eec2ba01b 100644 --- a/icu4c/source/test/intltest/intltest.h +++ b/icu4c/source/test/intltest/intltest.h @@ -299,6 +299,20 @@ public: UBool assertEquals(const char* message, int32_t expected, int32_t actual); UBool assertEquals(const char* message, int64_t expected, int64_t actual); UBool assertEquals(const char* message, double expected, double actual); + /** + * Asserts that two doubles are equal to within a positive delta. Returns + * false if they are not. + * + * If the expected value is infinity then the delta value is ignored. NaNs + * are considered equal: assertEquals(msg, NaN, NaN, *) passes. + * + * @param message - the identifying message for the AssertionError. + * @param expected - expected value. + * @param actual - the value to check against expected. + * @param delta - the maximum delta between expected and actual for which + * both numbers are still considered equal. + */ + UBool assertEquals(const char* message, double expected, double actual, double delta); UBool assertEquals(const char* message, UErrorCode expected, UErrorCode actual); UBool assertEquals(const char* message, const UnicodeSet& expected, const UnicodeSet& actual); UBool assertEquals(const char* message, @@ -321,6 +335,20 @@ public: UBool assertEquals(const UnicodeString& message, int32_t expected, int32_t actual); UBool assertEquals(const UnicodeString& message, int64_t expected, int64_t actual); UBool assertEquals(const UnicodeString& message, double expected, double actual); + /** + * Asserts that two doubles are equal to within a positive delta. Returns + * false if they are not. + * + * If the expected value is infinity then the delta value is ignored. NaNs + * are considered equal: assertEquals(msg, NaN, NaN, *) passes. + * + * @param message - the identifying message for the AssertionError. + * @param expected - expected value. + * @param actual - the value to check against expected. + * @param delta - the maximum delta between expected and actual for which + * both numbers are still considered equal. + */ + UBool assertEquals(const UnicodeString& message, double expected, double actual, double delta); UBool assertEquals(const UnicodeString& message, UErrorCode expected, UErrorCode actual); UBool assertEquals(const UnicodeString& message, const UnicodeSet& expected, const UnicodeSet& actual); UBool assertEquals(const UnicodeString& message, From 8cc7c94138e0a71c1ddd337bcfc8c149436fbbc8 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Thu, 30 Apr 2020 12:54:11 +0200 Subject: [PATCH 02/10] Update code for correct NaN/Inf handling. --- icu4c/source/test/intltest/intltest.cpp | 12 ++++++++---- icu4c/source/test/intltest/intltest.h | 8 ++++---- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/icu4c/source/test/intltest/intltest.cpp b/icu4c/source/test/intltest/intltest.cpp index d222d46d883..9f4ae348ab9 100644 --- a/icu4c/source/test/intltest/intltest.cpp +++ b/icu4c/source/test/intltest/intltest.cpp @@ -2026,8 +2026,7 @@ UBool IntlTest::assertEquals(const char* message, double expected, double actual) { bool bothNaN = std::isnan(expected) && std::isnan(actual); - bool bothInf = std::isinf(expected) && std::isinf(actual); - if (expected != actual && !bothNaN && !bothInf) { + if (expected != actual && !bothNaN) { errln((UnicodeString)"FAIL: " + message + "; got " + actual + "; expected " + expected); @@ -2045,9 +2044,14 @@ UBool IntlTest::assertEquals(const char* message, double expected, double actual, double delta) { + if (std::isnan(delta) || std::isinf(delta)) { + errln((UnicodeString)("FAIL: ") + message + "; nonsensical delta " + delta + + " - delta may not be NaN or Inf"); + return FALSE; + } bool bothNaN = std::isnan(expected) && std::isnan(actual); - bool bothInf = std::isinf(expected) && std::isinf(actual); - if (abs(expected - actual) > delta && !bothNaN && !bothInf) { + double difference = abs(expected - actual); + if (expected != actual && (difference > delta || std::isnan(difference)) && !bothNaN) { errln((UnicodeString)("FAIL: ") + message + "; got " + actual + "; expected " + expected + "; acceptable delta " + delta); return FALSE; diff --git a/icu4c/source/test/intltest/intltest.h b/icu4c/source/test/intltest/intltest.h index b3eec2ba01b..ada0c0bfd6d 100644 --- a/icu4c/source/test/intltest/intltest.h +++ b/icu4c/source/test/intltest/intltest.h @@ -303,8 +303,8 @@ public: * Asserts that two doubles are equal to within a positive delta. Returns * false if they are not. * - * If the expected value is infinity then the delta value is ignored. NaNs - * are considered equal: assertEquals(msg, NaN, NaN, *) passes. + * NaNs are considered equal: assertEquals(msg, NaN, NaN, *) passes. + * Infs are considered equal: assertEquals(msg, inf, inf, *) passes. * * @param message - the identifying message for the AssertionError. * @param expected - expected value. @@ -339,8 +339,8 @@ public: * Asserts that two doubles are equal to within a positive delta. Returns * false if they are not. * - * If the expected value is infinity then the delta value is ignored. NaNs - * are considered equal: assertEquals(msg, NaN, NaN, *) passes. + * NaNs are considered equal: assertEquals(msg, NaN, NaN, *) passes. + * Infs are considered equal: assertEquals(msg, inf, inf, *) passes. * * @param message - the identifying message for the AssertionError. * @param expected - expected value. From 8972e475fca868b9024a24919bb4d31d98138a6b Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Mon, 11 May 2020 16:53:29 +0200 Subject: [PATCH 03/10] Use JUnit-like assertEquals. Improve test messages. --- icu4c/source/test/intltest/intltest.cpp | 41 +++++++++++---------- icu4c/source/test/intltest/unitstest.cpp | 46 +++++++++++++----------- 2 files changed, 48 insertions(+), 39 deletions(-) diff --git a/icu4c/source/test/intltest/intltest.cpp b/icu4c/source/test/intltest/intltest.cpp index 9f4ae348ab9..7d04283ea27 100644 --- a/icu4c/source/test/intltest/intltest.cpp +++ b/icu4c/source/test/intltest/intltest.cpp @@ -2197,26 +2197,29 @@ UBool IntlTest::assertNotEquals(const char* message, return TRUE; } -// http://junit.sourceforge.net/javadoc/org/junit/Assert.html#assertEquals(java.lang.String,%20double,%20double,%20double) -UBool IntlTest::assertEqualsNear(const char *message, double expected, double actual, double precision) { - double diff = std::abs(expected - actual); - double diffPercent = - expected != 0 ? diff / expected - : diff; // If the expected is equals zero, we assume that - // the `diffPercent` is equal to the difference - // between the actual and the expected +// // TODO: the following link is misleading, since JUnit does absolute numbers and +// // this code does relative, and this code doesn't support NaN whereas JUnit +// // does. JUnit's function is also simply overloading "assertEquals". +// // http://junit.sourceforge.net/javadoc/org/junit/Assert.html#assertEquals(java.lang.String,%20double,%20double,%20double) +// UBool IntlTest::assertEqualsNear(const char *message, double expected, double actual, double precision) { +// double diff = std::abs(expected - actual); +// double diffPercent = +// expected != 0 ? diff / expected +// : diff; // If the expected is equals zero, we assume that +// // the `diffPercent` is equal to the difference +// // between the actual and the expected - if (diffPercent > precision) { - errln((UnicodeString) "FAIL: " + message + "; got " + actual + "; expected " + expected); - return FALSE; - } -#ifdef VERBOSE_ASSERTIONS - else { - logln((UnicodeString) "Ok: " + message + "; got " + expected); - } -#endif - return TRUE; -} +// if (diffPercent > precision) { +// errln((UnicodeString) "FAIL: " + message + "; got " + actual + "; expected " + expected); +// return FALSE; +// } +// #ifdef VERBOSE_ASSERTIONS +// else { +// logln((UnicodeString) "Ok: " + message + "; got " + expected); +// } +// #endif +// return TRUE; +// } static char ASSERT_BUF[256]; diff --git a/icu4c/source/test/intltest/unitstest.cpp b/icu4c/source/test/intltest/unitstest.cpp index bb5d216fab1..0d80b928a37 100644 --- a/icu4c/source/test/intltest/unitstest.cpp +++ b/icu4c/source/test/intltest/unitstest.cpp @@ -65,8 +65,8 @@ void UnitsTest::runIndexedTest(int32_t index, UBool exec, const char *&name, cha void UnitsTest::testConversionCapability() { struct TestCase { - const StringPiece source; - const StringPiece target; + const char *source; + const char *target; const UnitsConvertibilityState expectedState; } testCases[]{ {"meter", "foot", CONVERTIBLE}, // @@ -88,7 +88,9 @@ void UnitsTest::testConversionCapability() { ConversionRates conversionRates(status); auto convertibility = checkConvertibility(source, target, conversionRates, status); - assertEquals("Conversion Capability", testCase.expectedState, convertibility); + assertEquals(UnicodeString("Conversion Capability: ") + testCase.source + " to " + + testCase.target, + testCase.expectedState, convertibility); } } @@ -96,8 +98,8 @@ void UnitsTest::testSiPrefixes() { IcuTestErrorCode status(*this, "Units testSiPrefixes"); // Test Cases struct TestCase { - StringPiece source; - StringPiece target; + const char *source; + const char *target; const double inputValue; const double expectedValue; } testCases[]{ @@ -121,8 +123,9 @@ void UnitsTest::testSiPrefixes() { ConversionRates conversionRates(status); UnitConverter converter(source, target, conversionRates, status); - assertEqualsNear("test conversion", testCase.expectedValue, - converter.convert(testCase.inputValue), 0.001); + assertEquals(UnicodeString("testSiPrefixes: ") + testCase.source + " to " + testCase.target, + testCase.expectedValue, converter.convert(testCase.inputValue), + 0.0001 * testCase.expectedValue); } } @@ -131,8 +134,8 @@ void UnitsTest::testMass() { // Test Cases struct TestCase { - StringPiece source; - StringPiece target; + const char *source; + const char *target; const double inputValue; const double expectedValue; } testCases[]{ @@ -155,8 +158,9 @@ void UnitsTest::testMass() { ConversionRates conversionRates(status); UnitConverter converter(source, target, conversionRates, status); - assertEqualsNear("test conversion", testCase.expectedValue, - converter.convert(testCase.inputValue), 0.001); + assertEquals(UnicodeString("testMass: ") + testCase.source + " to " + testCase.target, + testCase.expectedValue, converter.convert(testCase.inputValue), + 0.0001 * testCase.expectedValue); } } @@ -164,8 +168,8 @@ void UnitsTest::testTemperature() { IcuTestErrorCode status(*this, "Units testTemperature"); // Test Cases struct TestCase { - StringPiece source; - StringPiece target; + const char *source; + const char *target; const double inputValue; const double expectedValue; } testCases[]{ @@ -188,8 +192,9 @@ void UnitsTest::testTemperature() { ConversionRates conversionRates(status); UnitConverter converter(source, target, conversionRates, status); - assertEqualsNear("test conversion", testCase.expectedValue, - converter.convert(testCase.inputValue), 0.001); + assertEquals(UnicodeString("testTemperature: ") + testCase.source + " to " + testCase.target, + testCase.expectedValue, converter.convert(testCase.inputValue), + 0.0001 * abs(testCase.expectedValue)); } } @@ -198,8 +203,8 @@ void UnitsTest::testArea() { // Test Cases struct TestCase { - StringPiece source; - StringPiece target; + const char *source; + const char *target; const double inputValue; const double expectedValue; } testCases[]{ @@ -225,8 +230,9 @@ void UnitsTest::testArea() { ConversionRates conversionRates(status); UnitConverter converter(source, target, conversionRates, status); - assertEqualsNear("test conversion", testCase.expectedValue, - converter.convert(testCase.inputValue), 0.001); + assertEquals(UnicodeString("testArea: ") + testCase.source + " to " + testCase.target, + testCase.expectedValue, converter.convert(testCase.inputValue), + 0.0001 * testCase.expectedValue); } } @@ -337,7 +343,7 @@ void unitsTestDataLineFn(void *context, char *fields[][2], int32_t fieldCount, U double got = converter.convert(1000); msg.clear(); msg.append("Converting 1000 ", status).append(x, status).append(" to ", status).append(y, status); - unitsTest->assertEqualsNear(msg.data(), expected, got, 0.0001); + unitsTest->assertEquals(msg.data(), expected, got, 0.0001 * expected); } /** From 2701c326e119575d77ee0a207be6bb0450f16015 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Mon, 11 May 2020 17:02:28 +0200 Subject: [PATCH 04/10] Improve @param delta documentation. --- icu4c/source/test/intltest/intltest.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/icu4c/source/test/intltest/intltest.h b/icu4c/source/test/intltest/intltest.h index ada0c0bfd6d..91cf83a39c5 100644 --- a/icu4c/source/test/intltest/intltest.h +++ b/icu4c/source/test/intltest/intltest.h @@ -309,8 +309,8 @@ public: * @param message - the identifying message for the AssertionError. * @param expected - expected value. * @param actual - the value to check against expected. - * @param delta - the maximum delta between expected and actual for which - * both numbers are still considered equal. + * @param delta - the maximum delta for the absolute difference between + * expected and actual for which both numbers are still considered equal. */ UBool assertEquals(const char* message, double expected, double actual, double delta); UBool assertEquals(const char* message, UErrorCode expected, UErrorCode actual); From f6837f333b1a540a0b152e5f080ff70cdacfe101 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Mon, 11 May 2020 19:12:41 +0200 Subject: [PATCH 05/10] Attempt to disambiguate 'abs', for MacOSX build failure. --- icu4c/source/test/intltest/unitstest.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/icu4c/source/test/intltest/unitstest.cpp b/icu4c/source/test/intltest/unitstest.cpp index 0d80b928a37..9971793051c 100644 --- a/icu4c/source/test/intltest/unitstest.cpp +++ b/icu4c/source/test/intltest/unitstest.cpp @@ -194,7 +194,7 @@ void UnitsTest::testTemperature() { assertEquals(UnicodeString("testTemperature: ") + testCase.source + " to " + testCase.target, testCase.expectedValue, converter.convert(testCase.inputValue), - 0.0001 * abs(testCase.expectedValue)); + 0.0001 * abs((double)testCase.expectedValue)); } } From 072423ece6a7015b14cf4edf7303eef294912a1a Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Mon, 11 May 2020 20:19:50 +0200 Subject: [PATCH 06/10] Use uprv_fabs. (abs() is for integer types.) --- icu4c/source/test/intltest/unitstest.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/icu4c/source/test/intltest/unitstest.cpp b/icu4c/source/test/intltest/unitstest.cpp index 9971793051c..ab94df55dbd 100644 --- a/icu4c/source/test/intltest/unitstest.cpp +++ b/icu4c/source/test/intltest/unitstest.cpp @@ -13,6 +13,7 @@ #include "filestrm.h" #include "intltest.h" #include "number_decimalquantity.h" +#include "putilimp.h" #include "unicode/ctest.h" #include "unicode/measunit.h" #include "unicode/unistr.h" @@ -194,7 +195,7 @@ void UnitsTest::testTemperature() { assertEquals(UnicodeString("testTemperature: ") + testCase.source + " to " + testCase.target, testCase.expectedValue, converter.convert(testCase.inputValue), - 0.0001 * abs((double)testCase.expectedValue)); + 0.0001 * uprv_fabs(testCase.expectedValue)); } } From cee268df398b660f44270ae2dee282f41e58f392 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Wed, 10 Jun 2020 00:55:20 +0200 Subject: [PATCH 07/10] Code review feedback. Temporarily 'assertEqualsNewNear'. --- icu4c/source/test/intltest/intltest.cpp | 46 +++++++----------------- icu4c/source/test/intltest/intltest.h | 10 ++++-- icu4c/source/test/intltest/unitstest.cpp | 14 ++++---- 3 files changed, 28 insertions(+), 42 deletions(-) diff --git a/icu4c/source/test/intltest/intltest.cpp b/icu4c/source/test/intltest/intltest.cpp index 7d04283ea27..ebb6729317b 100644 --- a/icu4c/source/test/intltest/intltest.cpp +++ b/icu4c/source/test/intltest/intltest.cpp @@ -2040,10 +2040,12 @@ UBool IntlTest::assertEquals(const char* message, return TRUE; } -UBool IntlTest::assertEquals(const char* message, - double expected, - double actual, - double delta) { +// FIXME: rename to assertEqualsNear. It's temporarily "NewNear" just to ensure +// we catch all callsites (to change last parameter from relative to absolute). +UBool IntlTest::assertEqualsNewNear(const char* message, + double expected, + double actual, + double delta) { if (std::isnan(delta) || std::isinf(delta)) { errln((UnicodeString)("FAIL: ") + message + "; nonsensical delta " + delta + " - delta may not be NaN or Inf"); @@ -2197,30 +2199,6 @@ UBool IntlTest::assertNotEquals(const char* message, return TRUE; } -// // TODO: the following link is misleading, since JUnit does absolute numbers and -// // this code does relative, and this code doesn't support NaN whereas JUnit -// // does. JUnit's function is also simply overloading "assertEquals". -// // http://junit.sourceforge.net/javadoc/org/junit/Assert.html#assertEquals(java.lang.String,%20double,%20double,%20double) -// UBool IntlTest::assertEqualsNear(const char *message, double expected, double actual, double precision) { -// double diff = std::abs(expected - actual); -// double diffPercent = -// expected != 0 ? diff / expected -// : diff; // If the expected is equals zero, we assume that -// // the `diffPercent` is equal to the difference -// // between the actual and the expected - -// if (diffPercent > precision) { -// errln((UnicodeString) "FAIL: " + message + "; got " + actual + "; expected " + expected); -// return FALSE; -// } -// #ifdef VERBOSE_ASSERTIONS -// else { -// logln((UnicodeString) "Ok: " + message + "; got " + expected); -// } -// #endif -// return TRUE; -// } - static char ASSERT_BUF[256]; static const char* extractToAssertBuf(const UnicodeString& message) { @@ -2275,11 +2253,13 @@ UBool IntlTest::assertEquals(const UnicodeString& message, double actual) { return assertEquals(extractToAssertBuf(message), expected, actual); } -UBool IntlTest::assertEquals(const UnicodeString& message, - double expected, - double actual, - double delta) { - return assertEquals(extractToAssertBuf(message), expected, actual, delta); +// FIXME: rename to assertEqualsNear. It's temporarily "NewNear" just to ensure +// we catch all callsites (to change last parameter from relative to absolute). +UBool IntlTest::assertEqualsNewNear(const UnicodeString& message, + double expected, + double actual, + double delta) { + return assertEqualsNewNear(extractToAssertBuf(message), expected, actual, delta); } UBool IntlTest::assertEquals(const UnicodeString& message, UErrorCode expected, diff --git a/icu4c/source/test/intltest/intltest.h b/icu4c/source/test/intltest/intltest.h index 91cf83a39c5..8414f3124fd 100644 --- a/icu4c/source/test/intltest/intltest.h +++ b/icu4c/source/test/intltest/intltest.h @@ -299,6 +299,9 @@ public: UBool assertEquals(const char* message, int32_t expected, int32_t actual); UBool assertEquals(const char* message, int64_t expected, int64_t actual); UBool assertEquals(const char* message, double expected, double actual); + // FIXME: rename to assertEqualsNear. It's temporarily "NewNear" just to + // ensure we catch all callsites (to change last parameter from relative to + // absolute). /** * Asserts that two doubles are equal to within a positive delta. Returns * false if they are not. @@ -312,7 +315,7 @@ public: * @param delta - the maximum delta for the absolute difference between * expected and actual for which both numbers are still considered equal. */ - UBool assertEquals(const char* message, double expected, double actual, double delta); + UBool assertEqualsNewNear(const char* message, double expected, double actual, double delta); UBool assertEquals(const char* message, UErrorCode expected, UErrorCode actual); UBool assertEquals(const char* message, const UnicodeSet& expected, const UnicodeSet& actual); UBool assertEquals(const char* message, @@ -335,6 +338,9 @@ public: UBool assertEquals(const UnicodeString& message, int32_t expected, int32_t actual); UBool assertEquals(const UnicodeString& message, int64_t expected, int64_t actual); UBool assertEquals(const UnicodeString& message, double expected, double actual); + // FIXME: rename to assertEqualsNear. It's temporarily "NewNear" just to + // ensure we catch all callsites (to change last parameter from relative to + // absolute). /** * Asserts that two doubles are equal to within a positive delta. Returns * false if they are not. @@ -348,7 +354,7 @@ public: * @param delta - the maximum delta between expected and actual for which * both numbers are still considered equal. */ - UBool assertEquals(const UnicodeString& message, double expected, double actual, double delta); + UBool assertEqualsNewNear(const UnicodeString& message, double expected, double actual, double delta); UBool assertEquals(const UnicodeString& message, UErrorCode expected, UErrorCode actual); UBool assertEquals(const UnicodeString& message, const UnicodeSet& expected, const UnicodeSet& actual); UBool assertEquals(const UnicodeString& message, diff --git a/icu4c/source/test/intltest/unitstest.cpp b/icu4c/source/test/intltest/unitstest.cpp index ab94df55dbd..9b27be5eaf1 100644 --- a/icu4c/source/test/intltest/unitstest.cpp +++ b/icu4c/source/test/intltest/unitstest.cpp @@ -124,7 +124,7 @@ void UnitsTest::testSiPrefixes() { ConversionRates conversionRates(status); UnitConverter converter(source, target, conversionRates, status); - assertEquals(UnicodeString("testSiPrefixes: ") + testCase.source + " to " + testCase.target, + assertEqualsNewNear(UnicodeString("testSiPrefixes: ") + testCase.source + " to " + testCase.target, testCase.expectedValue, converter.convert(testCase.inputValue), 0.0001 * testCase.expectedValue); } @@ -159,9 +159,9 @@ void UnitsTest::testMass() { ConversionRates conversionRates(status); UnitConverter converter(source, target, conversionRates, status); - assertEquals(UnicodeString("testMass: ") + testCase.source + " to " + testCase.target, - testCase.expectedValue, converter.convert(testCase.inputValue), - 0.0001 * testCase.expectedValue); + assertEqualsNewNear(UnicodeString("testMass: ") + testCase.source + " to " + testCase.target, + testCase.expectedValue, converter.convert(testCase.inputValue), + 0.0001 * testCase.expectedValue); } } @@ -193,7 +193,7 @@ void UnitsTest::testTemperature() { ConversionRates conversionRates(status); UnitConverter converter(source, target, conversionRates, status); - assertEquals(UnicodeString("testTemperature: ") + testCase.source + " to " + testCase.target, + assertEqualsNewNear(UnicodeString("testTemperature: ") + testCase.source + " to " + testCase.target, testCase.expectedValue, converter.convert(testCase.inputValue), 0.0001 * uprv_fabs(testCase.expectedValue)); } @@ -231,7 +231,7 @@ void UnitsTest::testArea() { ConversionRates conversionRates(status); UnitConverter converter(source, target, conversionRates, status); - assertEquals(UnicodeString("testArea: ") + testCase.source + " to " + testCase.target, + assertEqualsNewNear(UnicodeString("testArea: ") + testCase.source + " to " + testCase.target, testCase.expectedValue, converter.convert(testCase.inputValue), 0.0001 * testCase.expectedValue); } @@ -344,7 +344,7 @@ void unitsTestDataLineFn(void *context, char *fields[][2], int32_t fieldCount, U double got = converter.convert(1000); msg.clear(); msg.append("Converting 1000 ", status).append(x, status).append(" to ", status).append(y, status); - unitsTest->assertEquals(msg.data(), expected, got, 0.0001 * expected); + unitsTest->assertEqualsNewNear(msg.data(), expected, got, 0.0001 * expected); } /** From 399c16597c5fed8ff9a51b92376c5210847a6b16 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Wed, 17 Jun 2020 22:59:56 +0200 Subject: [PATCH 08/10] Temporarily restore assertEqualsNear. This function was restored as part of the original merge of units-staging into this development branch, in case it was in used in new code in units-staging. With the rebase, we are thus also restoring it temporarily, at the point in the sequence of commits where the merge had been. --- icu4c/source/test/intltest/intltest.cpp | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/icu4c/source/test/intltest/intltest.cpp b/icu4c/source/test/intltest/intltest.cpp index ebb6729317b..42d7f37756d 100644 --- a/icu4c/source/test/intltest/intltest.cpp +++ b/icu4c/source/test/intltest/intltest.cpp @@ -2199,6 +2199,27 @@ UBool IntlTest::assertNotEquals(const char* message, return TRUE; } +// http://junit.sourceforge.net/javadoc/org/junit/Assert.html#assertEquals(java.lang.String,%20double,%20double,%20double) +UBool IntlTest::assertEqualsNear(const char *message, double expected, double actual, double precision) { + double diff = std::abs(expected - actual); + double diffPercent = + expected != 0 ? diff / expected + : diff; // If the expected is equals zero, we assume that + // the `diffPercent` is equal to the difference + // between the actual and the expected + + if (diffPercent > precision) { + errln((UnicodeString) "FAIL: " + message + "; got " + actual + "; expected " + expected); + return FALSE; + } +#ifdef VERBOSE_ASSERTIONS + else { + logln((UnicodeString) "Ok: " + message + "; got " + expected); + } +#endif + return TRUE; +} + static char ASSERT_BUF[256]; static const char* extractToAssertBuf(const UnicodeString& message) { From a20b1cd4eac83cdf9466b12c90a058ed36b94081 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Thu, 18 Jun 2020 00:39:53 +0200 Subject: [PATCH 09/10] Move new assertEqualsNear code into place. --- icu4c/source/test/intltest/intltest.cpp | 67 +++++++-------------- icu4c/source/test/intltest/intltest.h | 12 +--- icu4c/source/test/intltest/unitstest.cpp | 74 ++++++++++-------------- 3 files changed, 55 insertions(+), 98 deletions(-) diff --git a/icu4c/source/test/intltest/intltest.cpp b/icu4c/source/test/intltest/intltest.cpp index 42d7f37756d..04b3bd145b5 100644 --- a/icu4c/source/test/intltest/intltest.cpp +++ b/icu4c/source/test/intltest/intltest.cpp @@ -2040,32 +2040,6 @@ UBool IntlTest::assertEquals(const char* message, return TRUE; } -// FIXME: rename to assertEqualsNear. It's temporarily "NewNear" just to ensure -// we catch all callsites (to change last parameter from relative to absolute). -UBool IntlTest::assertEqualsNewNear(const char* message, - double expected, - double actual, - double delta) { - if (std::isnan(delta) || std::isinf(delta)) { - errln((UnicodeString)("FAIL: ") + message + "; nonsensical delta " + delta + - " - delta may not be NaN or Inf"); - return FALSE; - } - bool bothNaN = std::isnan(expected) && std::isnan(actual); - double difference = abs(expected - actual); - if (expected != actual && (difference > delta || std::isnan(difference)) && !bothNaN) { - errln((UnicodeString)("FAIL: ") + message + "; got " + actual + "; expected " + expected + - "; acceptable delta " + delta); - return FALSE; - } -#ifdef VERBOSE_ASSERTIONS - else { - logln((UnicodeString)("Ok: ") + message + "; got " + actual); - } -#endif - return TRUE; -} - UBool IntlTest::assertEquals(const char* message, UBool expected, UBool actual) { @@ -2199,22 +2173,25 @@ UBool IntlTest::assertNotEquals(const char* message, return TRUE; } -// http://junit.sourceforge.net/javadoc/org/junit/Assert.html#assertEquals(java.lang.String,%20double,%20double,%20double) -UBool IntlTest::assertEqualsNear(const char *message, double expected, double actual, double precision) { - double diff = std::abs(expected - actual); - double diffPercent = - expected != 0 ? diff / expected - : diff; // If the expected is equals zero, we assume that - // the `diffPercent` is equal to the difference - // between the actual and the expected - - if (diffPercent > precision) { - errln((UnicodeString) "FAIL: " + message + "; got " + actual + "; expected " + expected); +UBool IntlTest::assertEqualsNear(const char* message, + double expected, + double actual, + double delta) { + if (std::isnan(delta) || std::isinf(delta)) { + errln((UnicodeString)("FAIL: ") + message + "; nonsensical delta " + delta + + " - delta may not be NaN or Inf"); + return FALSE; + } + bool bothNaN = std::isnan(expected) && std::isnan(actual); + double difference = abs(expected - actual); + if (expected != actual && (difference > delta || std::isnan(difference)) && !bothNaN) { + errln((UnicodeString)("FAIL: ") + message + "; got " + actual + "; expected " + expected + + "; acceptable delta " + delta); return FALSE; } #ifdef VERBOSE_ASSERTIONS else { - logln((UnicodeString) "Ok: " + message + "; got " + expected); + logln((UnicodeString)("Ok: ") + message + "; got " + actual); } #endif return TRUE; @@ -2274,14 +2251,6 @@ UBool IntlTest::assertEquals(const UnicodeString& message, double actual) { return assertEquals(extractToAssertBuf(message), expected, actual); } -// FIXME: rename to assertEqualsNear. It's temporarily "NewNear" just to ensure -// we catch all callsites (to change last parameter from relative to absolute). -UBool IntlTest::assertEqualsNewNear(const UnicodeString& message, - double expected, - double actual, - double delta) { - return assertEqualsNewNear(extractToAssertBuf(message), expected, actual, delta); -} UBool IntlTest::assertEquals(const UnicodeString& message, UErrorCode expected, UErrorCode actual) { @@ -2302,6 +2271,12 @@ UBool IntlTest::assertNotEquals(const UnicodeString &message, int32_t actual) { return assertNotEquals(extractToAssertBuf(message), expectedNot, actual); } +UBool IntlTest::assertEqualsNear(const UnicodeString& message, + double expected, + double actual, + double delta) { + return assertEqualsNear(extractToAssertBuf(message), expected, actual, delta); +} #if !UCONFIG_NO_FORMATTING UBool IntlTest::assertEquals(const UnicodeString& message, diff --git a/icu4c/source/test/intltest/intltest.h b/icu4c/source/test/intltest/intltest.h index 8414f3124fd..756bb1cb4f2 100644 --- a/icu4c/source/test/intltest/intltest.h +++ b/icu4c/source/test/intltest/intltest.h @@ -299,9 +299,6 @@ public: UBool assertEquals(const char* message, int32_t expected, int32_t actual); UBool assertEquals(const char* message, int64_t expected, int64_t actual); UBool assertEquals(const char* message, double expected, double actual); - // FIXME: rename to assertEqualsNear. It's temporarily "NewNear" just to - // ensure we catch all callsites (to change last parameter from relative to - // absolute). /** * Asserts that two doubles are equal to within a positive delta. Returns * false if they are not. @@ -315,12 +312,12 @@ public: * @param delta - the maximum delta for the absolute difference between * expected and actual for which both numbers are still considered equal. */ - UBool assertEqualsNewNear(const char* message, double expected, double actual, double delta); + UBool assertEqualsNear(const char* message, double expected, double actual, double delta); UBool assertEquals(const char* message, UErrorCode expected, UErrorCode actual); UBool assertEquals(const char* message, const UnicodeSet& expected, const UnicodeSet& actual); UBool assertEquals(const char* message, const std::vector& expected, const std::vector& actual); - UBool assertEqualsNear(const char* message, double expected, double actual, double precision); + #if !UCONFIG_NO_FORMATTING UBool assertEquals(const char* message, const Formattable& expected, const Formattable& actual, UBool possibleDataError=FALSE); @@ -338,9 +335,6 @@ public: UBool assertEquals(const UnicodeString& message, int32_t expected, int32_t actual); UBool assertEquals(const UnicodeString& message, int64_t expected, int64_t actual); UBool assertEquals(const UnicodeString& message, double expected, double actual); - // FIXME: rename to assertEqualsNear. It's temporarily "NewNear" just to - // ensure we catch all callsites (to change last parameter from relative to - // absolute). /** * Asserts that two doubles are equal to within a positive delta. Returns * false if they are not. @@ -354,7 +348,7 @@ public: * @param delta - the maximum delta between expected and actual for which * both numbers are still considered equal. */ - UBool assertEqualsNewNear(const UnicodeString& message, double expected, double actual, double delta); + UBool assertEqualsNear(const UnicodeString& message, double expected, double actual, double delta); UBool assertEquals(const UnicodeString& message, UErrorCode expected, UErrorCode actual); UBool assertEquals(const UnicodeString& message, const UnicodeSet& expected, const UnicodeSet& actual); UBool assertEquals(const UnicodeString& message, diff --git a/icu4c/source/test/intltest/unitstest.cpp b/icu4c/source/test/intltest/unitstest.cpp index 9b27be5eaf1..0186f87fa28 100644 --- a/icu4c/source/test/intltest/unitstest.cpp +++ b/icu4c/source/test/intltest/unitstest.cpp @@ -124,9 +124,9 @@ void UnitsTest::testSiPrefixes() { ConversionRates conversionRates(status); UnitConverter converter(source, target, conversionRates, status); - assertEqualsNewNear(UnicodeString("testSiPrefixes: ") + testCase.source + " to " + testCase.target, - testCase.expectedValue, converter.convert(testCase.inputValue), - 0.0001 * testCase.expectedValue); + assertEqualsNear(UnicodeString("testSiPrefixes: ") + testCase.source + " to " + testCase.target, + testCase.expectedValue, converter.convert(testCase.inputValue), + 0.0001 * testCase.expectedValue); } } @@ -159,9 +159,9 @@ void UnitsTest::testMass() { ConversionRates conversionRates(status); UnitConverter converter(source, target, conversionRates, status); - assertEqualsNewNear(UnicodeString("testMass: ") + testCase.source + " to " + testCase.target, - testCase.expectedValue, converter.convert(testCase.inputValue), - 0.0001 * testCase.expectedValue); + assertEqualsNear(UnicodeString("testMass: ") + testCase.source + " to " + testCase.target, + testCase.expectedValue, converter.convert(testCase.inputValue), + 0.0001 * testCase.expectedValue); } } @@ -193,9 +193,9 @@ void UnitsTest::testTemperature() { ConversionRates conversionRates(status); UnitConverter converter(source, target, conversionRates, status); - assertEqualsNewNear(UnicodeString("testTemperature: ") + testCase.source + " to " + testCase.target, - testCase.expectedValue, converter.convert(testCase.inputValue), - 0.0001 * uprv_fabs(testCase.expectedValue)); + assertEqualsNear(UnicodeString("testTemperature: ") + testCase.source + " to " + testCase.target, + testCase.expectedValue, converter.convert(testCase.inputValue), + 0.0001 * uprv_fabs(testCase.expectedValue)); } } @@ -231,9 +231,9 @@ void UnitsTest::testArea() { ConversionRates conversionRates(status); UnitConverter converter(source, target, conversionRates, status); - assertEqualsNewNear(UnicodeString("testArea: ") + testCase.source + " to " + testCase.target, - testCase.expectedValue, converter.convert(testCase.inputValue), - 0.0001 * testCase.expectedValue); + assertEqualsNear(UnicodeString("testArea: ") + testCase.source + " to " + testCase.target, + testCase.expectedValue, converter.convert(testCase.inputValue), + 0.0001 * testCase.expectedValue); } } @@ -344,7 +344,7 @@ void unitsTestDataLineFn(void *context, char *fields[][2], int32_t fieldCount, U double got = converter.convert(1000); msg.clear(); msg.append("Converting 1000 ", status).append(x, status).append(" to ", status).append(y, status); - unitsTest->assertEqualsNewNear(msg.data(), expected, got, 0.0001 * expected); + unitsTest->assertEqualsNear(msg.data(), expected, got, 0.0001 * expected); } /** @@ -460,38 +460,14 @@ class ExpectedOutput { } }; -// TODO(Hugo): Add a comment and Use AssertEqualsNear. +// Checks a vector of Measure instances against ExpectedOutput. void checkOutput(UnitsTest *unitsTest, const char *msg, ExpectedOutput expected, const MaybeStackVector &actual, double precision) { IcuTestErrorCode status(*unitsTest, "checkOutput"); - bool success = true; - if (expected._compoundCount != actual.length()) { - success = false; - } - for (int i = 0; i < actual.length(); i++) { - if (i >= expected._compoundCount) { - break; - } - // assertEqualsNear("test conversion", expected._amounts[i], - // actual[i]->getNumber().getDouble(status), 0.0001); - - double diff = std::abs(expected._amounts[i] - actual[i]->getNumber().getDouble(status)); - double diffPercent = expected._amounts[i] != 0 ? diff / expected._amounts[i] : diff; - if (diffPercent > precision) { - success = false; - break; - } - - if (expected._measureUnits[i] != actual[i]->getUnit()) { - success = false; - break; - } - } - - CharString testMessage("test case: ", status); + CharString testMessage("Test case \"", status); testMessage.append(msg, status); - testMessage.append(", expected output: ", status); + testMessage.append("\": expected output: ", status); testMessage.append(expected.toDebugString().c_str(), status); testMessage.append(", obtained output:", status); for (int i = 0; i < actual.length(); i++) { @@ -500,8 +476,19 @@ void checkOutput(UnitsTest *unitsTest, const char *msg, ExpectedOutput expected, testMessage.append(" ", status); testMessage.appendInvariantChars(actual[i]->getUnit().getIdentifier(), status); } - - unitsTest->assertTrue(testMessage.data(), success); + if (!unitsTest->assertEquals(testMessage.data(), expected._compoundCount, actual.length())) { + return; + }; + for (int i = 0; i < actual.length(); i++) { + double permittedDiff = precision * expected._amounts[i]; + if (permittedDiff == 0) { + // If 0 is expected, still permit a small delta. + // TODO: revisit this experimentally chosen value: + permittedDiff = 0.00000001; + } + unitsTest->assertEqualsNear(testMessage.data(), expected._amounts[i], + actual[i]->getNumber().getDouble(status), permittedDiff); + } } /** @@ -581,7 +568,8 @@ void unitPreferencesTestDataLineFn(void *context, char *fields[][2], int32_t fie if (status.errIfFailureAndReset("router.route(inputAmount, ...)")) { return; } - checkOutput(unitsTest, msg.data(), expected, result, 0.0001); + // TODO: revisit this experimentally chosen precision: + checkOutput(unitsTest, msg.data(), expected, result, 0.0000000001); } /** From 086881fa1d31dd774f6300725c88864f903ceba9 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Thu, 18 Jun 2020 01:34:42 +0200 Subject: [PATCH 10/10] A bit more constness. --- icu4c/source/test/intltest/unitstest.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/icu4c/source/test/intltest/unitstest.cpp b/icu4c/source/test/intltest/unitstest.cpp index 0186f87fa28..ba97cd25db2 100644 --- a/icu4c/source/test/intltest/unitstest.cpp +++ b/icu4c/source/test/intltest/unitstest.cpp @@ -66,8 +66,8 @@ void UnitsTest::runIndexedTest(int32_t index, UBool exec, const char *&name, cha void UnitsTest::testConversionCapability() { struct TestCase { - const char *source; - const char *target; + const char *const source; + const char *const target; const UnitsConvertibilityState expectedState; } testCases[]{ {"meter", "foot", CONVERTIBLE}, //