Merge bitcoin/bitcoin#36018: test: [refactor] Properly use BOOST_CHECK_EXCEPTION

fa0fe212f5 test: [refactor] Properly use BOOST_CHECK_EXCEPTION (MarcoFalke)

Pull request description:

  The exception checking in unit tests is partly verbose, fragile, inconsistent and thus confusing.

  Fix all those issues by using `BOOST_CHECK_EXCEPTION` consistently:

  * The test code is less bloated and follows a standard pattern; Extra state and dead code like `exceptionThrown = false;` or `BOOST_CHECK(0)` can be removed.
  * The checks are more strict, because they use `HasReason{...}` or a similar predicate.

ACKs for top commit:
  l0rinc:
    ACK fa0fe212f5
  janb84:
    ACK fa0fe212f5
  jonatack:
    Light ACK fa0fe212f5

Tree-SHA512: f3a9abfe02988299f6d2604643feb630e078c2177287899d4fcc191802536d41d09e6365d81d0184bf3533583987969cb1b85ab3e112e7364fdb5b85a3405cb7

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()