test: Add ParseMoney and ParseScript tests #23185

MarcoFalke commented at 12:16 PM on October 5, 2021: member

Add missing tests

practicalswift commented at 12:26 PM on October 5, 2021: contributor

Concept ACK

DrahtBot added the label Build system on Oct 5, 2021

MarcoFalke removed the label Build system on Oct 5, 2021

MarcoFalke added the label Tests on Oct 5, 2021

shaavan commented at 4:34 PM on October 5, 2021: contributor

Concept ACK

The new tests look good and logically suffices with the code of both the functions.

I would like to propose some additions to the test. I am not yet proficient in writing tests for functions, so I might be wrong. I was understanding the ParseScript function’s code, and I think it would be good to do some additions to the testing of this function

BOOST_CHECK_EQUAL(HexStr(ParseScript("-9")), "59"); To test the line: (w->front() == '-' && w->size() > 1 && std::all_of(w->begin()+1, w->end(), ::IsDigit)))

And,

BOOST_CHECK_EXCEPTION(ParseScript("11111111111111111111111"), std::runtime_error, HasReason("script parse error: decimal numeric value only allowed in the range -0xFFFFFFFF...0xFFFFFFFF")); To test the line: throw std::runtime_error("script parse error: decimal numeric value only allowed in the " "range -0xFFFFFFFF...0xFFFFFFFF");

I hope my suggestion might help you improve upon the PR.

in src/test/util_tests.cpp:1245 in fa326b1c7c outdated

1238 | @@ -1239,6 +1239,11 @@ BOOST_AUTO_TEST_CASE(util_FormatMoney)
1239 |  BOOST_AUTO_TEST_CASE(util_ParseMoney)
1240 |  {
1241 |      BOOST_CHECK_EQUAL(ParseMoney("0.0").value(), 0);
1242 | +    BOOST_CHECK_EQUAL(ParseMoney(".").value(), 0);
1243 | +    BOOST_CHECK_EQUAL(ParseMoney("0.").value(), 0);
1244 | +    BOOST_CHECK_EQUAL(ParseMoney(".0").value(), 0);
1245 | +    BOOST_CHECK_EQUAL(ParseMoney(".6789").value(), 6789'0000);

practicalswift commented at 9:14 AM on October 6, 2021:

    BOOST_CHECK_EQUAL(ParseMoney(".6789").value(), 67'890'000);

MarcoFalke commented at 9:20 AM on October 6, 2021:

I am reading satoshis in groups of 4 :sweat_smile:

jonatack commented at 9:31 AM on October 6, 2021:

I am reading satoshis in groups of 4 :sweat_smile:

Same!

practicalswift commented at 9:46 AM on October 6, 2021:

¯_(ツ)_/¯

practicalswift commented at 9:14 AM on October 6, 2021: contributor

cr ACK fa326b1c7c82cdace70ad7b897e09ac2797a16a6

in src/test/util_tests.cpp:1242 in fa326b1c7c outdated

1238 | @@ -1239,6 +1239,11 @@ BOOST_AUTO_TEST_CASE(util_FormatMoney)
1239 |  BOOST_AUTO_TEST_CASE(util_ParseMoney)
1240 |  {
1241 |      BOOST_CHECK_EQUAL(ParseMoney("0.0").value(), 0);
1242 | +    BOOST_CHECK_EQUAL(ParseMoney(".").value(), 0);

kiminuo commented at 10:02 AM on October 6, 2021:

Maybe it would be worth checking two-dots cases like:

ParseMoney("..")
ParseMoney("0..0")

From whitespace characters, I can see covered spaces but not tabulators (for example; pattern for TrimString is const std::string& pattern = " \f\n\r\t\v"). Maybe it would be good to add tests for this too.

MarcoFalke commented at 11:01 AM on October 6, 2021:

Thanks, done

MarcoFalke force-pushed on Oct 6, 2021

MarcoFalke commented at 11:01 AM on October 6, 2021: member

Force pushed with ideas by @kiminuo and @shaavan. Thanks!

DrahtBot commented at 11:56 AM on October 6, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14752 (tests: Unit tests for IsPayToWitnessScriptHash and IsWitnessProgram by domob1812)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

shaavan commented at 12:57 PM on October 6, 2021: contributor

Updates since my last review:

New test lines have been added to the test of the ParseScript and ParseMoney functions.

The new checks work correctly and compile successfully. I would like to suggest one more addition, which I somehow missed when I reviewed this PR earlier.

In the ParseScript function:

boost::algorithm::split(words, s, boost::algorithm::is_any_of(" \t\n"), boost::algorithm::token_compress_on);

    for (std::vector<std::string>::const_iterator w = words.begin(); w != words.end(); ++w)
    {
        …
    }

That means it can work for multiple integer values given to it. But there is no explicit check for it. Some further checks like: BOOST_CHECK_EQUAL(HexStr(ParseScript("1 2")), "5152"); & BOOST_CHECK_EQUAL(HexStr(ParseScript("1 -9")), "510189");

Should be added.

test: Add ParseMoney and ParseScript tests fa1477e706

MarcoFalke force-pushed on Oct 6, 2021

MarcoFalke commented at 6:04 PM on October 6, 2021: member

Thanks, done

practicalswift commented at 7:43 AM on October 7, 2021: contributor

cr ACK fa1477e706504f45a7a0f51b9f4b8014ee6a7d8d

shaavan approved

shaavan commented at 2:02 PM on October 7, 2021: contributor

tACK fa1477e706504f45a7a0f51b9f4b8014ee6a7d8d

Updates since my last review:

Test for multiple integer input in one string is added to ParseScript tests.

MarcoFalke merged this on Oct 8, 2021

MarcoFalke closed this on Oct 8, 2021

MarcoFalke deleted the branch on Oct 8, 2021

sidhujag referenced this in commit 51fb415617 on Oct 8, 2021

DrahtBot locked this on Oct 30, 2022