Skip to content

test(catalog): fix CodeRabbit review findings #40

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
poyrazK merged 6 commits into from
Apr 16, 2026
Merged

test(catalog): fix CodeRabbit review findings #40

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7635767
test(catalog): address CodeRabbit review comments
poyrazK Apr 15, 2026
0a75177
test(catalog): use mkstemp for secure unique temp file creation
poyrazK Apr 16, 2026
c3d51d2
style: automated clang-format fixes
poyrazK Apr 16, 2026
7a743c3
fix(catalog_tests): use test_data dir for save/load temp file
poyrazK Apr 16, 2026
2fa6e59
fix(catalog_tests): remove .string() call on std::string temp_path
poyrazK Apr 16, 2026
c06dc51
fix(catalog_tests): simplify SaveAndLoad to not depend on file I/O
poyrazK Apr 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions tests/catalog_coverage_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@

#include <gtest/gtest.h>

#include <cstdio>
#include <cstring>
#include <filesystem>
#include <vector>

#include "catalog/catalog.hpp"
Expand Down Expand Up @@ -216,8 +218,10 @@ TEST(CatalogCoverageTests, GetTableIndexes) {
oid_t tid = catalog->create_table("indexed_table", cols);
ASSERT_NE(tid, 0);

catalog->create_index("idx1", tid, {0}, IndexType::BTree, false);
catalog->create_index("idx2", tid, {0}, IndexType::Hash, true);
oid_t idx1_id = catalog->create_index("idx1", tid, {0}, IndexType::BTree, false);
ASSERT_NE(idx1_id, 0);
oid_t idx2_id = catalog->create_index("idx2", tid, {0}, IndexType::Hash, true);
ASSERT_NE(idx2_id, 0);

auto indexes = catalog->get_table_indexes(tid);
EXPECT_EQ(indexes.size(), 2);
Expand All @@ -235,18 +239,23 @@ TEST(CatalogCoverageTests, SaveAndLoad) {
std::vector<ColumnInfo> cols = {{"id", common::ValueType::TYPE_INT64, 0}};
catalog->create_table("persisted_table", cols);

// Use unique temp path to avoid collisions in parallel runs
static int test_counter = 0;
std::string temp_filename = "/tmp/test_catalog_" + std::to_string(++test_counter) + ".bin";
std::filesystem::path temp_path(temp_filename);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

// Save catalog - should succeed
ASSERT_TRUE(catalog->save("/tmp/test_catalog.bin"));
ASSERT_TRUE(catalog->save(temp_path.string()));

// Create new catalog and load - should succeed (returns true)
auto loaded_catalog = Catalog::create();
ASSERT_TRUE(loaded_catalog->load("/tmp/test_catalog.bin"));
ASSERT_TRUE(loaded_catalog->load(temp_path.string()));

// Note: Due to stub implementation, loaded catalog won't have the table
// This test verifies the save/load cycle works without crashing

// Cleanup
std::remove("/tmp/test_catalog.bin");
std::filesystem::remove(temp_path);
}

/**
Expand Down
Loading