-
Notifications
You must be signed in to change notification settings - Fork 38.6k
test: Add ParseMoney and ParseScript tests #23185
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.
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
Conversation
|
Concept ACK |
shaavan
left a comment
There was a problem hiding this 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.
|
cr ACK fa326b1c7c82cdace70ad7b897e09ac2797a16a6 |
fa326b1 to
fa58c81
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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
left a comment
There was a problem hiding this 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:
- 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.
fa58c81 to
fa1477e
Compare
|
Thanks, done |
|
cr ACK fa1477e |
shaavan
left a comment
There was a problem hiding this 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:
- Test for multiple integer input in one string is added to ParseScript tests.
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
Add missing tests