Skip to content
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

tests: Improve 'CAmount' tests #14460

Merged
merged 1 commit into from Oct 20, 2018

Conversation

Projects
None yet
6 participants
@hebasto
Copy link
Member

commented Oct 10, 2018

This provides:

  • more MoneyRange tests;
  • new CFeeRate constructor tests with zero byte size;
  • explicit using of the CAmount type.
Improve CAmount tests
This provides:
  - more `MoneyRange` tests;
  - new `CFeeRate` constructor tests with zero byte size;
  - explicit using of the `CAmount` type.

@fanquake fanquake added the Tests label Oct 11, 2018

@promag

This comment has been minimized.

Copy link
Member

commented Oct 11, 2018

explicit using of the CAmount type.

Why? Is there an advantage?

@hebasto

This comment has been minimized.

Copy link
Member Author

commented Oct 11, 2018

@promag

Why?

CAmount GetFee(size_t nBytes) const;

Is there an advantage?

It clearly shows what type of return value we are expecting from the methods being testing.

@promag

This comment has been minimized.

Copy link
Member

commented Oct 11, 2018

utACK 29ed2d6.

@sipa

This comment has been minimized.

Copy link
Member

commented Oct 12, 2018

utACK.

Is the eventual goal to remove the implicit conversion from other types to CAmount?

@arvidn perhaps you're interested in this?

@jb55

This comment has been minimized.

Copy link
Contributor

commented Oct 12, 2018

Is the eventual goal to remove the implicit conversion from other types to CAmount

I hope so, right now this would remove testing of implicit conversions... is that a good thing?

@hebasto

This comment has been minimized.

Copy link
Member Author

commented Oct 13, 2018

@sipa @jb55
Thank you for your reviews.

Is it worth to just add an explicit test case for type conversions like this:

BOOST_AUTO_TEST_CASE(CAmountTypeConversionTest)
{
    BOOST_CHECK_EQUAL(CAmount(-MAX_MONEY), -MAX_MONEY);
    BOOST_CHECK_EQUAL(CAmount(-1), -1);
    BOOST_CHECK_EQUAL(CAmount(0), 0);
    BOOST_CHECK_EQUAL(CAmount(1), 1);
    BOOST_CHECK_EQUAL(CAmount(MAX_MONEY), MAX_MONEY);
}

or just keep in mind that

typedef int64_t CAmount;

@laanwj

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

Is it worth to just add an explicit test case for type conversions like this:

I don't think so—because at the same time that would be worth testing (e.g. CAmount becomes an actual type instead of typedef), implicit conversion will be no longer possible

@laanwj

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

utACK 29ed2d6

1 similar comment
@sipa

This comment has been minimized.

Copy link
Member

commented Oct 20, 2018

utACK 29ed2d6

@sipa sipa merged commit 29ed2d6 into bitcoin:master Oct 20, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

sipa added a commit that referenced this pull request Oct 20, 2018

Merge #14460: tests: Improve 'CAmount' tests
29ed2d6 Improve CAmount tests (Hennadii Stepanov)

Pull request description:

  This provides:
    - more `MoneyRange` tests;
    - new `CFeeRate` constructor tests with zero byte size;
    - explicit using of the `CAmount` type.

Tree-SHA512: ca0ad6ccb37909a2a5c11034dc07b316a84c32fb40c6f8b6cfc28ebec72a1de157f31d22e767ae80d70ed06d7296f23870cc5ed0689f34a754ae763d50e23d43

@hebasto hebasto deleted the hebasto:20181010-amount-tests branch Oct 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.