DatabaseManger - unit testing and reduce commits #3122

springfall2008 · 2025-12-25T17:10:35Z

This PR adds comprehensive unit tests for the DatabaseManager component and fixes a bug in the stop() method.

Changes

Delayed commit

Only call commit after the queue is empty to reduce number of writes

New Tests (apps/predbat/tests/test_db_manager.py)

test_db_manager_set_get_state: Tests set_state_db/get_state_db with attributes, custom timestamps, and timezone conversion (GMT+2 → GMT+0)
test_db_manager_entities_and_history: Tests get_all_entities_db and get_history_db with 10 hourly data points
test_db_manager_error_handling: Tests graceful handling of non-existent entities and IPC queue resilience
test_db_manager_persistence: Tests data persistence across database restart

Bug Fix (apps/predbat/db_manager.py)

Fixed stop() method to properly send 'stop' command through IPC queue before cleanup
Ensures the stop command handler (lines 81-83) is properly executed

Test Infrastructure

All tests use real threading and SQLite databases with temporary directories
Tests verify proper thread exit after stop command
Registered 4 new tests in TEST_REGISTRY

All tests passing in ~5 seconds.

Delay commit during queue of changes

Copilot

Pull request overview

This PR adds comprehensive unit tests for the DatabaseManager component and includes a bug fix in the stop() method plus refactoring of database commit operations.

Key Changes

Added 4 comprehensive unit tests covering set/get state operations, entity/history queries, error handling, and data persistence across restarts
Fixed stop() method to properly send stop command through IPC queue before cleanup
Refactored commit operations in db_engine.py by extracting _commit_db() method and implementing batch commits when queue is empty

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 12 comments.

File	Description
apps/predbat/tests/test_db_manager.py	New comprehensive test suite with 520 lines covering all DatabaseManager operations including threading, IPC, and SQLite persistence
apps/predbat/unit_test.py	Registered 4 new DatabaseManager tests in TEST_REGISTRY
apps/predbat/db_manager.py	Modified stop() method to send IPC stop command and sleep before setting api_started flag; added batch commit logic when queue is empty
apps/predbat/db_engine.py	Extracted _commit_db() method; removed immediate commits after INSERT operations to enable batch committing; added commit to _close()

Copilot · 2025-12-25T17:14:56Z

apps/predbat/db_engine.py

        state = str(state)

        # Put the entity_id into entities table if its not in already
        self.db_cursor.execute("INSERT OR IGNORE INTO entities (entity_name) VALUES (?)", (entity_id,))


Removing the commit after inserting a new entity creates a potential data consistency issue. If the process crashes or stops between inserting the entity and the eventual commit (which now only happens when the queue is empty), the entity_id won't be persisted but _get_entity_index_db on line 116 will return the index from the in-memory cache. This causes subsequent state inserts to reference a non-existent entity_index.

While batch committing improves performance, this specific INSERT OR IGNORE should be committed immediately to ensure entity metadata is persisted before state records reference it. Consider either: (1) keeping the commit here, or (2) clearing the entity_id_cache on rollback/restart to prevent stale references.

Suggested change

self.db_cursor.execute("INSERT OR IGNORE INTO entities (entity_name) VALUES (?)", (entity_id,))

self.db_cursor.execute("INSERT OR IGNORE INTO entities (entity_name) VALUES (?)", (entity_id,))

# If a new entity was inserted, commit immediately to ensure it is persisted

if self.db_cursor.rowcount:

self._commit_db()

Copilot · 2025-12-25T17:14:57Z

apps/predbat/tests/test_db_manager.py

+        # Use GMT+2 timezone for testing timezone conversion
+        self.local_tz = pytz.timezone("Etc/GMT-2")  # GMT+2


The timezone comment "GMT+2" is misleading. pytz.timezone("Etc/GMT-2") actually represents GMT+2 (2 hours ahead of UTC), not GMT-2. This is due to POSIX convention where the sign is inverted. The comment on line 37 correctly states "GMT+2" but the code comment logic is confusing.

To avoid confusion, consider using a more explicit timezone like pytz.timezone("Europe/Athens") which is clearly UTC+2 (or UTC+3 during DST), or add a clarifying comment about the POSIX sign inversion.

Suggested change

# Use GMT+2 timezone for testing timezone conversion

self.local_tz = pytz.timezone("Etc/GMT-2") # GMT+2

# Use a clear UTC+2 timezone for testing timezone conversion

self.local_tz = pytz.timezone("Europe/Athens") # UTC+2 on 2025-12-25

Copilot · 2025-12-25T17:14:57Z

apps/predbat/db_manager.py

+                # Commit if the queue is empty
+                if not self.db_queue:
+                    if hasattr(self.db_engine, "_commit_db"):
+                        self.db_engine._commit_db()


The commit logic added at lines 85-88 doesn't execute when processing the "stop" command because the code checks if not self.db_queue after the stop command is processed but before the loop exits. Since the queue is empty after popping the stop command, this commit would execute. However, this creates a subtle bug: the commit happens after api_stop is set to True (line 82), but the loop condition while not self.api_stop: (line 57) will evaluate to False on the next iteration, skipping any remaining queue items.

Consider moving the commit check to occur before the stop command sets api_stop = True, or ensure that the final commit in _close() at line 95 is sufficient for cleanup. The current flow is: (1) process stop, (2) commit if queue empty, (3) exit loop, (4) call _close() which also commits. This double-commit is unnecessary.

Copilot · 2025-12-25T17:14:57Z

apps/predbat/db_manager.py

+        await asyncio.sleep(0.1)
        self.api_started = False


The sleep duration of 0.1 seconds may not be sufficient for the stop command to be processed by the database thread. The IPC mechanism uses a queue and event-based system where the stop command needs to: (1) be added to the queue, (2) trigger the sync_event, (3) be processed by bridge_event, (4) wake the async loop, and (5) execute the stop handler.

Consider either increasing the sleep duration (e.g., to 0.3-0.5 seconds) to give the thread time to process, or better yet, use a proper wait mechanism that checks if the thread has actually stopped (e.g., wait for api_started to become False with a timeout).

Suggested change

await asyncio.sleep(0.1)

self.api_started = False

# Wait for the database manager loop to actually stop, with a timeout

timeout = 2.0 # seconds

poll_interval = 0.05 # seconds

start_time = time.monotonic()

while self.api_started and (time.monotonic() - start_time) < timeout:

await asyncio.sleep(poll_interval)

if self.api_started:

self.log("Warn: db_manager did not stop within timeout")

Copilot · 2025-12-25T17:14:58Z

apps/predbat/tests/test_db_manager.py

+            # Test 3: Set state with custom timestamp (test timezone conversion)
+            custom_timestamp = datetime(2025, 12, 25, 10, 0, 0, tzinfo=mock_base.local_tz)
+            entity_id2 = "sensor.test_solar"
+            state2 = "15.3"
+            attributes2 = {"unit_of_measurement": "kW"}
+
+            result2 = await loop.run_in_executor(None, db_mgr.set_state_db, entity_id2, state2, attributes2, custom_timestamp)
+            await asyncio.sleep(0.2)
+
+            retrieved2 = await loop.run_in_executor(None, db_mgr.get_state_db, entity_id2)
+            assert retrieved2 is not None, "get_state_db returned None for entity2"
+            assert retrieved2["state"] == state2, f"Expected state {state2}, got {retrieved2['state']}"
+            assert retrieved2["attributes"]["unit_of_measurement"] == "kW", "Attribute mismatch for entity2"
+            print(f"✓ Custom timestamp handled correctly: {retrieved2}")


The test description states "timezone conversion (GMT+2 → GMT+0)" but the actual test doesn't verify timezone conversion is working correctly. The custom timestamp at line 122 is created with mock_base.local_tz timezone, and line 160 in db_manager.py shows that set_state_db converts it to GMT+0 by subtracting the UTC offset. However, the test at line 130-134 only verifies the state and attributes are retrieved - it doesn't verify the timestamp was converted correctly.

Consider adding an assertion that checks the timestamp in the retrieved state matches the expected GMT+0 time, not the original GMT+2 time.

Copilot · 2025-12-25T17:14:59Z

apps/predbat/db_manager.py

        self.api_stop = True
        self.send_via_ipc("stop", {}, expect_response=False)
+        await asyncio.sleep(0.1)
        self.api_started = False


Setting api_started = False immediately after sending the stop command and sleeping creates a race condition. The async event loop should set this flag to False (line 97 in start() method) after completing cleanup. Setting it prematurely here means:

Tests might incorrectly think the thread has exited when it's still processing

Other code checking api_started will see False before the database connection is actually closed

The thread itself sets it to False at line 97, making this redundant

Remove line 132 and rely on the thread's own cleanup to set api_started = False after proper shutdown.

Suggested change

self.api_started = False

Copilot · 2025-12-25T17:14:59Z

apps/predbat/tests/test_db_manager.py

+            state2 = "15.3"
+            attributes2 = {"unit_of_measurement": "kW"}
+
+            result2 = await loop.run_in_executor(None, db_mgr.set_state_db, entity_id2, state2, attributes2, custom_timestamp)


Variable result2 is not used.

Suggested change

result2 = await loop.run_in_executor(None, db_mgr.set_state_db, entity_id2, state2, attributes2, custom_timestamp)

await loop.run_in_executor(None, db_mgr.set_state_db, entity_id2, state2, attributes2, custom_timestamp)

Copilot · 2025-12-25T17:14:59Z

apps/predbat/tests/test_db_manager.py

+                    try:
+                        # Remove 'Z' suffix if present
+                        ts = timestamp_str.rstrip("Z")
+                        parsed = datetime.strptime(ts, TIME_FORMAT_DB)


Variable parsed is not used.

Suggested change

parsed = datetime.strptime(ts, TIME_FORMAT_DB)

datetime.strptime(ts, TIME_FORMAT_DB)

Copilot · 2025-12-25T17:14:59Z

apps/predbat/tests/test_db_manager.py

+        print(f"[TEST LOG] {message}")
+
+
+class MockDatabaseManager(DatabaseManager):


This class does not call ComponentBase.init during initialization. (MockDatabaseManager.init may be missing a call to a base class init)

Copilot · 2025-12-25T17:15:00Z

apps/predbat/tests/test_db_manager.py

+        return self.base.now_utc_exact
+
+    @property
+    def local_tz(self):


This method is shadowed by attribute local_tz in superclass ComponentBase. (read-only property may cause an error if written to in the superclass)

springfall2008 added 2 commits December 25, 2025 17:07

Database manager tests

ccc3f3a

Delay commit during queue of changes

CLeanup

ccf2c0b

Copilot AI review requested due to automatic review settings December 25, 2025 17:10

Copilot started reviewing on behalf of springfall2008 December 25, 2025 17:11 View session

Copilot AI reviewed Dec 25, 2025

View reviewed changes

springfall2008 added 3 commits December 25, 2025 18:19

Speedup test, add commit for new entity id

fc3b5e7

Commit on exit

3b2062e

Cleanup review feedback

665993f

springfall2008 changed the title ~~Add comprehensive unit tests for DatabaseManager~~ DatabaseManger - unit testing and reduce commits Dec 25, 2025

Add new index

575a5f3

springfall2008 mentioned this pull request Dec 25, 2025

Internal Database Disk IO improvements and CPU reduction #3114

Open

springfall2008 merged commit b66c580 into main Dec 25, 2025
1 check passed

springfall2008 deleted the fixes3 branch December 25, 2025 18:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

DatabaseManger - unit testing and reduce commits #3122

DatabaseManger - unit testing and reduce commits #3122

springfall2008 commented Dec 25, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 25, 2025

Uh oh!

Copilot AI Dec 25, 2025

Uh oh!

Copilot AI Dec 25, 2025

Uh oh!

Copilot AI Dec 25, 2025

Uh oh!

Copilot AI Dec 25, 2025

Uh oh!

Copilot AI Dec 25, 2025

Uh oh!

Copilot AI Dec 25, 2025

Uh oh!

Copilot AI Dec 25, 2025

Uh oh!

Copilot AI Dec 25, 2025

Uh oh!

Copilot AI Dec 25, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-        self.db_cursor.execute("INSERT OR IGNORE INTO entities (entity_name) VALUES (?)", (entity_id,))
+        self.db_cursor.execute("INSERT OR IGNORE INTO entities (entity_name) VALUES (?)", (entity_id,))
+        # If a new entity was inserted, commit immediately to ensure it is persisted
+        if self.db_cursor.rowcount:
+            self._commit_db()

		# Use GMT+2 timezone for testing timezone conversion
		self.local_tz = pytz.timezone("Etc/GMT-2") # GMT+2

-        await asyncio.sleep(0.1)
-        self.api_started = False
+        # Wait for the database manager loop to actually stop, with a timeout
+        timeout = 2.0  # seconds
+        poll_interval = 0.05  # seconds
+        start_time = time.monotonic()
+        while self.api_started and (time.monotonic() - start_time) < timeout:
+            await asyncio.sleep(poll_interval)
+        if self.api_started:
+            self.log("Warn: db_manager did not stop within timeout")

	result2 = await loop.run_in_executor(None, db_mgr.set_state_db, entity_id2, state2, attributes2, custom_timestamp)
	await loop.run_in_executor(None, db_mgr.set_state_db, entity_id2, state2, attributes2, custom_timestamp)

	parsed = datetime.strptime(ts, TIME_FORMAT_DB)
	datetime.strptime(ts, TIME_FORMAT_DB)

		print(f"[TEST LOG] {message}")


		class MockDatabaseManager(DatabaseManager):

Uh oh!

DatabaseManger - unit testing and reduce commits #3122

DatabaseManger - unit testing and reduce commits #3122

Conversation

springfall2008 commented Dec 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Delayed commit

New Tests (apps/predbat/tests/test_db_manager.py)

Bug Fix (apps/predbat/db_manager.py)

Test Infrastructure

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

springfall2008 commented Dec 25, 2025 •

edited

Loading