test: refactor coverage tests for better resource management and deterministic network failure

Author: poyrazK

tests/transaction_coverage_tests.cpp

@@ -4,18 +4,17 @@
  */
 
 #include <gtest/gtest.h>
-
 #include <atomic>
 #include <chrono>
-#include <cstdio>
 #include <thread>
 #include <vector>
+#include <cstdio>
 
 #include "catalog/catalog.hpp"
 #include "common/config.hpp"
 #include "storage/buffer_pool_manager.hpp"
-#include "storage/heap_table.hpp"
 #include "storage/storage_manager.hpp"
+#include "storage/heap_table.hpp"
 #include "transaction/lock_manager.hpp"
 #include "transaction/transaction.hpp"
 #include "transaction/transaction_manager.hpp"
@@ -31,49 +30,51 @@ namespace {
  * @brief Fixture for transaction-related coverage tests to ensure proper resource management.
  */
 class TransactionCoverageTests : public ::testing::Test {
-   protected:
+protected:
     void SetUp() override {
-        catalog = Catalog::create();
-        disk_manager = std::make_unique<StorageManager>("./test_data");
-        bpm = std::make_unique<BufferPoolManager>(config::Config::DEFAULT_BUFFER_POOL_SIZE,
-                                                  *disk_manager);
-        lm = std::make_unique<LockManager>();
-        tm = std::make_unique<TransactionManager>(*lm, *catalog, *bpm, nullptr);
+        catalog_ptr = Catalog::create();
+        disk_manager_ptr = std::make_unique<StorageManager>("./test_data");
+        bpm_ptr = std::make_unique<BufferPoolManager>(config::Config::DEFAULT_BUFFER_POOL_SIZE, *disk_manager_ptr);
+        lm_ptr = std::make_unique<LockManager>();
+        tm_ptr = std::make_unique<TransactionManager>(*lm_ptr, *catalog_ptr, *bpm_ptr, nullptr);
 
         std::vector<ColumnInfo> cols = {{"id", common::ValueType::TYPE_INT64, 0},
                                         {"val", common::ValueType::TYPE_TEXT, 1}};
-        catalog->create_table("rollback_stress", cols);
-
+        catalog_ptr->create_table("rollback_stress", cols);
+        
         executor::Schema schema;
         schema.add_column("id", common::ValueType::TYPE_INT64);
         schema.add_column("val", common::ValueType::TYPE_TEXT);
-
-        table = std::make_unique<HeapTable>("rollback_stress", *bpm, schema);
-        table->create();
-
+        
+        table_ptr = std::make_unique<HeapTable>("rollback_stress", *bpm_ptr, schema);
+        table_ptr->create();
+        
         txn = nullptr;
     }
 
     void TearDown() override {
         if (txn != nullptr) {
-            tm->abort(txn);
+            tm_ptr->abort(txn);
         }
-        table.reset();
-        tm.reset();
-        lm.reset();
-        bpm.reset();
-        disk_manager.reset();
-        catalog.reset();
-
+        table_ptr.reset();
+        tm_ptr.reset();
+        lm_ptr.reset();
+        bpm_ptr.reset();
+        disk_manager_ptr.reset();
+        catalog_ptr.reset();
+        
         static_cast<void>(std::remove("./test_data/rollback_stress.heap"));
     }
 
-    std::unique_ptr<Catalog> catalog;
-    std::unique_ptr<StorageManager> disk_manager;
-    std::unique_ptr<BufferPoolManager> bpm;
-    std::unique_ptr<LockManager> lm;
-    std::unique_ptr<TransactionManager> tm;
-    std::unique_ptr<HeapTable> table;
+    // Pointers managed by the fixture
+    std::unique_ptr<Catalog> catalog_ptr;
+    std::unique_ptr<StorageManager> disk_manager_ptr;
+    std::unique_ptr<BufferPoolManager> bpm_ptr;
+    std::unique_ptr<LockManager> lm_ptr;
+    std::unique_ptr<TransactionManager> tm_ptr;
+    std::unique_ptr<HeapTable> table_ptr;
+    
+    // Live transaction pointer for cleanup
     Transaction* txn;
 };
 
@@ -88,7 +89,7 @@ TEST(TransactionCoverageTestsStandalone, LockManagerConcurrency) {
     std::atomic<bool> stop{false};
 
     Transaction writer_txn(100);
-
+    
     // Writers holds exclusive lock initially
     ASSERT_TRUE(lm.acquire_exclusive(&writer_txn, "RESOURCE"));
 
@@ -111,7 +112,7 @@ TEST(TransactionCoverageTestsStandalone, LockManagerConcurrency) {
 
     // Release writer lock, readers should proceed
     lm.unlock(&writer_txn, "RESOURCE");
-
+    
     // Wait for all readers to get the lock
     for (int i = 0; i < 50 && shared_granted.load() < num_readers; ++i) {
         std::this_thread::sleep_for(std::chrono::milliseconds(50));
@@ -126,46 +127,44 @@ TEST(TransactionCoverageTestsStandalone, LockManagerConcurrency) {
 
 /**
  * @brief Tests deep rollback functionality via the Undo Log.
+ * Uses the TransactionCoverageTests fixture for automated cleanup.
  */
 TEST_F(TransactionCoverageTests, DeepRollback) {
-    txn = tm->begin();
+    // Expose symbols to reuse existing test body logic
+    TransactionManager& tm = *tm_ptr;
+    HeapTable& table = *table_ptr;
 
+    txn = tm.begin();
+    
     // 1. Insert some data
-    auto rid1 = table->insert(
-        executor::Tuple({common::Value::make_int64(1), common::Value::make_text("A")}),
-        txn->get_id());
+    auto rid1 = table.insert(executor::Tuple({common::Value::make_int64(1), common::Value::make_text("A")}), txn->get_id());
     txn->add_undo_log(UndoLog::Type::INSERT, "rollback_stress", rid1);
-
-    auto rid2 = table->insert(
-        executor::Tuple({common::Value::make_int64(2), common::Value::make_text("B")}),
-        txn->get_id());
+    
+    auto rid2 = table.insert(executor::Tuple({common::Value::make_int64(2), common::Value::make_text("B")}), txn->get_id());
     txn->add_undo_log(UndoLog::Type::INSERT, "rollback_stress", rid2);
 
     // 2. Update data
-    table->remove(rid1, txn->get_id());  // Mark old version deleted
-    auto rid1_new = table->insert(
-        executor::Tuple({common::Value::make_int64(1), common::Value::make_text("A_NEW")}),
-        txn->get_id());
+    table.remove(rid1, txn->get_id()); // Mark old version deleted
+    auto rid1_new = table.insert(executor::Tuple({common::Value::make_int64(1), common::Value::make_text("A_NEW")}), txn->get_id());
     txn->add_undo_log(UndoLog::Type::UPDATE, "rollback_stress", rid1_new, rid1);
 
     // 3. Delete data
-    table->remove(rid2, txn->get_id());
+    table.remove(rid2, txn->get_id());
     txn->add_undo_log(UndoLog::Type::DELETE, "rollback_stress", rid2);
 
-    EXPECT_EQ(table->tuple_count(), 1U);  // rid1_new is active, rid1 and rid2 are logically deleted
+    EXPECT_EQ(table.tuple_count(), 1U); // rid1_new is active, rid1 and rid2 are logically deleted
 
     // 4. Abort
-    tm->abort(txn);
-    txn = nullptr;  // Marked as aborted and handled by TearDown if still set
+    tm.abort(txn);
+    txn = nullptr; // Marked as aborted and handled by TearDown if still set
 
     // 5. Verify restoration
-    EXPECT_EQ(table->tuple_count(),
-              0U);  // Inserted rows should be physically removed or logically invisible
-
+    EXPECT_EQ(table.tuple_count(), 0U); // Inserted rows should be physically removed or logically invisible
+    
     // The table should be empty because we aborted the inserts
-    auto iter = table->scan();
+    auto iter = table.scan();
     executor::Tuple t;
     EXPECT_FALSE(iter.next(t));
 }
 
-}  // namespace
+} // namespace

tests/utils_coverage_tests.cpp

@@ -4,10 +4,8 @@
  */
 
 #include <gtest/gtest.h>
-
 #include <string>
 #include <vector>
-
 #include "common/value.hpp"
 #include "network/rpc_client.hpp"
 
@@ -21,7 +19,7 @@ namespace {
  */
 TEST(UtilsCoverageTests, ValueEdgeCases) {
     // 1. Accessor failure paths
-    Value v_int(ValueType::TYPE_INT32);  // Default 0
+    Value v_int(ValueType::TYPE_INT32); // Default 0
     EXPECT_THROW(v_int.as_bool(), std::runtime_error);
     EXPECT_THROW(v_int.as_float64(), std::runtime_error);
     EXPECT_THROW(v_int.as_text(), std::runtime_error);
@@ -33,7 +31,7 @@ TEST(UtilsCoverageTests, ValueEdgeCases) {
     EXPECT_THROW(v_text.as_int32(), std::runtime_error);
 
     // 2. boundary to_* conversions
-    EXPECT_EQ(v_text.to_int64(), 0);  // Text doesn't auto-convert to int in to_int64()
+    EXPECT_EQ(v_text.to_int64(), 0); // Text doesn't auto-convert to int in to_int64()
     EXPECT_EQ(v_text.to_float64(), 0.0);
 
     Value v_null = Value::make_null();
@@ -43,7 +41,7 @@ TEST(UtilsCoverageTests, ValueEdgeCases) {
 
     Value v_f(1.23);
     EXPECT_EQ(v_f.to_int64(), 1);
-
+    
     // 3. Numeric check
     EXPECT_TRUE(v_int.is_numeric());
     EXPECT_TRUE(v_f.is_numeric());
@@ -61,41 +59,41 @@ TEST(UtilsCoverageTests, ValueComparisons) {
     Value v_text("A");
 
     // Equality
-    EXPECT_TRUE(v_int == v_float);  // Numeric equality
+    EXPECT_TRUE(v_int == v_float); // Numeric equality
     EXPECT_FALSE(v_int == v_text);
     EXPECT_FALSE(v_int == v_null);
 
     // Less than
-    EXPECT_FALSE(v_null < v_int);  // NULL is not less than anything
-    EXPECT_TRUE(v_int < v_null);   // non-NULL is less than NULL (by convention in this impl)
-
+    EXPECT_FALSE(v_null < v_int); // NULL is not less than anything
+    EXPECT_TRUE(v_int < v_null);  // non-NULL is less than NULL (by convention in this impl)
+    
     Value v_int_small(5);
     EXPECT_TRUE(v_int_small < v_float);
     EXPECT_FALSE(v_float < v_int_small);
 
     Value v_text_b("B");
     EXPECT_TRUE(v_text < v_text_b);
-
+    
     // Mixed numeric/non-numeric comparison
-    EXPECT_FALSE(v_int < v_text);
+    EXPECT_FALSE(v_int < v_text); 
 }
 
 /**
  * @brief Tests RpcClient behavior on connection failures.
- *
+ * 
  * We use localhost on a reserved/privileged port (1) to trigger an immediate
- * "Connection refused" from the OS. This provides a deterministic failure
- * without the multi-second timeouts associated with non-routable external
- * IP addresses (like 192.0.2.1), ensuring the CI pipeline remains fast while
+ * "Connection refused" from the OS. This provides a deterministic failure 
+ * without the multi-second timeouts associated with non-routable external 
+ * IP addresses (like 192.0.2.1), ensuring the CI pipeline remains fast while 
  * still covering the error handling logic in RpcClient.
  */
 TEST(UtilsCoverageTests, RpcClientFailure) {
     // Attempt to connect to an unreachable port on localhost
-    RpcClient client("127.0.0.1", 1);
-
+    RpcClient client("127.0.0.1", 1); 
+    
     EXPECT_FALSE(client.connect());
     EXPECT_FALSE(client.is_connected());
-
+    
     std::vector<uint8_t> resp;
     // These should return false as they require a valid connection
     EXPECT_FALSE(client.call(RpcType::AppendEntries, {1, 2, 3}, resp));
@@ -117,4 +115,4 @@ TEST(UtilsCoverageTests, ValueHash) {
     EXPECT_NE(hasher(v1), hasher(v_null));
 }
 
-}  // namespace
+} // namespace