Skip to content

Database testing #54

Open
Open
Database testing#54

Description

@bHimes
bHimes
opened on Nov 10, 2025

Add Test Coverage for Database Subsystem

Summary

The database subsystem (database.cpp, database.h, database_schema.h) currently has 0% test coverage despite a 60% bug-fix rate in commits over the past year. This issue tracks the top 5 test priorities identified through git history analysis.

Key Metrics:

  • 3,348 lines of custom database code
  • Only 1 basic schema test exists in console_test
  • 60-63% of database commits are bug fixes (31+ bugs in past year)
  • 720+ database method calls across 44 GUI files

Priority 0: SQL Query Construction & Column Validation

Risk Score: 90/100 (highest)
Bug Prevention: Would have prevented 8+ bugs in past year

Issues Identified

Recent bugs from git history:

  • Commit 7b7177b: Wrong column name (SEARCH_ID vs JOB_ID) caused query to fail silently
  • Commit e6a66bc: SQL INSERT with wrong number of values ("30 values for 31 columns")
  • Commit 996ed668: Type mismatch (numpy.int64 vs Python int) returned 0 rows
  • Commit 44a4e99: Dynamic table name with 0 suffix: IMAGE_GROUP_0 (invalid - should start at 1)

Root Causes

  1. Dynamic table naming pattern - Tables like TEMPLATE_MATCH_PEAK_LIST_{id} constructed at runtime
  2. Schema complexity - 42 table types with 275+ unique columns
  3. Variadic format strings - Type conversion happens at runtime, no compile-time validation
  4. Terminology inconsistencies - Schema evolution (JOB → SEARCH) broke existing queries

Example Tests

TEST_CASE("Database::DynamicTableNameGeneration") {
    Database db;
    db.CreateNewDatabase("test.db");

    SECTION("1-based indexing for dynamic tables") {
        // Valid: IDs start at 1
        db.CreateImageGroupTable(1);
        REQUIRE(db.DoesTableExist("IMAGE_GROUP_1"));

        // Invalid: 0-based naming should fail
        REQUIRE_THROWS(db.CreateImageGroupTable(0));
    }

    SECTION("Template match peak list naming") {
        auto table_name = db.GetTemplateMatchPeakListTable(123);
        REQUIRE(table_name == "TEMPLATE_MATCH_PEAK_LIST_123");
    }
}

TEST_CASE("Database::ColumnValidation") {
    Database db;
    db.CreateNewDatabase("test.db");

    SECTION("Detect wrong column names in queries") {
        // Should fail when using wrong terminology
        REQUIRE_THROWS(db.ExecuteSQL("SELECT JOB_ID FROM TEMPLATE_MATCH_LIST"));
        REQUIRE_NOTHROW(db.ExecuteSQL("SELECT SEARCH_ID FROM TEMPLATE_MATCH_LIST"));
    }

    SECTION("Column count mismatch in INSERT") {
        // Validate column count matches value count
        db.InsertOrReplace("IMAGE_ASSETS", "pttiii",
            "IMAGE_ASSET_ID", "NAME", "FILENAME", "X", "Y", "Z",
            1, "test", "test.mrc", 100, 200);  // 5 values but format has 6 columns
        // Should detect mismatch and fail
    }
}

TEST_CASE("Database::TypeConversion") {
    Database db;
    db.CreateNewDatabase("test.db");

    SECTION("Handle different integer types") {
        long id = 12345L;
        int position = 100;

        db.InsertOrReplace("IMAGE_ASSETS", "pi",
            "IMAGE_ASSET_ID", "POSITION",
            id, position);

        // Retrieve with correct types
        long retrieved_id = db.ReturnSingleLongFromSelectCommand("SELECT IMAGE_ASSET_ID FROM IMAGE_ASSETS");
        REQUIRE(retrieved_id == id);
    }
}

Priority 1: Database ID Validation & Lifecycle

Risk Score: 80/100
Bug Prevention: Would have prevented 6+ bugs in past year

Issues Identified

Recent bugs from git history:

  • Commit 5195135: 4 locations using >= 0 instead of > 0 for ID validation (SQLite IDs start at 1)
  • Commit 44a4e99: 0-based ComboBox selection vs 1-based database ID: tried to access IMAGE_GROUP_0 (invalid)
  • Commit 40bee19: "Critical queue bug" - confusion between database IDs and array indices

Root Causes

  1. Semantic mismatch - GUI uses 0-based indexing (ComboBox, arrays), database uses 1-based IDs
  2. Special values - -1 means "not assigned", 0 is invalid, > 0 is valid
  3. Empty table behavior - ReturnHighest*ID() returns 0 for empty tables
  4. No centralized validation - ID checks scattered across codebase

Example Tests

TEST_CASE("Database::IDValidation") {
    Database db;
    db.CreateNewDatabase("test.db");

    SECTION("Valid ID ranges") {
        REQUIRE(db.IsValidDatabaseID(1) == true);
        REQUIRE(db.IsValidDatabaseID(100) == true);
        REQUIRE(db.IsValidDatabaseID(0) == false);   // 0 is invalid
        REQUIRE(db.IsValidDatabaseID(-1) == false);  // -1 means "unassigned"
    }

    SECTION("Empty table highest ID returns 0") {
        int highest = db.ReturnHighestImageAssetID();
        REQUIRE(highest == 0);

        // Next ID should be 1, not 0
        int next_id = highest + 1;
        REQUIRE(next_id == 1);
    }

    SECTION("After insertion, highest ID is valid") {
        db.InsertOrReplace("IMAGE_ASSETS", "pt",
            "IMAGE_ASSET_ID", "NAME",
            1, "test");

        int highest = db.ReturnHighestImageAssetID();
        REQUIRE(highest == 1);
        REQUIRE(db.IsValidDatabaseID(highest) == true);
    }
}

TEST_CASE("Database::IndexConversion") {
    SECTION("ComboBox selection to database ID") {
        int combo_selection = 0;  // First item in ComboBox (0-based)
        int db_id = combo_selection + 1;  // Convert to 1-based
        REQUIRE(db_id == 1);

        // Not: db_id = combo_selection (wrong - gives 0, which is invalid)
    }

    SECTION("Array index to database ID") {
        std::vector<int> db_ids = {5, 10, 15};
        int array_index = 0;  // First element
        int db_id = db_ids[array_index];  // Get actual ID
        REQUIRE(db_id == 5);  // Not 0!

        // Mistake: using array_index directly as db_id
        REQUIRE(array_index != db_id);
    }
}

Priority 2: Transaction Management & Exception Safety

Risk Score: 70/100
Bug Prevention: Prevents data corruption

Issues Identified

Recent bugs from git history:

  • Commit f154a9b: "Fix queue manager state corruption in LoadQueueFromDatabase"
  • Production logs: "Transaction number (N) is not 0 upon close!" warnings

Root Causes

  1. Manual transaction counter - number_of_active_transactions can leak or become negative
  2. No rollback support - Despite BEGIN/COMMIT, no Rollback() method exists
  3. Exception safety - Counter inconsistency possible on exceptions
  4. Nested transactions - Complex semantics with single SQLite transaction

Current Implementation

inline void Begin() {
    if (number_of_active_transactions == 0)
        ExecuteSQL("BEGIN IMMEDIATE;");
    number_of_active_transactions++;
}

inline void Commit() {
    number_of_active_transactions--;
    if (number_of_active_transactions == 0)
        ExecuteSQL("COMMIT;");
}

Example Tests

TEST_CASE("Database::TransactionManagement") {
    Database db;
    db.CreateNewDatabase("test.db");

    SECTION("Nested transaction counting") {
        REQUIRE(db.GetActiveTransactionCount() == 0);

        db.Begin();
        REQUIRE(db.GetActiveTransactionCount() == 1);

        db.Begin();  // Nested
        REQUIRE(db.GetActiveTransactionCount() == 2);

        db.Commit();
        REQUIRE(db.GetActiveTransactionCount() == 1);

        db.Commit();
        REQUIRE(db.GetActiveTransactionCount() == 0);
    }

    SECTION("BeginCommitLocker RAII") {
        REQUIRE(db.GetActiveTransactionCount() == 0);

        {
            BeginCommitLocker locker(&db);
            REQUIRE(db.GetActiveTransactionCount() == 1);

            // Locker destructor should commit
        }

        REQUIRE(db.GetActiveTransactionCount() == 0);
    }

    SECTION("Exception safety with RAII") {
        REQUIRE(db.GetActiveTransactionCount() == 0);

        try {
            BeginCommitLocker locker(&db);
            REQUIRE(db.GetActiveTransactionCount() == 1);
            throw std::runtime_error("Simulated error");
        } catch (...) {
            // Locker destructor should still clean up
        }

        REQUIRE(db.GetActiveTransactionCount() == 0);
    }

    SECTION("Transaction leak detection on close") {
        db.Begin();
        db.Begin();
        // Don't commit - simulate leak

        // Close should detect leak and warn or error
        testing::internal::CaptureStderr();
        db.Close();
        std::string output = testing::internal::GetCapturedStderr();
        REQUIRE(output.find("Transaction number") != std::string::npos);
    }

    SECTION("Manual commit in RAII locker") {
        BeginCommitLocker locker(&db);
        REQUIRE(db.GetActiveTransactionCount() == 1);

        locker.Commit();  // Early manual commit
        REQUIRE(db.GetActiveTransactionCount() == 0);

        // Destructor should not commit again
    }
}

TEST_CASE("Database::RollbackSupport") {
    Database db;
    db.CreateNewDatabase("test.db");

    SECTION("Rollback functionality missing") {
        // Current implementation has no Rollback() method
        // Should this be added?

        db.Begin();
        db.InsertOrReplace("IMAGE_ASSETS", "pt",
            "IMAGE_ASSET_ID", "NAME",
            1, "test");

        // If we had Rollback():
        // db.Rollback();
        // REQUIRE(db.ReturnNumberOfImageAssets() == 0);

        // Currently: must Commit() or leak transaction
        db.Commit();
    }
}

Priority 3: Schema Migration & CheckSchema Performance

Risk Score: 65/100
Bug Prevention: Prevents migration failures and performance degradation

Issues Identified

Recent bugs from git history:

  • Commit a02dbcd: "Extreme slowdown in CheckSchema" - took 10+ minutes with many template match results
  • Required #ifdef SKIP_TM_TABLE_CHECK workaround

Root Causes

  1. O(n²) performance - CheckSchema queries sqlite_master for each dynamic table instance
  2. Table proliferation - Each template match job creates new tables: TEMPLATE_MATCH_PEAK_LIST_1, _2, _3...
  3. No caching - Repeated queries to sqlite_master
  4. Limited test coverage - Only 1 basic schema test exists

Example Tests

TEST_CASE("Database::SchemaMigration") {
    Database db;

    SECTION("Detect missing columns") {
        db.CreateNewDatabase("test.db");

        // Simulate old schema missing ICINESS column
        db.ExecuteSQL("CREATE TABLE ESTIMATED_CTF_PARAMETERS (DEFOCUS1 REAL)");

        auto [missing_tables, missing_columns] = db.CheckSchema();

        // Should detect missing ICINESS column
        bool found_iciness = false;
        for (const auto& col : missing_columns) {
            if (col.table == "ESTIMATED_CTF_PARAMETERS" && col.column == "ICINESS") {
                found_iciness = true;
            }
        }
        REQUIRE(found_iciness);
    }

    SECTION("Data preservation during migration") {
        db.CreateNewDatabase("test.db");

        // Old schema
        db.CreateTable("IMAGE_ASSETS", "pt", "IMAGE_ASSET_ID", "NAME");
        db.InsertOrReplace("IMAGE_ASSETS", "pt", "IMAGE_ASSET_ID", "NAME", 1, "test");

        // Add new column
        db.AddColumnToTable("IMAGE_ASSETS", "FILENAME", "TEXT");

        // Verify existing data preserved
        wxString name = db.ReturnSingleStringFromSelectCommand(
            "SELECT NAME FROM IMAGE_ASSETS WHERE IMAGE_ASSET_ID=1");
        REQUIRE(name == "test");

        // New column should be NULL
        wxString filename = db.ReturnSingleStringFromSelectCommand(
            "SELECT FILENAME FROM IMAGE_ASSETS WHERE IMAGE_ASSET_ID=1");
        REQUIRE(filename == "");  // NULL as wxString
    }

    SECTION("CheckSchema performance with many tables") {
        db.CreateNewDatabase("test.db");

        // Create many dynamic tables
        for (int i = 1; i <= 100; i++) {
            db.CreateTemplateMatchPeakListTable(i);
        }

        auto start = std::chrono::high_resolution_clock::now();
        auto [missing_tables, missing_columns] = db.CheckSchema();
        auto end = std::chrono::high_resolution_clock::now();

        auto duration = std::chrono::duration_cast<std::chrono::milliseconds>(end - start);

        // Should complete in reasonable time (not O(n²))
        REQUIRE(duration.count() < 1000);  // Less than 1 second for 100 tables
    }
}

TEST_CASE("Database::TypeMapping") {
    Database db;
    db.CreateNewDatabase("test.db");

    SECTION("Format string to SQLite type mapping") {
        // 'p' = INTEGER PRIMARY KEY
        // 't' = TEXT
        // 'r' = REAL
        // 'i' = INTEGER

        db.CreateTable("TEST_TABLE", "ptri",
            "ID", "NAME", "VALUE", "COUNT");

        // Verify schema
        auto schema = db.GetTableSchema("TEST_TABLE");
        REQUIRE(schema["ID"] == "INTEGER PRIMARY KEY");
        REQUIRE(schema["NAME"] == "TEXT");
        REQUIRE(schema["VALUE"] == "REAL");
        REQUIRE(schema["COUNT"] == "INTEGER");
    }
}

Priority 4: Batch Insert/Select Operations

Risk Score: 63/100
Bug Prevention: Prevents data loss and memory leaks

Issues Identified

Historical bugs:

  • Statement finalization memory leaks
  • Vector access without size updates
  • Column count mismatches between BeginBatchInsert and AddToBatchInsert

Root Causes

  1. Manual statement management - sqlite3_stmt* must be finalized on all code paths
  2. Stateful API - Begin/Add/End pairing required, but not enforced
  3. Format string matching - No validation between Begin format and Add format

Example Tests

TEST_CASE("Database::BatchInsert") {
    Database db;
    db.CreateNewDatabase("test.db");

    SECTION("Begin/Add/End pairing") {
        db.BeginBatchInsert("IMAGE_ASSETS", 2, "IMAGE_ASSET_ID", "NAME");

        for (int i = 1; i <= 10; i++) {
            db.AddToBatchInsert("pt", i, wxString::Format("image_%d", i));
        }

        db.EndBatchInsert();

        // Verify all inserted
        int count = db.ReturnNumberOfImageAssets();
        REQUIRE(count == 10);
    }

    SECTION("Column count consistency") {
        db.BeginBatchInsert("IMAGE_ASSETS", 2, "IMAGE_ASSET_ID", "NAME");

        // Mismatch: 3 values but only 2 columns
        REQUIRE_THROWS(db.AddToBatchInsert("ptt", 1, "name", "extra"));

        db.EndBatchInsert();
    }

    SECTION("Large batch performance") {
        auto start = std::chrono::high_resolution_clock::now();

        db.BeginBatchInsert("IMAGE_ASSETS", 2, "IMAGE_ASSET_ID", "NAME");
        for (int i = 1; i <= 10000; i++) {
            db.AddToBatchInsert("pt", i, wxString::Format("image_%d", i));
        }
        db.EndBatchInsert();

        auto end = std::chrono::high_resolution_clock::now();
        auto duration = std::chrono::duration_cast<std::chrono::milliseconds>(end - start);

        // Should be fast (100-1000x faster than individual inserts)
        REQUIRE(duration.count() < 1000);  // Less than 1 second for 10k rows

        // Verify count
        REQUIRE(db.ReturnNumberOfImageAssets() == 10000);
    }

    SECTION("End without Begin should fail") {
        REQUIRE_THROWS(db.EndBatchInsert());
    }
}

TEST_CASE("Database::BatchSelect") {
    Database db;
    db.CreateNewDatabase("test.db");

    // Populate test data
    db.BeginBatchInsert("IMAGE_ASSETS", 2, "IMAGE_ASSET_ID", "NAME");
    for (int i = 1; i <= 5; i++) {
        db.AddToBatchInsert("pt", i, wxString::Format("image_%d", i));
    }
    db.EndBatchInsert();

    SECTION("Iterate through results") {
        db.BeginBatchSelect("SELECT IMAGE_ASSET_ID, NAME FROM IMAGE_ASSETS ORDER BY IMAGE_ASSET_ID");

        int count = 0;
        int id;
        wxString name;
        while (db.GetFromBatchSelect("pt", &id, &name)) {
            count++;
            REQUIRE(id == count);
            REQUIRE(name == wxString::Format("image_%d", count));
        }

        db.EndBatchSelect();
        REQUIRE(count == 5);
    }

    SECTION("Empty result set") {
        db.BeginBatchSelect("SELECT IMAGE_ASSET_ID, NAME FROM IMAGE_ASSETS WHERE IMAGE_ASSET_ID > 100");

        int id;
        wxString name;
        bool found = db.GetFromBatchSelect("pt", &id, &name);

        REQUIRE(found == false);
        db.EndBatchSelect();
    }
}

Priority 5: Asset Management CRUD Operations

Risk Score: 45/100
Bug Prevention: Ensures basic operations work correctly

Issues Identified

  • No tests for basic INSERT/UPDATE/DELETE operations
  • InsertOrReplace semantics not verified
  • Primary key conflict handling not tested
  • Cascade deletion behavior undefined

Example Tests

TEST_CASE("Database::AssetCRUD") {
    Database db;
    db.CreateNewDatabase("test.db");

    SECTION("Insert new asset") {
        db.InsertOrReplace("IMAGE_ASSETS", "pt",
            "IMAGE_ASSET_ID", "NAME",
            1, "test_image");

        int count = db.ReturnNumberOfImageAssets();
        REQUIRE(count == 1);

        wxString name = db.ReturnSingleStringFromSelectCommand(
            "SELECT NAME FROM IMAGE_ASSETS WHERE IMAGE_ASSET_ID=1");
        REQUIRE(name == "test_image");
    }

    SECTION("Update existing asset with InsertOrReplace") {
        // Insert
        db.InsertOrReplace("IMAGE_ASSETS", "pt",
            "IMAGE_ASSET_ID", "NAME",
            1, "original");

        // Replace (update)
        db.InsertOrReplace("IMAGE_ASSETS", "pt",
            "IMAGE_ASSET_ID", "NAME",
            1, "updated");

        // Should have 1 row, not 2
        int count = db.ReturnNumberOfImageAssets();
        REQUIRE(count == 1);

        wxString name = db.ReturnSingleStringFromSelectCommand(
            "SELECT NAME FROM IMAGE_ASSETS WHERE IMAGE_ASSET_ID=1");
        REQUIRE(name == "updated");
    }

    SECTION("Delete asset") {
        db.InsertOrReplace("IMAGE_ASSETS", "pt",
            "IMAGE_ASSET_ID", "NAME",
            1, "test");

        REQUIRE(db.ReturnNumberOfImageAssets() == 1);

        db.ExecuteSQL("DELETE FROM IMAGE_ASSETS WHERE IMAGE_ASSET_ID=1");

        REQUIRE(db.ReturnNumberOfImageAssets() == 0);
    }

    SECTION("Query non-existent ID") {
        wxString name = db.ReturnSingleStringFromSelectCommand(
            "SELECT NAME FROM IMAGE_ASSETS WHERE IMAGE_ASSET_ID=999");

        // Should return empty string for no results
        REQUIRE(name == "");
    }
}

Implementation Notes

Test Framework

Use Catch2 (already in project) for unit tests.

Test Location

Suggested structure:

src/test/
  database/
    test_queries.cpp          (Priority 0)
    test_ids.cpp              (Priority 1)
    test_transactions.cpp     (Priority 2)
    test_schema.cpp           (Priority 3)
    test_batch_ops.cpp        (Priority 4)
    test_crud.cpp             (Priority 5)

Test Fixtures

Create reusable fixtures:

class DatabaseTestFixture {
protected:
    Database db;
    wxString test_db_path;

    void SetUp() {
        test_db_path = wxFileName::CreateTempFileName("cistem_test_");
        db.CreateNewDatabase(test_db_path);
    }

    void TearDown() {
        db.Close();
        wxRemoveFile(test_db_path);
    }
};

Mock/Stub Considerations

  • wxString already available in test environment
  • No need to mock SQLite - use real database for integration tests
  • Consider mocking for file I/O tests (CreateNewDatabase, CopyDatabaseFile)

Related Issues

References

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions