-
Move random test utilities from
setup_common
to a newrandom
file, as many tests don’t use this code. -
Create a helper to generate semi-random CAmounts up to
MONEY_RANGE
rather than only uint32, and use the helper in the unit tests. -
De-duplicate a shared
add_coin
method by extracting it to acoins
test utility.
test: create random and coins utils, add amount helper, dedupe add_coin #26940
pull jonatack wants to merge 3 commits into bitcoin:master from jonatack:unit-tests-CAmount-generation changing 39 files +139 −43-
jonatack commented at 0:07 am on January 21, 2023: contributor
-
DrahtBot commented at 0:07 am on January 21, 2023: contributor
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviews
See the guideline for information on the review process.
Type Reviewers ACK john-moffett, pinheadmz, achow101 If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Conflicts
Reviewers, this pull request conflicts with the following ones:
- #26817 (doc: Bump copyright years to present (headers only) by MarcoFalke)
- #25325 (Add pool based memory resource by martinus)
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.
-
DrahtBot added the label Tests on Jan 21, 2023
-
jonatack force-pushed on Jan 21, 2023
-
jonatack marked this as ready for review on Jan 21, 2023
-
in src/test/util/setup_common.cpp:102 in 3cd46094d1 outdated
97+{ 98+ switch (InsecureRandRange(100)) { 99+ case 0: return 0; 100+ case 1: return 1; 101+ case 2: return MAX_MONEY; 102+ default: return InsecureRandRange(InsecureRandBool() ? COIN : MAX_MONEY);
john-moffett commented at 2:19 pm on January 22, 2023:Can you explain the motivation for restricting the range toCOIN
half the time? Is it just because they’re more common values?
jonatack commented at 7:35 pm on January 22, 2023:Thanks for looking! Yes, the idea was to often generate a real-life value, while still testing the fullMoneyRange
insrc/consensus/amount.h
.
jonatack commented at 7:43 pm on January 22, 2023:Updated its Doxygen doc with that info.john-moffett approvedjohn-moffett commented at 2:20 pm on January 22, 2023: contributorutACK 3cd46094d159373bda7efbaaf8c28bb78397947f
I like the idea of having more intelligent “random” data that includes edge cases a significant portion of the time.
in src/test/util/setup_common.cpp:96 in 3cd46094d1 outdated
92@@ -91,6 +93,16 @@ void Seed(FastRandomContext& ctx) 93 ctx = FastRandomContext(seed); 94 } 95 96+CAmount SemiRandMoneyAmount()
fanquake commented at 2:24 pm on January 22, 2023:No thoughts on the change, but not sure this code should be insetup_common
. Probablysrc/test/util/something
?
jonatack commented at 7:41 pm on January 22, 2023:Done. Didn’t see a good existing home for it, so I moved the random generation test code fromsetup_common
tosrc/test/util/random
, which seems beneficial as many of the unit tests don’t call it, and then added the test helper there.jonatack renamed this:
test: add and use a helper for generating semi-random CAmounts
test: Move rand utils from setup_common to random and add a helper
on Jan 22, 2023jonatack force-pushed on Jan 22, 2023jonatack force-pushed on Jan 22, 2023jonatack force-pushed on Jan 22, 2023jonatack commented at 7:48 pm on January 22, 2023: contributorUpdated with feedback from @john-moffett and @fanquake (thanks!)jonatack force-pushed on Jan 22, 2023jonatack force-pushed on Jan 22, 2023in src/Makefile.test_util.include:1 in 4c3d011a04 outdated
0@@ -1,4 +1,4 @@ 1-# Copyright (c) 2013-2019 The Bitcoin Core developers 2+# Copyright (c) 2013-2023 The Bitcoin Core developers
fanquake commented at 11:06 am on January 23, 2023:There’s no need to manually bump dates, and they’ll likely just be removed (#26817), so you can drop any changes like this.
jonatack commented at 5:28 pm on January 23, 2023:Done.jonatack force-pushed on Jan 23, 2023in src/test/util/random.h:40 in 866917c2f7 outdated
39+ 40+/** 41+ * Generate a semi-random CAmount up to COIN (a typical value) or MAX_MONEY (the 42+ * full money range), with occasional edge case values of 0, 1, and MAX_MONEY. 43+ */ 44+static inline CAmount SemiRandMoneyAmount()
maflcko commented at 10:38 am on January 31, 2023:Not sure if we should imitate a fuzz engine in the unit tests to pick special values. A fuzz engine is generally a lot better at finding those.
So I’d say, if you see value in testing more edge cases, I’d recommend to write a fuzz test, not a unit test with a hand written “edge-case finder”.
jonatack commented at 5:35 pm on February 1, 2023:I thought about that, but unlike probing for a crash as fuzzers do, our unit tests are using the rand generators to check returned values, i.e. non-crash behavior, from those inputs and this change follows and improves on their intention. (Sure, adding a fuzz test would be good and provide complementary coverage.)
We could simplify the helper to just
InsecureRandRange(MAX_MONEY)
if reviewers prefer, but I think the proposed version provides more robust and useful coverage.
maflcko commented at 8:38 am on February 2, 2023:unlike probing for a crash as fuzzers do, our unit tests are using the rand generators to check returned values
Conceptually unit tests and fuzz tests are no different, if you take away the fuzz engine. A fuzz test doing a check is no different than a unit test doing a check. There may be an implementation detail in the exit code (and other stuff) if one calls assert vs BOOST_REQUIRE or BOOST_CHECK, but in both cases the test exe will end unsuccessfully.
I think the proposed version provides more robust and useful coverage.
Would be good to provide an example where this leads to increased line or branch coverage
jonatack commented at 5:21 pm on February 6, 2023:We could simplify the helper to just
InsecureRandRange(MAX_MONEY)
if reviewers preferHappy to update with a concrete suggestion within the scope here of improving the existing unit tests.
fanquake commented at 10:38 am on February 7, 2023:Would be good to provide an example where this leads to increased line or branch coverage
I think this is the key point. If we’re going to write special randomness/edge case functions, we should be able to show they are improving test coverage. Otherwise have a fuzzer pick random values.
jonatack commented at 4:54 pm on February 7, 2023:Removed the edge cases. It uses the money range rather than only uint32, while remaining in the same vein as the existing unit test rand helpers.maflcko commented at 10:38 am on January 31, 2023: memberNo objection, but not seeing the benefit over a fuzz test either?DrahtBot added the label Needs rebase on Feb 1, 2023jonatack renamed this:
test: Move rand utils from setup_common to random and add a helper
test: create random and coins utils, add amount helper, dedupe add_coin
on Feb 1, 2023jonatack force-pushed on Feb 1, 2023jonatack force-pushed on Feb 1, 2023DrahtBot removed the label Needs rebase on Feb 1, 2023jonatack commented at 6:15 pm on February 1, 2023: contributorRebased, squashed the money helper changes to one commit, and de-duplicated a sharedadd_coin
method by extracting it to a test utility.jonatack force-pushed on Feb 1, 2023in src/test/util/blockfilter.cpp:31 in d34bc34c8c outdated
27@@ -28,4 +28,3 @@ bool ComputeFilter(BlockFilterType filter_type, const CBlockIndex* block_index, 28 filter = BlockFilter(filter_type, block, block_undo); 29 return true; 30 } 31-
pinheadmz commented at 4:16 pm on February 6, 2023:Why remove the trailing newline?
jonatack commented at 5:10 pm on February 6, 2023:We don’t normally have extra trailing newlines in files.pinheadmz commented at 5:07 pm on February 6, 2023: memberconcept ACK tested and reviewed, I like the deliberate introduction of edge cases into the tests and certainly makes sense to refactor outAddTestCoin()
since it is duplicated. I’m not entirely sure about the move of all the random functions is a net benefit, you had to include that in 33 files when those functions were already sitting nicely insetup_common
pinheadmz commented at 5:19 pm on February 6, 2023: memberACK d34bc34c8c69781c913faa3e572af8030d099551
The location of the random functions isn’t a blocker for me.
0-----BEGIN PGP SIGNED MESSAGE----- 1Hash: SHA256 2 3ACK d34bc34c8c69781c913faa3e572af8030d099551 4-----BEGIN PGP SIGNATURE----- 5 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmPhNmYACgkQ5+KYS2KJ 7yTqGYA//cUrRxVOTho65ZbiTMKbev32/NMPI4aTt+j4Jl3UtSt96kqlBgSYdGftC 81V4B6pj2efxjFEuGQd8n1pzUt5pm8bkdg1r6JfVjEFNaz/ZqzagJS/93RU4/+3Ui 9ffYt1jLI55ws2iy/owStLNmWqyRmh2YazTRPb+h1YAQ8FsuyIlpz75HJnKORsoA4 10gcZwJ84YxSPaYYyCLpQ/++0PcofcR7q2CFidnZ8Dlcc3LwNSbdEk7zOwSkdAbT51 11KIjIWH8Zh0LAgMbWCyXnmf1er3IdnCW/DSczQRqqbr+Mpn+0iQvyKsfqS+wJMOFj 12Yjjg7aZ8NBx4nCw9mK+Ey3/XO1oAqcNVjRJinTKW7I6WhxShBY2vr9sF9ttX4nSS 137mEQf6iYRkCPCkRDrgTCtdNMzXtVQQRwZm+vN9w5Rs7AHz7SBJFDCpxC2hnMa1ij 14FkowcK/FW1/LYYTceJmvwwbP5UuM5JN6z8j4DE6m/wztkiAOM0JSBe6IushlPplT 15xeByjBkj3HEUoRStd4bZhZindIDplqHnzBEbqMuJFY8rLXrN2Vr6pP4esuUHlmh4 16Nnm7In2py8RABm5sjeMugb5ElZR+zyDwnUElfqhwQf7iVKTqBfV+Jw05dH0Xmh9l 17dM/qOScbDnDlg1IaIkHvFTkJSTirdb5pwalG5dg1i+rjsYeDoxw= 18=2W5E 19-----END PGP SIGNATURE-----
pinheadmz’s public key is on keybase
DrahtBot added the label Needs rebase on Feb 6, 2023Move random test util code from setup_common to random
as many of the unit tests don't use this code
jonatack force-pushed on Feb 6, 2023jonatack commented at 9:21 pm on February 6, 2023: contributorTrivial rebase per
git range-diff 52ddbd5 d34bc34 e6de5ad
for the merge of #26345. Thank you for the ACKs, @john-moffett and @pinheadmz, mind re-ACKing?I don’t have a strong opinion on extracting the random utils to a helper. As most of the test files don’t need them to be a part of their compilation unit, it might be beneficial to speed up builds a bit, and it provides clearer code organization. OTOH it’s more to review so am happy also to leave them in
setup_common
or extract only the new helper.DrahtBot removed the label Needs rebase on Feb 6, 2023pinheadmz commented at 3:53 pm on February 7, 2023: memberACK e6de5ad8dce4fea053f75de5ee0189ed5f91df41
verified rebase change is minimal, also double checked that all
add_coin()
calls were updated in the unit tests0-----BEGIN PGP SIGNED MESSAGE----- 1Hash: SHA256 2 3ACK e6de5ad8dce4fea053f75de5ee0189ed5f91df41 4-----BEGIN PGP SIGNATURE----- 5 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmPic+UACgkQ5+KYS2KJ 7yToqeA//Uf61CfE9JSLcqfWT2IOu2J7AsFKY09zsXG+TpQQz1NiVOUvKPkQC0RYA 8MdPnvrXTodlsYxf1c1K9xbAkmeQ356ptlcl/sDi5w5y2eQ82Ul5kFIrfEpPMB7M8 9qfkcIdRsz3Ctv9ps0wbfeXSeW0rw2LbokQ8qhChtyzJAeUbyTDkjvjHq9vEMNs8G 10QiGRFDEZzwxj+VkmsKOhYfJvOKUXXZJDGTzbUaATuLdXkbB17pE06n4nOAGUqbJa 11rFbSC8EZVdwBp4Ryh3jtF7+FXlDizjh1Wc+Mushx1/LvBA2vRkz4U39e97VIRTEW 12wHPvLkSyCtUX6HxuOCC2hGQY0mMfrxrtMTgUUQvTwIdu8S7JEHeEmQ2GhAGJ1T67 13Bsnih3FMpHN7VI6ugek1Egdu08CXO5ybU6hyJNgaRTsIC29XlEmYVBGqj5XuyHsO 14TYLStNA9A2cgHr/HfTrR/FB7RwaFqA47uPBvOapwmdV0dK7a1tCfJHrQxTO+SNkF 15Gdg/gBE+yYqjEgA9uqa9t6rHPrQnFtdjG3Bp0AVFJ8WSh3l8P/ZJjmtGvYEFd7xt 16LP6EEGPnt/vfmXnmViqDSmASRqnIa7sDdWcI92CG9Ds3+77SeHpFzRfPBH0zN3bi 17X/zMr8XjCxbw8hswNUxSve4yc0KW5Nu2jTiv9zfJTSfoLYdDhC8= 18=3Nhu 19-----END PGP SIGNATURE-----
pinheadmz’s public key is on keybase
maflcko commented at 4:19 pm on February 7, 2023: memberI still don’t understand why this change is being made. #26940 (review)
It claims to add coverage, without providing any evidence or even intuition.
I am thinking that, absent that, it should be assumed that it will remove coverage. For example, sanitizers, such as the UB signed integer overflow sanitizer require large enough values to detect an overflow. If a majority of the values are now
0
and1
, where before they weren’t, it’ll have a harder time to trigger an overflow and sanitize the code.jonatack force-pushed on Feb 7, 2023jonatack commented at 5:02 pm on February 7, 2023: contributorIf a majority of the values are now
0
and1
Only 1% was 0 and 1% was 1. I liked that it would check edge cases often enough to provide feedback when running
make check
that our current tests may miss, especially for developers running the unit tests locally to verify they are green, say, before pushing an update.Anyway, that’s gone now; removed the edge cases in the last push. It uses the money range rather than only uint32, while remaining in the same vein as the existing unit test rand helpers, and updated the PR description.
Create InsecureRandMoneyAmount() test util helper
to generate semi-random CAmounts up to MAX_MONEY rather than only uint32, and use it in the unit tests.
De-duplicate add_coin methods to a test util helper 4275195606in src/test/util/random.h:40 in 25ea24d0a2 outdated
35+static inline bool InsecureRandBool() 36+{ 37+ return g_insecure_rand_ctx.randbool(); 38+} 39+ 40+static inline CAmount SemiRandMoneyAmount()
john-moffett commented at 8:36 pm on February 7, 2023:If this is now just returning an amount in the uniform[0 .. MAX_MONEY)
range, maybe rename toInsecureRandMoneyAmount()
? Also, bit of a nit, but maybe have it returnInsecureRandRange(MAX_MONEY + CAmount(1))
, sinceMAX_MONEY
is a validCAmount
?
jonatack commented at 11:09 pm on February 9, 2023:Done.jonatack force-pushed on Feb 9, 2023john-moffett approvedjohn-moffett commented at 0:44 am on February 10, 2023: contributorACK 4275195606e6f42466d9a8ef766b3035833df4d5DrahtBot requested review from pinheadmz on Feb 10, 2023pinheadmz commented at 3:19 pm on February 10, 2023: memberACK 4275195606e6f42466d9a8ef766b3035833df4d5
0-----BEGIN PGP SIGNED MESSAGE----- 1Hash: SHA256 2 3ACK 4275195606e6f42466d9a8ef766b3035833df4d5 4-----BEGIN PGP SIGNATURE----- 5 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmPmX4QACgkQ5+KYS2KJ 7yTp9Qg/8DS+6sPz95i2whVHQhnphYYAbbF68udZnle8PqeKof7lWGHOnCF3our+/ 8m2oOGfwXdviYMtIJzB+qBpwRHWonJStIl90Tz6yWqwnM2H5djWhpRBKUsjXMSPsI 9vpFJCL2eQNSohNcBJ3aG2rZ3SIrMRZaFBiC1cKKan5XYbZBistXFg5H7odmTTwa/ 10j70qkh3dK+NINELUC5Q+exVSPK9LSwH8tmfIKjd0mb0RL7ABXISEVEQSjkdIgOG9 11tNFc9o3Nlcseyha0V7bzkDEL0cSPoxInbxlUHv/o8+6+XnPL3cn2hGrFy424OMg6 12IvZeiQacQbaHzDjDNI9bcK4nyunLqPz1G3r53ibs2CU507Mfv/TNIoToFyJzg/HF 13kgb9qRhg/KwqaZ/2bT4OatAOja+t8lnFMvVL0NB0Uhmudv1kbcGERqIQWc8/0dru 1417vdbiu3YqAstDMhtndvy/hupEuBEqEc97xNmXu9xmB8TP9FKDpRS2qWWpvbQNna 157gh49R+eBu/0kxeMhIrc2oXL1S3oaVH1kNqYwGT6cM7Qvt1nVJgGohm2JCE9PuPf 16Md+VcMm/EhQwUjl7Ja58Sv3UdkWMmQKFj/DhVowJHht4tfDPtl6hR6iBitnB3/69 17aoh/KBaiey/cSOXVv7zX5TQv21452+fdDuZPjFeGPct+vHxf/II= 18=r6pu 19-----END PGP SIGNATURE-----
pinheadmz’s public key is on keybase
DrahtBot removed review request from pinheadmz on Feb 10, 2023achow101 commented at 10:19 pm on February 17, 2023: memberACK 4275195606e6f42466d9a8ef766b3035833df4d5achow101 merged this on Feb 17, 2023achow101 closed this on Feb 17, 2023
jonatack deleted the branch on Feb 17, 2023sidhujag referenced this in commit f40ed0e9e6 on Feb 25, 2023glozow commented at 10:29 am on April 5, 2023: memberWhat was the rationale for 81f5ade2a324167c03c5ce765a26bd42ed652723? test/util/random.h depends on test/util/setup_common because it’s usingg_insecure_rand_ctx
, which means setup_common can’t use any of the random functions without creating a circular dependency. Was this intentional / am I missing something? Should we be using a different set of random functions within setup_common?jonatack commented at 2:40 pm on April 5, 2023: contributor@glozow Thanks for bringing it up! The rationale was in this thread: #26940 (review). I noticed this as well, and thatsrc/test/util/chainstate.h
has the same dependency, and made a patch to remove those. Let me see if I can dig it up.fanquake referenced this in commit 24d5cf9314 on Jul 19, 2023bitcoin locked this on Apr 4, 2024
github-metadata-mirror
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-22 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me