tests: Improve 'CAmount' tests #14460

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:20181010-amount-tests changing 1 files +31 −26

hebasto commented at 8:36 PM on October 10, 2018: member
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.

29ed2d64f6

fanquake added the label Tests on Oct 11, 2018
promag commented at 6:39 AM on October 11, 2018: member

explicit using of the CAmount type.

Why? Is there an advantage?
hebasto commented at 6:53 AM on October 11, 2018: member

@promag

Why?

https://github.com/bitcoin/bitcoin/blob/eb7daf4d600eeb631427c018a984a77a34aca66e/src/policy/feerate.h#L37

Is there an advantage?

It clearly shows what type of return value we are expecting from the methods being testing.
promag commented at 10:46 AM on October 11, 2018: member

utACK 29ed2d6.
sipa commented at 8:11 PM on October 12, 2018: member

utACK.

Is the eventual goal to remove the implicit conversion from other types to CAmount? @arvidn perhaps you're interested in this?
jb55 commented at 9:09 PM on October 12, 2018: member

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 commented at 11:42 AM on October 13, 2018: member
@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 https://github.com/bitcoin/bitcoin/blob/fa84723e73d3d7766e7821881ac96ec203e50efc/src/amount.h#L12
laanwj commented at 6:25 PM on October 18, 2018: member

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 commented at 6:29 PM on October 18, 2018: member

utACK 29ed2d64f6d03e242068589ba35805b3fd2ad05a
sipa commented at 2:15 AM on October 20, 2018: member

utACK 29ed2d64f6d03e242068589ba35805b3fd2ad05a
sipa merged this on Oct 20, 2018
sipa closed this on Oct 20, 2018
sipa referenced this in commit 91482e5bf2 on Oct 20, 2018
hebasto deleted the branch on Oct 20, 2018
deadalnix referenced this in commit e584d180ab on Feb 14, 2020
Munkybooty referenced this in commit 5d88f198bf on Jun 24, 2021
PastaPastaPasta referenced this in commit 5a82ed5127 on Jun 27, 2021
PastaPastaPasta referenced this in commit 528a3a96b3 on Jun 28, 2021
Munkybooty referenced this in commit a49c9281a3 on Jun 28, 2021
PastaPastaPasta referenced this in commit c06f85d177 on Jun 29, 2021
Munkybooty referenced this in commit c213315770 on Jun 29, 2021
Munkybooty referenced this in commit b8a46a3c04 on Jun 29, 2021
Munkybooty referenced this in commit 5fd7f0606c on Jun 29, 2021
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels