Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Oct 5, 2021

Add missing tests

@practicalswift
Copy link
Contributor

Concept ACK

Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

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.

@practicalswift
Copy link
Contributor

cr ACK fa326b1c7c82cdace70ad7b897e09ac2797a16a6

@maflcko
Copy link
Member Author

maflcko commented Oct 6, 2021

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

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 6, 2021

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.

Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

Updates since my last review:

  1. 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.

@maflcko
Copy link
Member Author

maflcko commented Oct 6, 2021

Thanks, done

@practicalswift
Copy link
Contributor

cr ACK fa1477e

Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

tACK fa1477e

Updates since my last review:

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

@maflcko maflcko merged commit 9275869 into bitcoin:master Oct 8, 2021
@maflcko maflcko deleted the 2110-ToIntegral branch October 8, 2021 12:05
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 8, 2021
fa1477e test: Add ParseMoney and ParseScript tests (MarcoFalke)

Pull request description:

  Add missing tests

ACKs for top commit:
  practicalswift:
    cr ACK fa1477e
  shaavan:
    tACK fa1477e

Tree-SHA512: e57b4e8da4abe075b4ad7e7abd68c4d0eecf0c805acd2c72076aac4993d3ec5748fd02b721c4c110494db56fdbc199301e5cfd1dc0212f2002f355b47f70e539
@bitcoin bitcoin locked and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants