test: add ParseUInt8() test coverage #21438

jonatack commented at 1:14 PM on March 14, 2021: member

We have unit test and fuzzer coverage for

ParseInt64()
ParseInt32()
ParseUInt64()
ParseUInt32()

but not ParseUInt8(), so this pull adds it.

I was tempted to add a commit that applies clang formatting to the file, or one that updates the C-style casts to named casts, but resisted the temptation unless reviewers request it.

fanquake added the label Tests on Mar 14, 2021

practicalswift commented at 4:23 PM on March 15, 2021: contributor

cr ACK 04a67aa052f45e68096075d3d8acc83bc634a9a7

Thanks for improving coverage!

in src/test/util_tests.cpp:1521 in 04a67aa052 outdated

1516 | +    BOOST_CHECK(!ParseUInt8("1a", &n));
1517 | +    BOOST_CHECK(!ParseUInt8("aap", &n));
1518 | +    BOOST_CHECK(!ParseUInt8("0x1", &n)); // no hex
1519 | +    BOOST_CHECK(!ParseUInt8("0x1", &n)); // no hex
1520 | +    const char test_bytes[] = {'1', 0, '1'};
1521 | +    const std::string teststr(test_bytes, sizeof(test_bytes));

MarcoFalke commented at 4:30 PM on March 15, 2021:

nit: Could use C++17 string literals

jonatack commented at 6:54 PM on March 15, 2021:

thanks, done

MarcoFalke approved

MarcoFalke commented at 4:31 PM on March 15, 2021: member

ACK

Any reason to not extend the fuzz test as well?

jonatack force-pushed on Mar 15, 2021

jonatack commented at 6:56 PM on March 15, 2021: member

Updated per feedback (thanks!)

diff --git a/src/test/fuzz/parse_numbers.cpp b/src/test/fuzz/parse_numbers.cpp
index ddd2bcfba3..1ad5fb6a05 100644
--- a/src/test/fuzz/parse_numbers.cpp
+++ b/src/test/fuzz/parse_numbers.cpp
@@ -18,6 +18,9 @@ FUZZ_TARGET(parse_numbers)
     double d;
     (void)ParseDouble(random_string, &d);
 
+    uint8_t u8;
+    (void)ParseUInt8(random_string, &u8);
+
     int32_t i32;
     (void)ParseInt32(random_string, &i32);
     (void)atoi(random_string);
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 6447145b22..7d4fcd98b3 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -1517,9 +1517,7 @@ BOOST_AUTO_TEST_CASE(test_ParseUInt8)
     BOOST_CHECK(!ParseUInt8("aap", &n));
     BOOST_CHECK(!ParseUInt8("0x1", &n)); // no hex
     BOOST_CHECK(!ParseUInt8("0x1", &n)); // no hex
-    const char test_bytes[] = {'1', 0, '1'};
-    const std::string teststr(test_bytes, sizeof(test_bytes));
-    BOOST_CHECK(!ParseUInt8(teststr, &n)); // no embedded NULs
+    BOOST_CHECK(!ParseUInt8("1\01"s, &n)); // no embedded NULs
     // Overflow and underflow
     BOOST_CHECK(!ParseUInt8("-255", &n));
     BOOST_CHECK(!ParseUInt8("256", &n));

</p></details>

test: add ParseUInt8() unit and fuzz test coverage 24c6546946

refactor: reuse test string with embedded null char in util_tests 76782e560b

in src/test/util_tests.cpp:1520 in f8b120c5b7 outdated

1515 | +    BOOST_CHECK(!ParseUInt8("1 ", &n));
1516 | +    BOOST_CHECK(!ParseUInt8("1a", &n));
1517 | +    BOOST_CHECK(!ParseUInt8("aap", &n));
1518 | +    BOOST_CHECK(!ParseUInt8("0x1", &n)); // no hex
1519 | +    BOOST_CHECK(!ParseUInt8("0x1", &n)); // no hex
1520 | +    BOOST_CHECK(!ParseUInt8("1\01"s, &n)); // no embedded NULs

MarcoFalke commented at 7:19 PM on March 15, 2021:

\ is greedy and will eat (up to three?) digits that follow it. You'll have to split the string.

E.g.

src/test/util_tests.cpp:    BOOST_CHECK(!ParseMoney("\0" "1"s, ret));

MarcoFalke commented at 7:20 PM on March 15, 2021:

Maybe make it a global constant, so that it can be used in the other test cases as well?

jonatack commented at 10:18 PM on March 15, 2021:

Good point, thanks! Done (I think).

jonatack force-pushed on Mar 15, 2021

jonatack commented at 10:22 PM on March 15, 2021: member

Updated per @MarcoFalke's suggestions (thanks!)

diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 7d4fcd98b3..b1b71ef7c9 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -38,6 +38,7 @@
 #include <boost/test/unit_test.hpp>
 
 using namespace std::literals;
+static const std::string STRING_WITH_EMBEDDED_NULL_CHAR{"1"s "\0" "1"s};
 
 /* defined in logging.cpp */
 namespace BCLog {
@@ -1272,7 +1273,7 @@ BOOST_AUTO_TEST_CASE(util_ParseMoney)
 
     // Parsing strings with embedded NUL characters should fail
     BOOST_CHECK(!ParseMoney("\0-1"s, ret));
-    BOOST_CHECK(!ParseMoney("\0" "1"s, ret));
+    BOOST_CHECK(!ParseMoney(STRING_WITH_EMBEDDED_NULL_CHAR, ret));
     BOOST_CHECK(!ParseMoney("1\0"s, ret));
 }
 
@@ -1450,9 +1451,7 @@ BOOST_AUTO_TEST_CASE(test_ParseInt32)
     BOOST_CHECK(!ParseInt32("aap", &n));
     BOOST_CHECK(!ParseInt32("0x1", &n)); // no hex
     BOOST_CHECK(!ParseInt32("0x1", &n)); // no hex
-    const char test_bytes[] = {'1', 0, '1'};
-    std::string teststr(test_bytes, sizeof(test_bytes));
-    BOOST_CHECK(!ParseInt32(teststr, &n)); // no embedded NULs
+    BOOST_CHECK(!ParseInt32(STRING_WITH_EMBEDDED_NULL_CHAR, &n));
     // Overflow and underflow
     BOOST_CHECK(!ParseInt32("-2147483649", nullptr));
     BOOST_CHECK(!ParseInt32("2147483648", nullptr));
@@ -1480,9 +1479,7 @@ BOOST_AUTO_TEST_CASE(test_ParseInt64)
     BOOST_CHECK(!ParseInt64("1a", &n));
     BOOST_CHECK(!ParseInt64("aap", &n));
     BOOST_CHECK(!ParseInt64("0x1", &n)); // no hex
-    const char test_bytes[] = {'1', 0, '1'};
-    std::string teststr(test_bytes, sizeof(test_bytes));
-    BOOST_CHECK(!ParseInt64(teststr, &n)); // no embedded NULs
+    BOOST_CHECK(!ParseInt64(STRING_WITH_EMBEDDED_NULL_CHAR, &n));
     // Overflow and underflow
     BOOST_CHECK(!ParseInt64("-9223372036854775809", nullptr));
     BOOST_CHECK(!ParseInt64("9223372036854775808", nullptr));
@@ -1517,7 +1514,7 @@ BOOST_AUTO_TEST_CASE(test_ParseUInt8)
     BOOST_CHECK(!ParseUInt8("aap", &n));
     BOOST_CHECK(!ParseUInt8("0x1", &n)); // no hex
     BOOST_CHECK(!ParseUInt8("0x1", &n)); // no hex
-    BOOST_CHECK(!ParseUInt8("1\01"s, &n)); // no embedded NULs
+    BOOST_CHECK(!ParseUInt8(STRING_WITH_EMBEDDED_NULL_CHAR, &n));
     // Overflow and underflow
     BOOST_CHECK(!ParseUInt8("-255", &n));
     BOOST_CHECK(!ParseUInt8("256", &n));
@@ -1555,9 +1552,7 @@ BOOST_AUTO_TEST_CASE(test_ParseUInt32)
     BOOST_CHECK(!ParseUInt32("aap", &n));
     BOOST_CHECK(!ParseUInt32("0x1", &n)); // no hex
     BOOST_CHECK(!ParseUInt32("0x1", &n)); // no hex
-    const char test_bytes[] = {'1', 0, '1'};
-    std::string teststr(test_bytes, sizeof(test_bytes));
-    BOOST_CHECK(!ParseUInt32(teststr, &n)); // no embedded NULs
+    BOOST_CHECK(!ParseUInt32(STRING_WITH_EMBEDDED_NULL_CHAR, &n));
     // Overflow and underflow
     BOOST_CHECK(!ParseUInt32("-2147483648", &n));
     BOOST_CHECK(!ParseUInt32("4294967296", &n));
@@ -1586,9 +1581,7 @@ BOOST_AUTO_TEST_CASE(test_ParseUInt64)
     BOOST_CHECK(!ParseUInt64("1a", &n));
     BOOST_CHECK(!ParseUInt64("aap", &n));
     BOOST_CHECK(!ParseUInt64("0x1", &n)); // no hex
-    const char test_bytes[] = {'1', 0, '1'};
-    std::string teststr(test_bytes, sizeof(test_bytes));
-    BOOST_CHECK(!ParseUInt64(teststr, &n)); // no embedded NULs
+    BOOST_CHECK(!ParseUInt64(STRING_WITH_EMBEDDED_NULL_CHAR, &n));
     // Overflow and underflow
     BOOST_CHECK(!ParseUInt64("-9223372036854775809", nullptr));
     BOOST_CHECK(!ParseUInt64("18446744073709551616", nullptr));
@@ -1618,9 +1611,7 @@ BOOST_AUTO_TEST_CASE(test_ParseDouble)
     BOOST_CHECK(!ParseDouble("1a", &n));
     BOOST_CHECK(!ParseDouble("aap", &n));
     BOOST_CHECK(!ParseDouble("0x1", &n)); // no hex
-    const char test_bytes[] = {'1', 0, '1'};
-    std::string teststr(test_bytes, sizeof(test_bytes));
-    BOOST_CHECK(!ParseDouble(teststr, &n)); // no embedded NULs
+    BOOST_CHECK(!ParseDouble(STRING_WITH_EMBEDDED_NULL_CHAR, &n));
     // Overflow and underflow
     BOOST_CHECK(!ParseDouble("-1e10000", nullptr));
     BOOST_CHECK(!ParseDouble("1e10000", nullptr));

</p></details>

laanwj commented at 7:16 AM on March 16, 2021: member

Thanks for adding test coverage. ParseUInt8 was introduced in 102867c587f5f7954232fb8ed8e85cda78bb4d32 without adding tests. Code review ACK 76782e560b87dc87296144aeb4ee85eb3763b8e9

MarcoFalke commented at 7:49 AM on March 16, 2021: member

cr ACK 76782e560b87dc87296144aeb4ee85eb3763b8e9

laanwj merged this on Mar 16, 2021

laanwj closed this on Mar 16, 2021

jonatack deleted the branch on Mar 16, 2021

sidhujag referenced this in commit 9b5eefaa92 on Mar 16, 2021

Fabcien referenced this in commit 1d2c8c42a3 on Feb 21, 2022

DrahtBot locked this on Aug 18, 2022