test: Parse command line arguments from unit and fuzz tests, make addrman consistency check ratio easier to change #23373

vasild · 2021-10-27T12:18:56Z

Previously command line arguments passed to unit and fuzz tests would be ignored by the tests themselves. They would be used by the boost test framework (e.g. --run_test="addrman_tests/*") or by the fuzzer (e.g. -runs=1). However both provide ways to pass down the extra arguments to the test itself. Use that, parse the arguments and make them available to the tests via gArgs.

This makes the tests more flexible as they can be run with any bitcoind config option specified on the command line.

When creating AddrMan objects in tests, use -checkaddrman= (if provided) instead of hardcoding the check ratio in many different places. See #20233 (comment) for further motivation for this.

DrahtBot · 2021-10-27T14:33:20Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23545 (scripted-diff: Use clang-tidy syntax for C++ named arguments by MarcoFalke)
#22910 ([RFC] Encapsulate asmap in NetGroupManager by jnewbery)
#21878 (Make all networking code mockable by vasild)

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.

jonatack

Initial lightly tested code review ACK, was distracted by addrman unit test failures I was seeing that turned out to be already on master (and spurious, not sure yet what happened); will come back to finish reviewing here.

src/test/fuzz/fuzz.cpp

mzumsande

Concept ACK, I think it would be useful to have the option of providing arguments to unit and fuzz tests.

Is all the code processing -- DEBUG_LOG_OUT still needed when we might do as well -- -printttoconsole=1 ?

src/test/main.cpp

vasild · 2021-11-04T10:29:14Z

Is all the code processing -- DEBUG_LOG_OUT still needed when we might do as well -- -printttoconsole=1 ?

Not needed anymore. I did not remove it because I do not want to break existing functionality - I do not know how many scripts out there use it, or in CI, or how many devs are just used to it. Given that it is just for testing/development purposes, not used in real/production nodes I think it should be ok to remove it in a followup PR some time after this is merged.

vasild · 2021-11-04T15:33:04Z

9b6098f41f...0c541a6dba: take suggestions and print the error from unit tests if parsing of the arguments fails.

src/test/util/setup_common.cpp

src/test/fuzz/fuzz.cpp

src/test/fuzz/deserialize.cpp

src/test/fuzz/fuzz.cpp

src/test/addrman_tests.cpp

jonatack

Did a bit more testing. It seems it could be very helpful for this patch to print the parsed arguments and values as feedback to the user, to be sure they are correct. A few minor comments follow.

src/test/fuzz/connman.cpp

src/test/util/setup_common.h

src/test/main.cpp

vasild · 2021-11-05T14:31:36Z

0c541a6dba...17fac018c2: address suggestions, biggest changes due to avoidance of gArgs.

src/bench/addrman.cpp

src/test/fuzz/addrman.cpp

src/test/fuzz/connman.cpp

vasild · 2021-11-05T17:06:01Z

17fac018c2...b374709725: address suggestions

vasild · 2021-11-12T11:14:50Z

b374709725...51b241bf2a: rebase due to conflicts

josibake · 2021-12-27T11:59:50Z

ACK 72544a0

verified the change using git range-diff and also ran the fuzzer to verify. I think it makes sense to limit just to libFuzz for now, but it would be nice to confirm if this is an issue for other fuzzers or not

maflcko · 2022-01-04T08:40:12Z

doc/fuzzing.md

@@ -71,6 +71,14 @@ block^@M-^?M-^?M-^?M-^?M-^?nM-^?M-^?

 In this case the fuzzer managed to create a `block` message which when passed to `ProcessMessage(...)` increased coverage.

+It is possible to specify `bitcoind` arguments to the `fuzz` executable.


This should only be possible if that specific FUZZ test starts a Bitcoin Core node

Right, added some more words to make it clearer (hopefully). Feel free to suggest how to explain it better, if still unclear.

maflcko · 2022-01-04T08:43:38Z

src/test/README.md


-    test_bitcoin --log_level=all --run_test=getarg_tests -- DEBUG_LOG_OUT


I don't think --printtoconsole=1 is a replacement for -- DEBUG_LOG_OUT. DEBUG_LOG_OUT would work even if the unit test didn't start a Bitcoin Core node internally, no?

Right. However, currently all unit tests create such a node: BOOST_FIXTURE_TEST_SUITE(..., X) where X is BasicTestingSetup or something that has inherited from it. So I think DEBUG_LOG_OUT can be completely phased out, following this PR.

It is possible to create "pure" unit tests without any fixture, IIRC

Ok, are you suggesting to restore the mention of DEBUG_LOG_OUT in src/test/README.md or to not phase out DEBUG_LOG_OUT after this PR?

I guess it is fine to remove DEBUG_LOG_OUT. I just wanted to mention that not all test spin up a node.

vasild · 2022-01-04T17:12:12Z

72544a0053...e25dce2e91: rebase due to conflicts, elaborate the added doc snippet

Invalidates ACK from @mzumsande, @josibake

src/test/addrman_tests.cpp

mzumsande · 2022-01-05T15:07:06Z

ACK e25dce2 after rebase.

maflcko · 2022-01-11T10:48:21Z

ACK e25dce2 after rebase.

Oh, I didn't realize this meant re-ACK. I interpreted it as I'll ack after rebase, so I didn't merge this.

Retrieve the command line arguments from boost and pass them to `BasicTestingSetup` so that we gain extra flexibility of passing any config options on the test command line, e.g.: ``` test_bitcoin -- -printtoconsole=1 -checkaddrman=5 ```

Retrieve the command line arguments from the fuzzer and save them for later retrieval by `BasicTestingSetup` so that we gain extra flexibility of passing any config options on the test command line, e.g.: ``` FUZZ=addrman ./src/test/fuzz/fuzz --checkaddrman=5 ``` A fuzz test should call `MakeNoLogFileContext<>()` in its initialize function in order to invoke the constructor of `BasicTestingSetup`, which sets `gArgs`.

So that it is easy to modify through the file `bench/addrman.cpp`.

In addrman unit tests, make it possible to override the check ratio from the command line, without recompiling: ``` test_bitcoin --run_test="addrman_tests/*" -- -checkaddrman=1 ``` Also, make the arguments of the constructor of `AddrManTest` the same as the arguments of `AddrMan`.

Make it possible to override from the command line (without recompiling) the addrman check ratio in the common `TestingSetup::m_node::addrman` (used by all unit tests) instead of hardcoding it to 0: ``` test_bitcoin --run_test="transaction_tests/tx_valid" -- -checkaddrman=1 ```

Make it possible to override from the command line (without recompiling) the addrman check ratio in addrman fuzz tests instead of hardcoding it to 0: ``` FUZZ=addrman ./src/test/fuzz/fuzz --checkaddrman=5 ```

Make it possible to override from the command line (without recompiling) the addrman check ratio in non-addrman fuzz tests (connman and deserialize) instead of hardcoding it to 0: ``` FUZZ=connman ./src/test/fuzz/fuzz --checkaddrman=5 ```

vasild · 2022-01-11T11:17:21Z

e25dce2e91...7f122a4188: rebase due to conflicts

Invalidates ACK from @mzumsande

Previously invalidated ACK from @josibake

mzumsande · 2022-01-13T00:07:06Z

re-ACK 7f122a4

josibake · 2022-01-13T17:45:38Z

reACK 7f122a4

… fuzz t…

vasild mentioned this pull request Oct 27, 2021

addrman: Make consistency checks a runtime option #20233

Closed

DrahtBot added the GUI label Oct 27, 2021

This was referenced Oct 27, 2021

init: Coalesce Chainstate loading sequence between {,non-}unittest codepaths #23280

Merged

Make all networking code mockable #21878

Closed

test: Run AppInitSanityChecks before all tests #21763

Closed

jonatack reviewed Nov 3, 2021

View reviewed changes

src/test/fuzz/fuzz.cpp Outdated Show resolved Hide resolved

mzumsande reviewed Nov 3, 2021

View reviewed changes

src/test/main.cpp Show resolved Hide resolved

vasild force-pushed the checkaddrman_in_tests branch from 9b6098f to 0c541a6 Compare November 4, 2021 15:31

maflcko reviewed Nov 4, 2021

View reviewed changes

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

maflcko reviewed Nov 4, 2021

View reviewed changes

jonatack reviewed Nov 4, 2021

View reviewed changes

src/test/fuzz/connman.cpp Show resolved Hide resolved

src/test/util/setup_common.h Show resolved Hide resolved

src/test/main.cpp Show resolved Hide resolved

vasild force-pushed the checkaddrman_in_tests branch from 0c541a6 to 17fac01 Compare November 5, 2021 14:31

maflcko reviewed Nov 5, 2021

View reviewed changes

src/bench/addrman.cpp Outdated Show resolved Hide resolved

src/test/fuzz/addrman.cpp Outdated Show resolved Hide resolved

src/test/fuzz/connman.cpp Outdated Show resolved Hide resolved

vasild force-pushed the checkaddrman_in_tests branch from 17fac01 to b374709 Compare November 5, 2021 16:59

DrahtBot mentioned this pull request Nov 10, 2021

addrman: tidy up unit tests #23477

Merged

DrahtBot added the Needs rebase label Nov 12, 2021

vasild force-pushed the checkaddrman_in_tests branch from b374709 to 51b241b Compare November 12, 2021 11:14

maflcko removed the GUI label Nov 12, 2021

DrahtBot added GUI and removed Needs rebase labels Nov 12, 2021

This was referenced Nov 13, 2021

Add src/node/ and src/wallet/ code to node:: and wallet:: namespaces #23497

Merged

scripted-diff: Use clang-tidy syntax for C++ named arguments #23545

Closed

scripted-diff: Use clang-tidy syntax for C++ named arguments (tests only) #23546

Merged

DrahtBot added the Needs rebase label Dec 1, 2021

DrahtBot added the GUI label Dec 27, 2021

maflcko added Tests and removed GUI labels Dec 27, 2021

maflcko changed the title ~~Parse command line arguments from unit and fuzz tests, make addrman consistency check ratio easier to change~~ test: Parse command line arguments from unit and fuzz tests, make addrman consistency check ratio easier to change Dec 27, 2021

maflcko reviewed Jan 4, 2022

View reviewed changes

DrahtBot added the Needs rebase label Jan 4, 2022

vasild force-pushed the checkaddrman_in_tests branch from 72544a0 to e25dce2 Compare January 4, 2022 17:10

DrahtBot removed the Needs rebase label Jan 4, 2022

mzumsande reviewed Jan 5, 2022

View reviewed changes

src/test/addrman_tests.cpp Show resolved Hide resolved

DrahtBot added the Needs rebase label Jan 11, 2022

vasild added 7 commits January 11, 2022 11:53

bench: put addrman check ratio in a variable

6dff621

So that it is easy to modify through the file `bench/addrman.cpp`.

fuzz: addrman fuzz tests: override-able check ratio

3bd83e2

Make it possible to override from the command line (without recompiling) the addrman check ratio in addrman fuzz tests instead of hardcoding it to 0: ``` FUZZ=addrman ./src/test/fuzz/fuzz --checkaddrman=5 ```

vasild force-pushed the checkaddrman_in_tests branch from e25dce2 to 7f122a4 Compare January 11, 2022 11:17

DrahtBot removed the Needs rebase label Jan 11, 2022

maflcko merged commit d0bf9bb into bitcoin:master Jan 17, 2022

vasild deleted the checkaddrman_in_tests branch January 17, 2022 08:53

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 18, 2022

Merge bitcoin#23373: test: Parse command line arguments from unit and…

52b430a

… fuzz t…

bitcoin locked and limited conversation to collaborators Jan 17, 2023

		@@ -71,6 +71,14 @@ block^@M-^?M-^?M-^?M-^?M-^?nM-^?M-^?

		In this case the fuzzer managed to create a `block` message which when passed to `ProcessMessage(...)` increased coverage.

		It is possible to specify `bitcoind` arguments to the `fuzz` executable.


		test_bitcoin --log_level=all --run_test=getarg_tests -- DEBUG_LOG_OUT

test: Parse command line arguments from unit and fuzz tests, make addrman consistency check ratio easier to change #23373

test: Parse command line arguments from unit and fuzz tests, make addrman consistency check ratio easier to change #23373

Uh oh!

Conversation

vasild commented Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jonatack 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!

mzumsande 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!

vasild commented Nov 4, 2021

Uh oh!

vasild commented Nov 4, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vasild commented Nov 5, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vasild commented Nov 5, 2021

Uh oh!

vasild commented Nov 12, 2021

Uh oh!

josibake commented Dec 27, 2021

Uh oh!

maflcko Jan 4, 2022

Choose a reason for hiding this comment

Uh oh!

vasild Jan 4, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 4, 2022

Choose a reason for hiding this comment

Uh oh!

vasild Jan 4, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 4, 2022

Choose a reason for hiding this comment

Uh oh!

vasild Jan 5, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 5, 2022

Choose a reason for hiding this comment

Uh oh!

vasild commented Jan 4, 2022

Uh oh!

Uh oh!

mzumsande commented Jan 5, 2022

Uh oh!

maflcko commented Jan 11, 2022

Uh oh!

vasild commented Jan 11, 2022

Uh oh!

mzumsande commented Jan 13, 2022

vasild commented Oct 27, 2021 •

edited

Loading

DrahtBot commented Oct 27, 2021 •

edited

Loading

jonatack left a comment •

edited

Loading

mzumsande left a comment •

edited

Loading