#36091 test: Add debug output to common tested types
https://github.com/bitcoin/bitcoin/pull/36091 · · +394/-166 in 18 files, 2 commits · labels: Tests
Goal
- Print mismatched values in unit test failure messages instead of only reporting boolean failures
- Speed up debugging for developers investigating test regressions
This pull request defines `operator<<` stream formatting overloads for several core Bitcoin types (such as `CScript`, `FeeFrac`, `CAddress`, `CPubKey`, and `CTxDestination`) in `src/test/util/common.{h,cpp}`. It then converts callsites across 15 unit test suites from bare `BOOST_CHECK` equality comparisons to `BOOST_CHECK_EQUAL` and `BOOST_CHECK_EQUAL_COLLECTIONS`, allowing failure diagnostics to print actual mismatched values.
Problem: When unit test assertions using bare `BOOST_CHECK(a == b)` fail, Boost only reports a boolean failure without printing the values of the objects being compared, hindering debugging. Bitcoin Core custom types lacked stream output operators needed by equality check macros.
Category: Test infrastructure (#8 of 45)
P3 · test coverage
- P3 because it improves failure diagnostics across unit test suites
- Provides formatting infrastructure required by follow-up test work
- Does not fix test flakiness or expand coverage into high-risk code
Worthwhile test utility cleanup that improves failure diagnostics for unit test runs and provides printing infrastructure needed for #35713. It does not fix flaky tests or expand functional coverage into high-risk code paths, keeping it in P3.
Membership: Touches unit test utility files in src/test/util/ and test assertion macros across 15 unit test files.
Factors: security/stability 0, bug 0, performance 0, user value 0, leverage 1
Reviewability: Ready
- Ready for review
- Clean CI, no merge conflicts, and review feedback has been addressed
Clean CI and no merge conflicts. The author promptly addressed review comments and updated the callsite coverage.
Author status: active
Resolved concerns:
- Build overhead and header leakage: moved printer implementations from a header to `common.cpp` with forward declarations in `common.h` as suggested by maflcko
- Missing diagnostic details and potential crash on empty decoded BIP32 keys: fixed guards and added network, hex, and ASM disambiguation as noted by jeanpablojp
- Completeness of callsite migrations: author ran regular expression searches to exhaustively update matching checks
Agreement: Strong
- Strong consensus across multiple reviewers
- Support for the overall approach and test utility cleanup (maflcko)
- Verified by testing failure output and crash guards (jeanpablojp)
- Code approval after suggested callsite and formatting fixes were applied (ismaelsadeeq)
- Concept approval without detailed remarks (sedited, brunoerg)
Strong consensus with multiple Concept and Code ACKs (maflcko, ismaelsadeeq, jeanpablojp, sedited, brunoerg)
Reviewers tested the error output improvements, suggested enhancements that were implemented, and gave repeated ACKs.
- maflcko approved approach: 'approach lgtm... all of this seems fine'
- jeanpablojp gave a tested ACK after verifying failure printing and crash guards
- ismaelsadeeq gave a reACK after suggestions on callsite conversion and script formatting were applied
- sedited and brunoerg gave Concept ACKs
Review verdicts (DrahtBot): 0 (+2)
- Stale ACK: jeanpablojp, ismaelsadeeq
- Concept ACK: sedited, brunoerg
Dependencies
Enables:
Files
560 lines under test/bench/ci.
- src/test/util/common.cpp +146/-0
- src/test/script_tests.cpp +31/-31
- src/test/util/common.h +55/-0
- src/test/feefrac_tests.cpp +27/-26
- src/test/script_standard_tests.cpp +19/-19
- src/test/net_tests.cpp +16/-16
- src/test/rbf_tests.cpp +21/-10
- src/test/addrman_tests.cpp +15/-15
- src/test/mempool_fee_estimator_tests.cpp +14/-13
- src/test/key_tests.cpp +10/-10
- src/test/descriptor_tests.cpp +13/-6
- src/test/bip32_tests.cpp +7/-6
- src/test/txindex_tests.cpp +5/-4
- src/test/psbt_tests.cpp +4/-3
- src/test/txgraph_tests.cpp +4/-3
- src/test/netbase_tests.cpp +4/-2
- src/test/miner_tests.cpp +2/-2
- src/test/util/CMakeLists.txt +1/-0
Card
This PR adds stream output `operator<<` overloads for common types like `CScript`, `FeeFrac`, and `CTxDestination` in test utilities, and converts bare `BOOST_CHECK` comparisons across 15 unit test files to `BOOST_CHECK_EQUAL`. It solves the problem of opaque boolean failures during unit test runs, ensuring developers see the mismatched values when tests fail. The PR also provides printer logic intended for the unit test runner overhaul in #35713. Review state is active with strong multi-reviewer agreement and no blocking objections.