test: create random and coins utils, add amount helper, dedupe add_coin #26940

jonatack · 2023-01-21T00:07:02Z

Move random test utilities from setup_common to a new random 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 a coins test utility.

DrahtBot · 2023-01-21T00:07:04Z

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.

john-moffett

utACK 3cd4609

I like the idea of having more intelligent "random" data that includes edge cases a significant portion of the time.

src/test/util/setup_common.cpp

jonatack · 2023-01-22T19:48:42Z

Updated with feedback from @john-moffett and @fanquake (thanks!)

src/Makefile.test_util.include

maflcko

No objection, but not seeing the benefit over a fuzz test either?

src/test/util/random.h

jonatack · 2023-02-01T18:15:12Z

Rebased, squashed the money helper changes to one commit, and de-duplicated a shared add_coin method by extracting it to a test utility.

src/test/util/blockfilter.cpp

pinheadmz · 2023-02-06T17:07:55Z

concept ACK
tested and reviewed, I like the deliberate introduction of edge cases into the tests and certainly makes sense to refactor out AddTestCoin() 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 in setup_common

pinheadmz · 2023-02-06T17:19:45Z

ACK d34bc34

The location of the random functions isn't a blocker for me.

Show Signature

pinheadmz's public key is on keybase

as many of the unit tests don't use this code

jonatack · 2023-02-06T21:21:06Z

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

pinheadmz · 2023-02-07T15:53:59Z

ACK e6de5ad

verified rebase change is minimal, also double checked that all add_coin() calls were updated in the unit tests

Show Signature

pinheadmz's public key is on keybase

maflcko · 2023-02-07T16:19:03Z

I still don't understand why this change is being made. #26940 (comment)

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 and 1, where before they weren't, it'll have a harder time to trigger an overflow and sanitize the code.

jonatack · 2023-02-07T17:02:12Z

If a majority of the values are now 0 and 1

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.

src/test/util/random.h

to generate semi-random CAmounts up to MAX_MONEY rather than only uint32, and use it in the unit tests.

john-moffett

ACK 4275195

pinheadmz · 2023-02-10T15:19:23Z

ACK 4275195

Show Signature

pinheadmz's public key is on keybase

achow101 · 2023-02-17T22:19:02Z

ACK 4275195

…helper,…

glozow · 2023-04-05T10:29:50Z

What was the rationale for 81f5ade? test/util/random.h depends on test/util/setup_common because it's using g_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 · 2023-04-05T14:40:45Z

@glozow Thanks for bringing it up! The rationale was in this thread: #26940 (comment). I noticed this as well, and that src/test/util/chainstate.h has the same dependency, and made a patch to remove those. Let me see if I can dig it up.

jonatack · 2023-04-05T20:58:44Z

setup_common can't use any of the random functions without creating a circular dependency

Addressed with #27425.

…/setup_common to util/random 1cd45d4 test: move random.h include header from setup_common.h to cpp (Jon Atack) 1b246fd test: move remaining random test util code from setup_common to random (jonatack) Pull request description: and drop the `util/random` dependency on `util/setup_common`. This improves code separation and allows `util/setup_common` to call `util/random` functions without creating a circular dependency, thereby addressing bitcoin/bitcoin#26940 (comment) by glozow (thanks!) ACKs for top commit: MarcoFalke: lgtm ACK 1cd45d4 🌂 Tree-SHA512: 6ce63d9103ba9b04eebbd8ad02fe9aa79e356296533404034a1ae88e9b7ca0bc9a5c51fd754b71cf4e7b55b18bcd4d5474b2d588edee3851e3b3ce0e4d309a93

DrahtBot added the Tests label Jan 21, 2023

jonatack force-pushed the unit-tests-CAmount-generation branch from 43c7ff8 to 3cd4609 Compare January 21, 2023 01:01

jonatack marked this pull request as ready for review January 21, 2023 02:06

DrahtBot mentioned this pull request Jan 21, 2023

Add fee_est tool for debugging fee estimation code #10443

Closed

john-moffett approved these changes Jan 22, 2023

View reviewed changes

src/test/util/setup_common.cpp Outdated Show resolved Hide resolved

fanquake reviewed Jan 22, 2023

View reviewed changes

src/test/util/setup_common.cpp Outdated Show resolved Hide resolved

jonatack changed the title ~~test: add and use a helper for generating semi-random CAmounts~~ test: Move rand utils from setup_common to random and add a helper Jan 22, 2023

jonatack force-pushed the unit-tests-CAmount-generation branch 3 times, most recently from 884a5f8 to 4e5bf23 Compare January 22, 2023 19:46

jonatack force-pushed the unit-tests-CAmount-generation branch 2 times, most recently from bc24055 to 4c3d011 Compare January 22, 2023 22:32

DrahtBot mentioned this pull request Jan 23, 2023

test, build: Separate read_json function into its own module #25974

Merged

fanquake reviewed Jan 23, 2023

View reviewed changes

src/Makefile.test_util.include Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Jan 23, 2023

Add pool based memory resource #25325

Merged

jonatack force-pushed the unit-tests-CAmount-generation branch from 4c3d011 to 866917c Compare January 23, 2023 17:26

DrahtBot mentioned this pull request Jan 24, 2023

doc: Bump copyright years to present (headers only) #26817

Closed

maflcko reviewed Jan 31, 2023

View reviewed changes

src/test/util/random.h Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Feb 1, 2023

jonatack changed the title ~~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 Feb 1, 2023

jonatack force-pushed the unit-tests-CAmount-generation branch 2 times, most recently from 632a581 to 0196907 Compare February 1, 2023 18:05

DrahtBot removed the Needs rebase label Feb 1, 2023

jonatack force-pushed the unit-tests-CAmount-generation branch from 0196907 to d34bc34 Compare February 1, 2023 18:29

DrahtBot mentioned this pull request Feb 2, 2023

refactor: modernize the implementation of uint256.* #26345

Merged

pinheadmz reviewed Feb 6, 2023

View reviewed changes

src/test/util/blockfilter.cpp Show resolved Hide resolved

DrahtBot added the Needs rebase label Feb 6, 2023

Move random test util code from setup_common to random

81f5ade

as many of the unit tests don't use this code

jonatack force-pushed the unit-tests-CAmount-generation branch from d34bc34 to e6de5ad Compare February 6, 2023 20:44

DrahtBot removed the Needs rebase label Feb 6, 2023

jonatack force-pushed the unit-tests-CAmount-generation branch from e6de5ad to 25ea24d Compare February 7, 2023 16:50

john-moffett reviewed Feb 7, 2023

View reviewed changes

src/test/util/random.h Outdated Show resolved Hide resolved

jonatack added 2 commits February 9, 2023 15:03

Create InsecureRandMoneyAmount() test util helper

9d92c3d

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

4275195

jonatack force-pushed the unit-tests-CAmount-generation branch from 25ea24d to 4275195 Compare February 9, 2023 23:08

john-moffett approved these changes Feb 10, 2023

View reviewed changes

DrahtBot requested a review from pinheadmz February 10, 2023 11:59

DrahtBot removed the request for review from pinheadmz February 10, 2023 15:19

maflcko mentioned this pull request Feb 10, 2023

review request removed? maflcko/DrahtBot#28

Closed

achow101 merged commit a245429 into bitcoin:master Feb 17, 2023

jonatack deleted the unit-tests-CAmount-generation branch February 17, 2023 22:28

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 25, 2023

Merge bitcoin#26940: test: create random and coins utils, add amount …

f40ed0e

…helper,…

jonatack mentioned this pull request Apr 5, 2023

test: move remaining rand code from util/setup_common to util/random #27425

Merged

bitcoin locked and limited conversation to collaborators Apr 4, 2024

test: create random and coins utils, add amount helper, dedupe add_coin #26940

test: create random and coins utils, add amount helper, dedupe add_coin #26940

Uh oh!

Conversation

jonatack commented Jan 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jan 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

john-moffett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jonatack commented Jan 22, 2023

Uh oh!

Uh oh!

maflcko left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonatack commented Feb 1, 2023

Uh oh!

Uh oh!

pinheadmz commented Feb 6, 2023

Uh oh!

pinheadmz commented Feb 6, 2023

Uh oh!

jonatack commented Feb 6, 2023

Uh oh!

pinheadmz commented Feb 7, 2023

Uh oh!

maflcko commented Feb 7, 2023

Uh oh!

jonatack commented Feb 7, 2023

Uh oh!

Uh oh!

john-moffett left a comment

Choose a reason for hiding this comment

Uh oh!

pinheadmz commented Feb 10, 2023

Uh oh!

achow101 commented Feb 17, 2023

Uh oh!

glozow commented Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Apr 5, 2023

Uh oh!

jonatack commented Apr 5, 2023

Uh oh!

Uh oh!

jonatack commented Jan 21, 2023 •

edited

Loading

DrahtBot commented Jan 21, 2023 •

edited

Loading

maflcko left a comment •

edited

Loading

glozow commented Apr 5, 2023 •

edited

Loading