#36091 test: Add debug output to common tested types
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #36091 test: Add debug output to common tested types | Waiting on author | 1 (+2) | Strong | 0 + 384 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Adds stream output `operator<<` overloads for common Bitcoin Core types in `src/test/util/common.{h,cpp}` so test runners can print their contents. Converts equality assertions across 15 unit test suites from bare `BOOST_CHECK(a == b)` to `BOOST_CHECK_EQUAL` or `BOOST_CHECK_EQUAL_COLLECTIONS`. This produces informative mismatch output on test failure without changing production code.
Problem: When unit test assertions using `BOOST_CHECK(a == b)` fail, Boost only reports a boolean failure without printing mismatched values. Types like `FeeFrac`, `CScript`, `CAddress`, and `CTxDestination` lacked streaming operators in the test suite, making test failures harder to debug.
Evidence of importance:
- PR description: 'If the `<<` operator exists, boost can use this as a debug output when a check fails (besides `BOOST_CHECK`).'
- PR description notes that 'All of these display strings would be used as part of #35713, but we can also use them with boost today.'
Scope: Mostly mechanical changes across unit tests replacing `BOOST_CHECK` comparisons with `BOOST_CHECK_EQUAL` alongside new stream operators in `src/test/util/common.{h,cpp}`.
Risk of a mistake: low (tests). Touches only test utility helpers and unit test assertions, with no impact on production node behavior or release binaries.
Changed since the previous assessment: The scope and implementation remain the same; changes are minor rebasing and test macro adjustments.
Reviewability
Paused · Waiting on author
Open objection from sedited with no author reply for 10 days; review would repeat what has been asked.
Waiting on (from the thread read):
- author: sedited asked to convert recently added BOOST_CHECK call sites in bip32_tests, bip352_tests, and key_tests 2026-10-21
Agreement
Strong
substantive support, no open objection (ismaelsadeeq, jeanpablojp, ismaelsadeeq)
Thread notes: Added suggestion by sedited to convert new BOOST_CHECK callsites, and recorded a new ACK from maflcko.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| jeanpablojp (contributor) | objection, maintenance | Header drags addrman, key_io, and protocol into tests that do not need them, increasing preprocessed lines and causing unnecessary recompilations | fixed · fix pushed · Author moved implementations to common.cpp with forward declarations in common.h | answered | yes | 2026-08-29: 'feefrac_tests and txgraph_tests only need the FeeFrac printers, but this block drags addrman, key_io and protocol in with it. Touching addrman now rebuilds both, and it's about 12k extra preprocessed ' Status from: 2026-09-02: rustaceanrob: 'Added common.cpp with the implementations and all forward delc to common.h in latest push' |
| jeanpablojp (contributor) | objection, correctness | Printers omit fields compared by operator==, causing failed checks to display identical values or generic types without diagnostic detail | fixed · fix pushed · Author added missing fields to CAddress and CTxDestination printers | changed | yes | 2026-08-29: 'This one leaves out nTime and nServices, which operator== does compare, so in the two addrv1/addrv2 unserialize checks a regression in time or in flags comes out as [::1]:0 != [::1]:0. The CTxDestinat' Status from: 2026-08-30: rustaceanrob: 'Added additional info for CAddress and took a glance through the rest of the ==, I think all fields should be covered. A' |
| jeanpablojp (contributor) | objection, safety | Encode asserts on an empty key, causing bip32_tests to abort with SIGABRT instead of reporting failure and continuing the suite | fixed · fix pushed · Author added length and validity guards, verified by reviewer | changed | yes | 2026-08-29: 'Encode() asserts on an empty key, which is what DecodeExtKey leaves you with when it fails, and that's the value bip32_tests compares. I broke the decode to see. The case dies on SIGABRT and takes the' Status from: 2026-08-30: jeanpablojp: 'Yes, that's what I meant. I broke the decode again and now it prints CExtKey(<invalid>) and the suite finishes, no SIGAB' |
| jeanpablojp (contributor) | suggestion, correctness | Include m_net in CService printer so IPv6 and CJDNS with identical bytes do not print identically | fixed · fix pushed · Author added net field to CService debug string | answered | yes | 2026-08-30: 'While checking the changes I noticed a small nit. CService's == compares m_net, which the printer doesn't show, so IPv6 and CJDNS with the same bytes come out identical.' Status from: 2026-08-31: rustaceanrob: 'Added a net field to CService' |
| maflcko (member) | suggestion, approach | Move printer implementations to a cpp file with forward declarations in the header to reduce recompilations | fixed · fix pushed · Author moved implementations to common.cpp with forward declarations in common.h | answered | yes | 2026-09-01: 'Most of the functions can be just the signature and a fwd decl for the type, and the actual stringify can move to a cpp file? This saves re-compilation at least in the other cases' Status from: 2026-09-02: rustaceanrob: 'Added common.cpp with the implementations and all forward delc to common.h in latest push' |
| purpleKarrot (member) | suggestion, approach | De-template FeeFrac stream operator and move implementation to the source file | fixed · fix pushed · Author de-templated FeeFrac and ByRatioNegSize and moved implementations to source file | answered | yes | 2026-09-01: 'No need for separate template instantiations for each derived type... And then you can also move the implementation to the source file forward declare FeeFrac in the header.' Status from: 2026-09-01: rustaceanrob: 'I de-templated ByRatioNegSize as well because the only intended and current use case is with FeeFrac. Updated in latest ' |
| maflcko (member) | suggestion, approach | Handle std::pair printer generally or move it to the test to avoid unnecessary includes | fixed · fix pushed · Author generalized std::pair stream operator with concepts constraint | answered | yes | 2026-09-01: 'Why is this pair thing needed? Seems to be a single test-only thing? So either this should be moved to that test, or maybe pair should be handled generally, like the existing optional' Status from: 2026-09-02: rustaceanrob: 'I added a requires requires(std::ostream& os, const T& t, const U& u) { os << t; os << u; }, which I think it correct. O' |
| ismaelsadeeq (member) | suggestion, interface | Add contextual field labels in debug output representations | fixed · fix pushed · Author applied field labels across debug printers | answered | yes | 2026-09-07: 'nit: add context to what we are displaying here and other places.' Status from: 2026-09-08: rustaceanrob: 'Makes sense. Applied those changes as well as adding a addr field to CService and CAddress.' |
| ismaelsadeeq (member) | suggestion, maintenance | Add a comment in common.h describing the printable types for discoverability | fixed · fix pushed · Author added header comment in common.h describing purpose and noting debug-available types | answered | yes | 2026-09-07: 'Should we make common.h self-advertising so authors discover it without a reviewer? A short header comment listing the types it makes BOOST_CHECK_*-printable' Status from: 2026-09-08: rustaceanrob: 'Added a comment to common.h describing the purpose of the file.' |
| ismaelsadeeq (member) | suggestion, scope | Convert remaining BOOST_CHECK call sites in script_standard_tests | fixed · fix pushed · Author updated callsites in script_standard_tests | answered | yes | 2026-09-07: 'It seems you missed some.' Status from: 2026-09-08: rustaceanrob: 'Applied those changes, thanks' |
| ismaelsadeeq (member) | suggestion, scope | Add stream operator overload for CScript | fixed · fix pushed · Author added CScript debug output and updated psbt_tests and script_tests | answered | yes | 2026-09-07: 'Should we add for script too?' Status from: 2026-09-08: rustaceanrob: 'Yeah, definitely, I added a debug output and modified psbt_tests and script_tests to include it.' |
| jeanpablojp (contributor) | objection, correctness | ScriptToAsmStr decodes small pushes as numbers, allowing scripts that differ in bytes to display identical string representations on failure | fixed · fix pushed · Author added hex string alongside ASM representation | answered | yes | 2026-09-08: 'ScriptToAsmStr decodes any push of up to four bytes as a number, so scripts that differ in bytes can come out as the same string... Would it be worth adding hex=%s alongside?' Status from: 2026-09-08: rustaceanrob: 'added the hex string alongside the ASM in the latest push' |
| sedited (member) | question, scope | Ask how exhaustive the conversion of BOOST_CHECK callsites is intended to be | answered · Author clarified that conversion is intended to be exhaustive for types with added debug output and converted missed sites | answered | yes | 2026-09-14: 'How exhaustive is the conversion supposed to be? In this file alone there seem to be more cases where we compare scriptsigs.' Status from: 2026-09-15: rustaceanrob: 'It is intended to be completely exhaustive for the types with << added here. I ran the following: rg -nP ... which showe' |
| ismaelsadeeq (member) | suggestion, scope | Clarify in PR description whether TxGraph::Ref and integer comparisons are intentionally omitted | fixed · Author updated the PR description with rationale for omitted types | answered | yes | 2026-09-15: 'The PR description should be more explicit about this and the rationale.' Status from: 2026-09-16: rustaceanrob: 'Thanks, I updated the description to be more thorough as far as what this seeks to accomplish.' |
| sedited (member) | suggestion, scope | Convert recently added BOOST_CHECK call sites in bip32_tests, bip352_tests, and key_tests to BOOST_CHECK_EQUAL | open | answered | no | 2026-10-21: 'Looks like a few more arrived in the meantime... Can you add these? The PR seems good to go otherwise.' |
Support:
- sedited (member) (Concept ACK, 2026-08-26): no reason given: no reason given: Concept ACK [not substantive]
- brunoerg (member) (Concept ACK, 2026-08-26): no reason given: no reason given: Concept ACK [not substantive]
- jeanpablojp (contributor) (Concept ACK, 2026-08-29): no reason given: no reason given: Concept ACK [not substantive]
- maflcko (member) (Approach ACK, 2026-09-02): no reason given: no reason given: approach lgtm. Haven't looked closely, but all of this seems fine. [not substantive]
- ismaelsadeeq (member) (ACK, 2026-09-07) [read]: substantive: Code review ACK 056bd2582bc96f1fc2b47933badcd4fe85832c3c. Checked the new test modification and did not see any change in the test invariants.
- jeanpablojp (contributor) (ACK, 2026-09-08): no reason given: no reason given: re-ACK 387e443275587a46c25f21ed5e05e02439140a5a [not substantive]
- jeanpablojp (contributor) (Tested ACK, 2026-09-08) [tested]: substantive: Rebuilt and ran the unit tests on this head.
- ismaelsadeeq (member) (ACK, 2026-09-08): no reason given: no reason given: reACK 387e443275587a46c25f21ed5e05e02439140a5a, all my suggestions were addressed. [not substantive]
- ismaelsadeeq (member) (ACK, 2026-09-15): substantive: reACK fc42f878cb578f9d91d6b11e8dd6fe2057a54fbc... This is a nice cleanup consequence of the main goal, which is replacing the framework, and I support it
- ismaelsadeeq (member) (ACK, 2026-09-21): no reason given: no reason given: reACK e239d936cf0f294760e26c4824c022c0b66d8036 [not substantive]
- ismaelsadeeq (member) (ACK, 2026-09-28): no reason given: no reason given: reACK 1f6262c5599124a093589725d7f9311645ea92be [not substantive]
- maflcko (member) (ACK, 2026-10-16): no reason given: no reason given: happy to re-ack for the nits, but up to you. review ACK cf4412a5929433e58ee18239fb3537f4002fd76b [not substantive]
- maflcko (member) (ACK, 2026-10-19): no reason given: review ACK aa94fda3adf5600dc593f7520789f4b6c18de865 🗝 [not substantive]
- maflcko (member) (ACK, 2026-10-22): review ACK 722d08e03980c359bae4e3b65551d6b8ba615a7a 🍟 [not substantive]
Participants:
- brunoerg (member, support, 1 statements 2026-08-26 to 2026-08-26): Concept ACK
- sedited (member, support, 3 statements 2026-08-26 to 2026-10-21): Concept ACK; asked about exhaustiveness and suggested converting recently added callsites
- jeanpablojp (contributor, objection, 9 statements 2026-08-29 to 2026-09-08): raised objections on BIP32 empty key crash, omitted fields in equality display, header bloat, and ambiguous ASM decoding; subsequently tested and ACKed after fixes
- maflcko (member, support, 13 statements 2026-09-01 to 2026-10-22): Approach ACK and repeated ACKs; suggested moving printer implementations to common.cpp and generalizing pair printer
- purpleKarrot (member, neutral, 1 statements 2026-09-01 to 2026-09-01): suggested de-templating FeeFrac stream operator
- ismaelsadeeq (member, support, 10 statements 2026-09-07 to 2026-09-28): repeated ACKs; suggested adding CScript printer, contextual field labels, converting remaining call sites, and updating PR description
Dependencies
Enables:
Files
553 lines under test/bench/ci.
- src/test/util/common.cpp +140/-0
- src/test/script_tests.cpp +31/-31
- src/test/feefrac_tests.cpp +27/-26
- src/test/util/common.h +48/-0
- 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 +11/-11
- src/test/bip32_tests.cpp +11/-10
- src/test/descriptor_tests.cpp +13/-6
- src/test/psbt_tests.cpp +4/-3
- src/test/txgraph_tests.cpp +4/-3
- src/test/netbase_tests.cpp +4/-2
- src/test/bip352_tests.cpp +3/-2
- src/test/miner_tests.cpp +2/-2
- src/test/util/CMakeLists.txt +1/-0
Card
The short text the ranking pass compares across PRs.
This pull request adds stream insertion operator<< overloads for commonly tested Bitcoin Core types in src/test/util/common.{h,cpp} and updates assertions across 15 unit test suites from BOOST_CHECK to BOOST_CHECK_EQUAL or BOOST_CHECK_EQUAL_COLLECTIONS. Previously, equality assertions on these types only printed a boolean failure upon mismatch, forcing developers to write temporary debugging code to inspect actual values. These string formatters also prepare the test suite for the proposed migration away from Boost unit test runner in PR 35713.
Data
dossier JSON · extract JSON · model ledger, generated 2026-10-24T01:16, confidence high, input hash 8864fdfa8ae1b881
thread read through 2026-10-22 (58 statements), head 722d08e039
labels: Tests
thread read through 2026-10-22 (58 statements), head 722d08e039
labels: Tests
Categories
Test infrastructure
P3 · cleanup (ranking pass: Moved up: failure diagnostics plus preparation for the #35713 runner migration give it more leverage than standalone coverage additions.)
Improves unit test failure diagnostics by providing `operator<<` overloads for types like `FeeFrac`, `CScript`, and `CAddress` so Boost can print mismatched values. This improves debuggability and supports test framework migration in #35713, but does not address flaky tests or missing coverage.
Membership: Has the Tests label and adds unit test debugging utilities in src/test/util/common.{h,cpp} across 15 test suites.
Factors: security/stability 0, bug 0, performance 0, user value 0, leverage 1