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

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions