test: remove Boost Test from libtest

fanquake commented at 7:00 AM on September 5, 2022: member

Context is the discussion here: https://github.com/bitcoin/bitcoin/pull/25974/files#r961541457.

Output:

[test/util/chainstate.h:38] [CreateAndActivateUTXOSnapshot] Wrote UTXO snapshot to /var/folders/sq/z88fhjzj0b19ftsd2_bjrmjm0000gn/T/test_common_Bitcoin Core/8f2783bb3dbf10c669cd892192d70efcca4bab250226856fed7ffecdb378ffc7/test_snapshot.100.dat: {"coins_written":100,"base_hash":"571d80a9967ae599cec0448b0b0ba1cfb606f584d8069bd7166b86854ba7a191","base_height":100,"path":"/var/folders/sq/z88fhjzj0b19ftsd2_bjrmjm0000gn/T/test_common_Bitcoin Core/8f2783bb3dbf10c669cd892192d70efcca4bab250226856fed7ffecdb378ffc7/test_snapshot.100.dat","txoutset_hash":"cd1ba1c3f393058ae743b7c6bdbd00c897744cdcf022c9f2f0f2b4565c08a49c","nchaintx":101}

test: remove Boost Test from libtest util

Context is the discussion here:
https://github.com/bitcoin/bitcoin/pull/25974/files#r961541457.

a7dbf74d72

fanquake added the label Tests on Sep 5, 2022

Sjors commented at 4:38 PM on September 5, 2022: member

tACK a7dbf74

Though I'm not sure how to verify that there weren't other instances via indirect includes. It seems so at first glance, because include <boost/test only occurs in src/test and src/wallet/test and test/util/chainstate.h doesn't import from there.

in src/test/util/chainstate.h:39 in a7dbf74d72

  34 | @@ -36,8 +35,8 @@ CreateAndActivateUTXOSnapshot(node::NodeContext& node, const fs::path root, F ma
  35 |  
  36 |      UniValue result = CreateUTXOSnapshot(
  37 |          node, node.chainman->ActiveChainstate(), auto_outfile, snapshot_path, snapshot_path);
  38 | -    BOOST_TEST_MESSAGE(
  39 | -        "Wrote UTXO snapshot to " << fs::PathToString(snapshot_path.make_preferred()) << ": " << result.write());
  40 | +    LogPrintf(
  41 | +        "Wrote UTXO snapshot to %s: %s", fs::PathToString(snapshot_path.make_preferred()), result.write());

hebasto commented at 1:57 PM on September 7, 2022:

This change breaks the current behavior:

on master (0ebd4db32b39cb7c505148f090df4b7ac778c307):

$ ./src/test/test_bitcoin -t validation_chainstate_tests -l message
Running 2 test cases...
Wrote UTXO snapshot to /tmp/test_common_Bitcoin Core/5e0e8e31801ee29cbb6fa31d1208471d3ad2487432fdbf2cddda1b2184f91c76/test_snapshot.110.dat: {"coins_written":110,"base_hash":"696e92821f65549c7ee134edceeeeaaa4105647a3c4fd9f298c0aec0ab50425c","base_height":110,"path":"/tmp/test_common_Bitcoin Core/5e0e8e31801ee29cbb6fa31d1208471d3ad2487432fdbf2cddda1b2184f91c76/test_snapshot.110.dat","txoutset_hash":"1ebbf5850204c0bdb15bf030f47c7fe91d45c44c712697e4509ba67adb01c618","nchaintx":111}

*** No errors detected

this PR (a7dbf74d72abfe74aaeb3c11dbfa98d43b85b4ec):

$ ./src/test/test_bitcoin -t validation_chainstate_tests -l message
Running 2 test cases...

*** No errors detected

fanquake commented at 2:05 PM on September 7, 2022:

I don't think that's a break in behaviour. The message is still written to the .log.

hebasto commented at 2:29 PM on September 7, 2022:

The message is still written to the .log.

That is also a behavior change:

on master (0ebd4db32b39cb7c505148f090df4b7ac778c307):

$ ./src/test/test_bitcoin -t validation_chainstate_tests -- DEBUG_LOG_OUT | grep -e 'Wrote UTXO'

*** No errors detected

this PR (a7dbf74d72abfe74aaeb3c11dbfa98d43b85b4ec):

$ ./src/test/test_bitcoin -t validation_chainstate_tests -- DEBUG_LOG_OUT | grep -e 'Wrote UTXO'
2022-09-07T14:26:09.949830Z (mocktime: 2020-08-31T15:34:22Z) [test] [test/util/chainstate.h:39] [CreateAndActivateUTXOSnapshot] Wrote UTXO snapshot to /tmp/test_common_Bitcoin Core/d86bd6189fcd798f749edf44bf5029213c9a8add2bc39da5272b201d821e341d/test_snapshot.110.dat: {"coins_written":110,"base_hash":"696e92821f65549c7ee134edceeeeaaa4105647a3c4fd9f298c0aec0ab50425c","base_height":110,"path":"/tmp/test_common_Bitcoin Core/d86bd6189fcd798f749edf44bf5029213c9a8add2bc39da5272b201d821e341d/test_snapshot.110.dat","txoutset_hash":"1ebbf5850204c0bdb15bf030f47c7fe91d45c44c712697e4509ba67adb01c618","nchaintx":111}[Chainstate [ibd] @ height 110 (696e92821f65549c7ee134edceeeeaaa4105647a3c4fd9f298c0aec0ab50425c)] resized coinsdb cache to 0.0 MiB

*** No errors detected

I don't think that's a break in behaviour.

But behaviour change looks obvious, doesn't it?

fanquake commented at 2:58 PM on September 8, 2022:

But behaviour change looks obvious, doesn't it?

I'm not sure what point you're trying to make. I'm aware the behaviour is changing, but I don't think calling it a "break" is correct. What's being broken? We could just as easily delete this logging entirely, it's only informative, so I'm not sure why whether it gets dumped to the Boost message log, or into the .log output makes so much of a difference.

Ultimately I'm only doing this so we can remove the Boost header from the util lib, and this does not seem like something that should be a blocker to doing that.

MarcoFalke commented at 4:01 PM on September 8, 2022: member

For those who are wondering if this function is used in the fuzz tests. Likely no, because it would lead to linker errors if used in the fuzz tests:

/usr/bin/ld: test/fuzz/fuzz-addition_overflow.o: in function `_GLOBAL__sub_I_addition_overflow.cpp':
/usr/include/boost/test/unit_test_log.hpp:227: undefined reference to `boost::unit_test::unit_test_log_t::instance()'
clang-13: error: linker command failed with exit code 1 (use -v to see invocation)

theuni commented at 3:31 PM on September 9, 2022: member

ACK a7dbf74d72abfe74aaeb3c11dbfa98d43b85b4ec

Agree this is a slight behavioral change, but as it's the only use of boost it seems like pretty obvious low-hanging fruit.

From #25974:

Ah ok. I think I have a slight preference to keep boost out of libtest_util, and thus out of the fuzz tests

I think this also means that we could drop boost from the libtest_util_a_CPPFLAGS to enforce that new dependencies don't creep back in, no?

Unfortunately, boost is lumped in with BITCOIN_INCLUDES, but it's the only external dependency that remains there. Maybe it's time to break BOOST_CPPFLAGS out of there and use it where it's needed instead (all over the place I assume, lots of copy/paste), that way we can surgically enforce where it's allowed.

Consider that a suggestion for a follow-up PR :)

fanquake commented at 4:58 PM on September 9, 2022: member

Consider that a suggestion for a follow-up PR :)

Started in #26056.

MarcoFalke merged this on Sep 10, 2022

MarcoFalke closed this on Sep 10, 2022

fanquake deleted the branch on Sep 10, 2022

sidhujag referenced this in commit ad898b4ecd on Sep 11, 2022

bitcoin locked this on Sep 10, 2023

test: remove Boost Test from libtest_util #26009