[WIP] refactor: migrate unit tests to Google Test #31988

mabu44 commented at 5:02 PM on March 4, 2025: none

WIP: both the code and the following motivations will be updated with new information obtained during research and through feedback from reviewers.

Motivation

By migrating the unit tests from Boost to Google Test we can increase the chances of removing all dependencies from Boost in the future.

Benefits of Google Test

Most used C++ testing framework with a large community
Includes Google Mock for creating mock objects
Similar syntax to Boost, making refactor simpler
Google Test is thread-safe on systems where the pthread library is available, Boost.Test is not thread-safe.
Outputs currently running tests (see #8670)

Disadvantages of Google Test

Potential future dependency on Abseil, as stated in Google Test's main README

TODO

Verify whether Google Test meets all or some of the required features discussed in PR #8670
Check if the refactor can be done via scripted diffs: some replacement are trivial, but some changes are not. I don't know if we can create an hygienic commit using scripted-diffs.
Evaluate alternative testing frameworks, such as Catch2
Complete the code migration (currently only some tests have been refactored)

Ported some tests to Google Test ac56e51008

DrahtBot commented at 5:02 PM on March 4, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31988.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

fanquake closed this on Mar 4, 2025

in src/test/argsman_tests.cpp:6 in ac56e51008

   1 | @@ -2,6 +2,10 @@
   2 |  // Distributed under the MIT software license, see the accompanying
   3 |  // file COPYING or http://www.opensource.org/licenses/mit-license.php.
   4 |  
   5 | +#include <gtest/gtest.h>
   6 | +// WARNING: Google Mock implements an Assert macro, creating a name conflict with the Bitcoin Assert macro

hodlinator commented at 9:46 AM on March 5, 2025:

Seems like their Assert should have ended up inside testing::internal namespaces... But some C++ lookup logic might not require specifying them in some cases.

https://github.com/google/googletest/blob/e88cb95b92acbdce9b058dd894a68e1281b38495/googlemock/include/gmock/internal/gmock-internal-utils.h#L250

mabu44 commented at 11:46 AM on March 6, 2025:

It seems that Bitcoin defines an Assert macro in src/util/check.h, while GMock has an Assert inline function definition. If you try to move the inclusion of GMock at the end of the other include instructions, the preprocessor will try to use the Assert macro to change the code of gmock-internal-utils.h and fail in doing so because of a different number of parameters.

in src/test/argsman_tests.cpp:166 in ac56e51008

 181 |          }
 182 |  
 183 |          if (expect.list_value) {
 184 |              auto l = test.GetArgs("-value");
 185 | -            BOOST_CHECK_EQUAL_COLLECTIONS(l.begin(), l.end(), expect.list_value->begin(), expect.list_value->end());
 186 | +            ASSERT_THAT(l, ::testing::ContainerEq(expect.list_value.value()));

hodlinator commented at 9:47 AM on March 5, 2025:

Interesting to see how this is done in GoogleTest.

mabu44 commented at 9:26 AM on March 6, 2025:

This instruction is the reason why I needed Google Mock. Otherwise, I could have avoided such a dependency in the POC and checked with ASSERT_EQ on size and on items.

in src/test/coinstatsindex_tests.cpp:36 in ac56e51008

  34 |      // is started.
  35 | -    BOOST_CHECK(!coin_stats_index.BlockUntilSyncedToCurrentChain());
  36 | +    EXPECT_FALSE(coin_stats_index.BlockUntilSyncedToCurrentChain());
  37 |  
  38 | -    BOOST_REQUIRE(coin_stats_index.StartBackgroundSync());
  39 | +    EXPECT_TRUE(coin_stats_index.StartBackgroundSync());

hodlinator commented at 9:52 AM on March 5, 2025:

Should have been ASSERT_TRUE?

mabu44 commented at 9:17 AM on March 6, 2025:

Yes

in src/test/coinstatsindex_tests.cpp:30 in ac56e51008

  26 | @@ -27,13 +27,13 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_initial_sync, TestChain100Setup)
  27 |      }
  28 |  
  29 |      // CoinStatsIndex should not be found before it is started.
  30 | -    BOOST_CHECK(!coin_stats_index.LookUpStats(*block_index));
  31 | +    EXPECT_EQ(coin_stats_index.LookUpStats(*block_index), std::nullopt);

hodlinator commented at 9:54 AM on March 5, 2025:

Just curious - did you choose EXPECT_EQ/EXPECT_NE here and below over EXPECT_TRUE/EXPECT_FALSE to increase clarity, or some other reason?

mabu44 commented at 9:22 AM on March 6, 2025:

I started doing this kind of changes to increase clarity, but then realized that these improvements can be difficult to automate using scripted-diffs. So I avoided doing that on other tests.

hodlinator commented at 10:43 AM on March 5, 2025: contributor

(This PR was meant as a POC. Should probably be a draft and/or just opened in personal repo).

It's great that it highlights pros/cons.

Google Test is thread-safe on systems where the pthread library is available, Boost.Test is not thread-safe.

(In practice I've never knowingly run into any issues with Boost.Test regarding this).

Outputs currently running tests (see Replacing Boost Test Framework [#8670](/bitcoin-bitcoin/8670/))

My impression is that the way we use CTest+Boost.Test does output currently running tests, so this is no longer an issue. See:

₿ ctest -j 20 --test-dir build --output-on-failure
Internal ctest changing into directory: /home/hodlinator/bitcoin/build
Test project /home/hodlinator/bitcoin/build
        Start   6: secp256k1_tests
        Start   5: secp256k1_noverify_tests
        Start 125: coinselector_tests
        Start   7: secp256k1_exhaustive_tests
...
        Start 117: validation_block_tests
        Start 111: txvalidationcache_tests
...
  1/137 Test [#111](/bitcoin-bitcoin/111/): txvalidationcache_tests ..............   Passed    1.60 sec
        Start 124: db_tests
  2/137 Test [#117](/bitcoin-bitcoin/117/): validation_block_tests ...............   Passed    1.68 sec
        Start 133: wallet_crypto_tests

Potential future dependency on Abseil, as stated in Google Test's main README

This seems like a deal-breaker unfortunately, but still interesting to see how our test code would change.

Check if the refactor can be done via scripted diffs: some replacement are trivial, but some changes are not. I don't know if we can create an hygienic commit using scripted-diffs.

For an advanced scripted-diff example, see especially the last two commits in: https://github.com/bitcoin/bitcoin/pull/31260/commits It uses a Python script to make changes to the code, not sure about the hygienic aspect though.

Evaluate alternative testing frameworks, such as Catch2

This would be interesting, as Catch2 has a no-dependencies policy and seems more actively developed than Boost.Test. I'm however worried there could be pushback from adding Catch2 without completely getting rid of Boost for other things in the project, as the net number of dependencies would increase.

(Doesn't build for me, probably something NixOS-specific despite adding gtest to the environment).

CMake Error at /nix/store/01dapiw4pjrc9s0166dcyg9xkw0rv01z-cmake-3.31.5/share/cmake-3.31/Modules/FindPackageHandleStandardArgs.cmake:233 (message):
  Could NOT find GTest (missing: GTEST_LIBRARY GTEST_INCLUDE_DIR
  GTEST_MAIN_LIBRARY)
Call Stack (most recent call first):
  /nix/store/01dapiw4pjrc9s0166dcyg9xkw0rv01z-cmake-3.31.5/share/cmake-3.31/Modules/FindPackageHandleStandardArgs.cmake:603 (_FPHSA_FAILURE_MESSAGE)
  /nix/store/01dapiw4pjrc9s0166dcyg9xkw0rv01z-cmake-3.31.5/share/cmake-3.31/Modules/FindGTest.cmake:273 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
  CMakeLists.txt:585 (find_package)

l0rinc commented at 11:44 AM on March 5, 2025: contributor

Cool proof of concept, eventually we might want to give another testing framework a try, but not sure we're there yet. The limitations of Boost bothers me a bit every time I'm using it, but not enough to embark on a huge refactor yet. Maybe it would still be easier to strangle out the old one by having two options temporarily - or to have an api with minimal changes compared to current one, supporting both. Not sure, but my impression was that the pain isn't great enough yet. But if you're passionate about this, keep working on it!

hebasto commented at 12:07 PM on March 5, 2025: member

Motivation

By migrating the unit tests from Boost to Google Test we can increase the chances of removing all dependencies from Boost in the future.

Depending on Boost.Test for test-only code does not seem like a problem to me.

mabu44 commented at 9:10 AM on March 6, 2025: none

(This PR was meant as a POC. Should probably be a draft and/or just opened in personal repo).

Yes, this was a draft PR before being closed.

My impression is that the way we use CTest+Boost.Test does output currently running tests, so this is no longer an issue.

Right, but still valid if test_bitcoin is run directly, for instance because you need to provide custom parameters or execute only specific tests.

This seems like a deal-breaker unfortunately, but still interesting to see how our test code would change.

Agree

(Doesn't build for me, probably something NixOS-specific despite adding gtest to the environment).

Don't know about NixOS. On Debian the required packages are libgtest-dev and libgmock-dev. There is also a googletest package in the official repo but should not be required to compile and run the tests.

hodlinator commented at 10:15 AM on March 6, 2025: contributor

Yes, this was a draft PR before being closed.

Seems a bit harsh to me in that case. I understand that hebasto, fanquake & others have put time into carrying over Boost.Test from autotools into CMake recently, so not eager to have that work replaced. Getting Boost out of the production code is a higher priority than getting it out of test code, but I still think it is valid to work on.

Right, but still valid if test_bitcoin is run directly, for instance because you need to provide custom parameters or execute only specific tests.

One can use ctest ... -R pattern to filter tests and I've only passed something resembling parameters to unit tests when setting the RANDOM_CTX_SEED environment variable. But yeah, I can see how ctest might make test_bitcoin threadsafe in an artificial way.

Don't know about NixOS. On Debian the packages required are libgtest-dev and libgmock-dev. There is also a googletest package in the official repo but should not be required to compile and run the tests.

Figured it out, was mixing my dev shell with temporarily installed packages. gmock was apparently merged into gtest a while back in both upstream and Nix. Nice to take the PR for a spin.