Merge bitcoin/bitcoin#36018: test: [refactor] Properly use BOOST_CHECK_EXCEPTION
What changed, and why it matters
This commit is a pure test-code cleanup. It replaces hand-written try/catch blocks in Bitcoin Core's unit tests with a standard Boost testing macro, BOOST_CHECK_EXCEPTION. No production code is changed, and the behavior being tested is unchanged. There is no security fix or vulnerability here.
No security action needed. Treat as normal test maintenance / refactor.
Security signals we found
No strong security signals were identified.
Evidence from the diff
The merge commit refactors unit tests across nine test files to use BOOST_CHECK_EXCEPTION / BOOST_CHECK_NO_THROW consistently instead of manual try/catch/flag patterns. In a few cases the checks become stricter by matching exact exception reasons (e.g., ‘end of data’, ‘differential value overflow’). The commit only touches src/test/ and src/wallet/test/ files; no consensus, networking, wallet logic, or other production code is modified.
Changed components
src/test/addrman_tests.cppsrc/test/allocator_tests.cppsrc/test/blockencodings_tests.cppsrc/test/coins_tests.cppsrc/test/reverselock_tests.cppsrc/test/streams_tests.cppsrc/test/sync_tests.cppsrc/test/threadpool_tests.cppsrc/wallet/test/wallet_transaction_tests.cppInspect captured patch +30 / −109
### src/test/addrman_tests.cpp
@@ -12,6 +12,7 @@
#include <netgroup.h>
#include <random.h>
#include <test/data/asmap.raw.h>
+#include <test/util/common.h>
#include <test/util/setup_common.h>
#include <test/util/time.h>
#include <util/asmap.h>
@@ -1075,20 +1076,15 @@ BOOST_AUTO_TEST_CASE(load_addrman)
// Test that the de-serialization does not throw an exception.
auto ssPeers1{AddrmanToStream(addrman)};
- bool exceptionThrown = false;
AddrMan addrman1{EMPTY_NETGROUPMAN, !DETERMINISTIC, GetCheckRatio(m_node)};
BOOST_CHECK(addrman1.Size() == 0);
- try {
+ {
unsigned char pchMsgTmp[4];
- ssPeers1 >> pchMsgTmp;
- ssPeers1 >> addrman1;
- } catch (const std::exception&) {
- exceptionThrown = true;
+ BOOST_CHECK_NO_THROW(ssPeers1 >> pchMsgTmp >> addrman1);
}
BOOST_CHECK(addrman1.Size() == 3);
- BOOST_CHECK(exceptionThrown == false);
// Test that ReadFromStream creates an addrman with the correct number of addrs.
DataStream ssPeers2 = AddrmanToStream(addrman);
@@ -1130,17 +1126,14 @@ BOOST_AUTO_TEST_CASE(load_addrman_corrupted)
{
// Test that the de-serialization of corrupted peers.dat throws an exception.
auto ssPeers1{MakeCorruptPeersDat()};
- bool exceptionThrown = false;
AddrMan addrman1{EMPTY_NETGROUPMAN, !DETERMINISTIC, GetCheckRatio(m_node)};
BOOST_CHECK(addrman1.Size() == 0);
- try {
+ BOOST_CHECK_EXCEPTION([&]
+ {
unsigned char pchMsgTmp[4];
ssPeers1 >> pchMsgTmp;
ssPeers1 >> addrman1;
- } catch (const std::exception&) {
- exceptionThrown = true;
- }
- BOOST_CHECK(exceptionThrown);
+ }(), std::ios_base::failure, HasReason{"end of data"});
// Test that ReadFromStream fails if peers.dat is corrupt
auto ssPeers2{MakeCorruptPeersDat()};
### src/test/allocator_tests.cpp
@@ -4,16 +4,17 @@
#include <common/system.h>
#include <support/lockedpool.h>
+#include <test/util/common.h>
+#include <util/byte_units.h>
+
+#include <boost/test/unit_test.hpp>
#include <limits>
#include <memory>
#include <stdexcept>
-#include <util/byte_units.h>
#include <utility>
#include <vector>
-#include <boost/test/unit_test.hpp>
-
BOOST_AUTO_TEST_SUITE(allocator_tests)
BOOST_AUTO_TEST_CASE(arena_tests)
@@ -36,12 +37,7 @@ BOOST_AUTO_TEST_CASE(arena_tests)
#endif
BOOST_CHECK(b.stats().used == 0);
BOOST_CHECK(b.stats().free == synth_size);
- try { // Test exception on double-free
- b.free(chunk);
- BOOST_CHECK(0);
- } catch(std::runtime_error &)
- {
- }
+ BOOST_CHECK_EXCEPTION(b.free(chunk), std::runtime_error, HasReason{"Arena: invalid or double free"});
void *a0 = b.alloc(128);
void *a1 = b.alloc(256);
@@ -225,12 +221,7 @@ BOOST_AUTO_TEST_CASE(lockedpool_tests_live)
BOOST_CHECK(*((uint32_t*)a0) == 0x1234);
pool.free(a0);
- try { // Test exception on double-free
- pool.free(a0);
- BOOST_CHECK(0);
- } catch(std::runtime_error &)
- {
- }
+ BOOST_CHECK_EXCEPTION(pool.free(a0), std::runtime_error, HasReason{"Arena: invalid or double free"});
// If more than one new arena was allocated for the above tests, something is wrong
BOOST_CHECK(pool.stats().total <= (initial.total + LockedPool::ARENA_SIZE));
// Usage must be back to where it started
### src/test/blockencodings_tests.cpp
@@ -454,16 +454,7 @@ BOOST_AUTO_TEST_CASE(TransactionsRequestDeserializationOverflowTest) {
WriteCompactSize(stream, req0.indexes[2]);
BlockTransactionsRequest req1;
- try {
- stream >> req1;
- // before patch: deserialize above succeeds and this check fails, demonstrating the overflow
- BOOST_CHECK(req1.indexes[1] < req1.indexes[2]);
- // this shouldn't be reachable before or after patch
- BOOST_CHECK(0);
- } catch(std::ios_base::failure &) {
- // deserialize should fail
- BOOST_CHECK(true); // Needed to suppress "Test case [...] did not check any assertions"
- }
+ BOOST_CHECK_EXCEPTION(stream >> req1, std::ios_base::failure, HasReason{"differential value overflow"});
}
BOOST_AUTO_TEST_SUITE_END()
### src/test/coins_tests.cpp
@@ -541,24 +541,14 @@ BOOST_AUTO_TEST_CASE(ccoins_serialization)
BOOST_CHECK_EQUAL(cc3.out.scriptPubKey.size(), 0U);
// scriptPubKey that ends beyond the end of the stream
- try {
- Coin cc4;
- SpanReader{"000007"_hex} >> cc4;
- BOOST_CHECK_MESSAGE(false, "We should have thrown");
- } catch (const std::ios_base::failure&) {
- }
+ BOOST_CHECK_EXCEPTION(SpanReader{"000007"_hex} >> Coin{}, std::ios_base::failure, HasReason{"end of data"});
// Very large scriptPubKey (3*10^9 bytes) past the end of the stream
DataStream tmp{};
uint64_t x = 3000000000ULL;
tmp << VARINT(x);
BOOST_CHECK_EQUAL(HexStr(tmp), "8a95c0bb00");
- try {
- Coin cc5;
- SpanReader{"00008a95c0bb00"_hex} >> cc5;
- BOOST_CHECK_MESSAGE(false, "We should have thrown");
- } catch (const std::ios_base::failure&) {
- }
+ BOOST_CHECK_EXCEPTION(SpanReader{"00008a95c0bb00"_hex} >> Coin{}, std::ios_base::failure, HasReason{"end of data"});
}
const static COutPoint OUTPOINT;
### src/test/reverselock_tests.cpp
@@ -56,7 +56,7 @@ BOOST_AUTO_TEST_CASE(reverselock_errors)
g_debug_lockorder_abort = false;
// Make sure trying to reverse lock a previous lock fails
- BOOST_CHECK_EXCEPTION(REVERSE_LOCK(lock2, mutex2), std::logic_error, HasReason("mutex2 was not most recent critical section locked"));
+ BOOST_CHECK_EXCEPTION(REVERSE_LOCK(lock2, mutex2), std::logic_error, HasReason{"mutex2 was not most recent critical section locked"});
BOOST_CHECK(lock2.owns_lock());
g_debug_lockorder_abort = prev;
@@ -67,14 +67,9 @@ BOOST_AUTO_TEST_CASE(reverselock_errors)
BOOST_CHECK(!lock.owns_lock());
- bool failed = false;
- try {
- REVERSE_LOCK(lock, mutex);
- } catch(...) {
- failed = true;
- }
+ BOOST_CHECK_EXCEPTION(REVERSE_LOCK(lock, mutex), std::system_error,
+ [](const std::system_error& e) { return e.code() == std::errc::operation_not_permitted; });
- BOOST_CHECK(failed);
BOOST_CHECK(!lock.owns_lock());
// Locking the original lock after it has been taken by a reverse lock
@@ -88,7 +83,6 @@ BOOST_AUTO_TEST_CASE(reverselock_errors)
BOOST_CHECK(!lock.owns_lock());
}
- BOOST_CHECK(failed);
BOOST_CHECK(lock.owns_lock());
}
### src/test/streams_tests.cpp
@@ -376,15 +376,8 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file)
}
file.seek(0, SEEK_SET);
- // The buffer size (second arg) must be greater than the rewind
- // amount (third arg).
- try {
- BufferedFile bfbad{file, 25, 25};
- BOOST_CHECK(false);
- } catch (const std::exception& e) {
- BOOST_CHECK(strstr(e.what(),
- "Rewind limit must be less than buffer size") != nullptr);
- }
+ // The buffer size must be greater than the rewind amount.
+ BOOST_CHECK_EXCEPTION((BufferedFile{file, /*nBufSize=*/25, /*nRewindIn=*/25}), std::ios_base::failure, HasReason{"Rewind limit must be less than buffer size"});
// The buffer is 25 bytes, allow rewinding 10 bytes.
BufferedFile bf{file, 25, 10};
@@ -414,13 +407,7 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file)
// extent. The current file offset is 3, so the following
// SetLimit() allows zero bytes to be read.
BOOST_CHECK(bf.SetLimit(3));
- try {
- bf >> i;
- BOOST_CHECK(false);
- } catch (const std::exception& e) {
- BOOST_CHECK(strstr(e.what(),
- "Attempt to position past buffer limit") != nullptr);
- }
+ BOOST_CHECK_EXCEPTION(bf >> i, std::ios_base::failure, HasReason{"Attempt to position past buffer limit"});
// The default argument removes the limit completely.
BOOST_CHECK(bf.SetLimit());
// The read position should still be at 3 (no change).
@@ -467,13 +454,7 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file)
BOOST_CHECK_EQUAL(bf.GetPos(), 40U);
// We've read the entire file, the next read should throw.
- try {
- bf >> i;
- BOOST_CHECK(false);
- } catch (const std::exception& e) {
- BOOST_CHECK(strstr(e.what(),
- "BufferedFile::Fill: end of file") != nullptr);
- }
+ BOOST_CHECK_EXCEPTION(bf >> i, std::ios_base::failure, HasReason{"BufferedFile::Fill: end of file"});
// Attempting to read beyond the end sets the EOF indicator.
BOOST_CHECK(bf.eof());
@@ -529,12 +510,7 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file_skip)
// SkipTo() honors the transfer limit; we can't position beyond the limit.
bf.SetLimit(13);
- try {
- bf.SkipTo(14);
- BOOST_CHECK(false);
- } catch (const std::exception& e) {
- BOOST_CHECK(strstr(e.what(), "Attempt to position past buffer limit") != nullptr);
- }
+ BOOST_CHECK_EXCEPTION(bf.SkipTo(14), std::ios_base::failure, HasReason{"Attempt to position past buffer limit"});
// We can position exactly to the transfer limit.
bf.SkipTo(13);
### src/test/sync_tests.cpp
@@ -18,19 +18,14 @@ void TestPotentialDeadLockDetected(MutexType& mutex1, MutexType& mutex2)
LOCK2(mutex1, mutex2);
}
BOOST_CHECK(LockStackEmpty());
- bool error_thrown = false;
- try {
+ {
+#ifdef DEBUG_LOCKORDER
+ BOOST_CHECK_EXCEPTION(LOCK2(mutex2, mutex1), std::logic_error, HasReason{"potential deadlock detected: mutex1 -> mutex2 -> mutex1"});
+#else
LOCK2(mutex2, mutex1);
- } catch (const std::logic_error& e) {
- BOOST_CHECK_EQUAL(e.what(), "potential deadlock detected: mutex1 -> mutex2 -> mutex1");
- error_thrown = true;
+#endif
}
BOOST_CHECK(LockStackEmpty());
- #ifdef DEBUG_LOCKORDER
- BOOST_CHECK(error_thrown);
- #else
- BOOST_CHECK(!error_thrown);
- #endif
}
#ifdef DEBUG_LOCKORDER
### src/test/threadpool_tests.cpp
@@ -381,11 +381,7 @@ BOOST_AUTO_TEST_CASE(start_mid_stop_does_not_deadlock)
// Restart the pool and resume workers so the stopper thread can proceed.
// This will throw an exception only if the pool handles Start-Stop race properly,
// otherwise it will proceed and hang the stopper_thread.
- try {
- threadPool.Start(NUM_WORKERS_DEFAULT);
- } catch (std::exception& e) {
- BOOST_CHECK_EQUAL(e.what(), "Thread pool has been interrupted or is stopping");
- }
+ BOOST_CHECK_EXCEPTION(threadPool.Start(NUM_WORKERS_DEFAULT), std::runtime_error, HasReason{"Thread pool has been interrupted or is stopping"});
workers_blocker.release(NUM_WORKERS_DEFAULT);
WAIT_FOR(blocking_tasks);
### src/wallet/test/wallet_transaction_tests.cpp
@@ -47,12 +47,7 @@ BOOST_AUTO_TEST_CASE(deserialize_rejects_mismatched_variant_txid)
// A variant whose txid doesn't match the canonical txid must be rejected.
std::map<Wtxid, CTransactionRef> bad_variants{{tx_b->GetWitnessHash(), tx_b}};
- try {
- CWalletTx(deserialize, ss, bad_variants);
- BOOST_FAIL("expected std::runtime_error was not thrown");
- } catch (const std::runtime_error& e) {
- BOOST_CHECK_EQUAL(std::string(e.what()), "variant txid does not match wallet txid");
- }
+ BOOST_CHECK_EXCEPTION(CWalletTx(deserialize, ss, bad_variants), std::runtime_error, HasReason{"variant txid does not match wallet txid"});
}
BOOST_AUTO_TEST_SUITE_END()Why this scored 15/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.