From 6dfa5bc66d40861146b3459a8de4383cba9ca6d4 Mon Sep 17 00:00:00 2001 From: Logan Cusano Date: Thu, 20 Aug 2026 03:06:41 -0400 Subject: [PATCH] fix: repair 10 stale tests in test_mqtt_handler.py and test_node_sweeper.py MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All 10 failures were tests that had drifted behind the product code, not regressions in it. Diagnosed each individually: test_mqtt_handler.py: - test_checkin_creates_new_node, test_checkin_new_node_defaults_lat_lon: unpacked 4 positional args from doc_set.call_args[0], but fstore.doc_set(collection, doc_id, data, merge=False) always passes merge as a kwarg, so only 3 positional args are ever recorded. Fixed the unpack to 3. - test_call_start_creates_call_doc, test_call_start_uses_now_when_started_at_missing: mocked fstore.doc_get, but _on_call_start looks the node up via the cached fstore.doc_get_cached (added when Firestore reads were cut to stay in the free tier). The unmocked doc_get_cached returned a bare MagicMock, which isn't awaitable. Mocked doc_get_cached instead; also fixed the same 4-vs-3 positional-arg unpack on doc_set's merge=False call. - test_call_end_updates_status_and_times, test_call_end_sets_audio_url_when_present: mocked fstore.doc_update, but _on_call_end now writes via fstore.doc_set(merge=True) (see the "Fix Upload 404 warning" commit — doc_update raised "No document to update" when call_end arrived before call_start). Also calls doc_get_cached to stamp org_id. Mocked doc_get_cached and asserted against doc_set instead of doc_update. test_node_sweeper.py: - test_stale_online_node_marked_offline, test_stale_recording_node_marked_offline, test_tz_naive_last_seen_is_handled, test_only_stale_nodes_updated_in_batch: _sweep() now calls app.routers.tokens.release_token(node_id) for every node it marks offline (added in 2a690ec, the PulseAudio/Discord-token work). These tests never mocked it, so the module-level patch("asyncio.to_thread", ...) meant for the node-query call leaked into release_token's own internal to_thread call, feeding it raw node dicts where it expected Firestore doc snapshots with .id — hence "AttributeError: 'dict' object has no attribute 'id'". Patched app.routers.tokens.release_token directly (it's imported inline inside _sweep, so patching the source module works); the batch test also now asserts release_token fires for exactly the two nodes that went offline. No product code changed — app/internal/mqtt_handler.py, app/routers/tokens.py, and app/internal/node_sweeper.py all behave as intended. This was pure test drift across two unrelated feature additions (Firestore-read caching, Discord-token release-on-offline) that landed without their tests being updated. 93 passed, 0 failed. Closes logan/server-26#10. Co-Authored-By: Claude Opus 5 --- drb-c2-core/tests/test_mqtt_handler.py | 33 ++++++++++++++++++-------- drb-c2-core/tests/test_node_sweeper.py | 24 +++++++++++++++---- 2 files changed, 43 insertions(+), 14 deletions(-) diff --git a/drb-c2-core/tests/test_mqtt_handler.py b/drb-c2-core/tests/test_mqtt_handler.py index e2fc6f9..cdbd063 100644 --- a/drb-c2-core/tests/test_mqtt_handler.py +++ b/drb-c2-core/tests/test_mqtt_handler.py @@ -67,7 +67,9 @@ async def test_checkin_creates_new_node(handler): ) mock_fstore.doc_set.assert_called_once() - _, _, doc, _ = mock_fstore.doc_set.call_args[0] + # doc_set(collection, doc_id, data, merge=False) — merge is passed as a + # kwarg in mqtt_handler.py, so only 3 positional args land in call_args[0]. + _, _, doc = mock_fstore.doc_set.call_args[0] assert doc["node_id"] == "new-node" assert doc["name"] == "Pi Zero W" assert doc["status"] == "unconfigured" @@ -84,7 +86,7 @@ async def test_checkin_new_node_defaults_lat_lon(handler): await handler._handle_checkin("new-node", {}) - _, _, doc, _ = mock_fstore.doc_set.call_args[0] + _, _, doc = mock_fstore.doc_set.call_args[0] assert doc["lat"] == 0.0 assert doc["lon"] == 0.0 @@ -200,13 +202,17 @@ async def test_call_start_creates_call_doc(handler): } with patch("app.internal.mqtt_handler.fstore") as mock_fstore: - mock_fstore.doc_get = AsyncMock(return_value=node) + # _on_call_start looks the node up via doc_get_cached (cached read, + # added to cut Firestore read volume — see doc_get_cached in + # app/internal/firestore.py), not the uncached doc_get. + mock_fstore.doc_get_cached = AsyncMock(return_value=node) mock_fstore.doc_set = AsyncMock() await handler._on_call_start("node-01", payload) mock_fstore.doc_set.assert_called_once() - _, _, doc, _ = mock_fstore.doc_set.call_args[0] + # doc_set(collection, doc_id, data, merge=False) — merge is a kwarg here too. + _, _, doc = mock_fstore.doc_set.call_args[0] assert doc["call_id"] == "call-abc123" assert doc["node_id"] == "node-01" assert doc["system_id"] == "sys-001" @@ -233,12 +239,12 @@ async def test_call_start_uses_now_when_started_at_missing(handler): payload = {"call_id": "call-xyz", "tgid": 99} with patch("app.internal.mqtt_handler.fstore") as mock_fstore: - mock_fstore.doc_get = AsyncMock(return_value=node) + mock_fstore.doc_get_cached = AsyncMock(return_value=node) mock_fstore.doc_set = AsyncMock() await handler._on_call_start("node-01", payload) - _, _, doc, _ = mock_fstore.doc_set.call_args[0] + _, _, doc = mock_fstore.doc_set.call_args[0] assert doc["started_at"] is not None @@ -250,11 +256,17 @@ async def test_call_end_updates_status_and_times(handler): } with patch("app.internal.mqtt_handler.fstore") as mock_fstore: - mock_fstore.doc_update = AsyncMock() + # _on_call_end writes via doc_set(merge=True) now, not doc_update — see + # the "Fix Upload 404 warning" commit: doc_update raised "No document + # to update" when call_end raced ahead of call_start, so it was + # switched to a merging doc_set. It also reads the node via the + # cached doc_get_cached to stamp org_id. + mock_fstore.doc_get_cached = AsyncMock(return_value=None) + mock_fstore.doc_set = AsyncMock() await handler._on_call_end("node-01", payload) - updates = mock_fstore.doc_update.call_args[0][2] + updates = mock_fstore.doc_set.call_args[0][2] assert updates["status"] == "ended" assert updates["ended_at"] is not None @@ -268,11 +280,12 @@ async def test_call_end_sets_audio_url_when_present(handler): } with patch("app.internal.mqtt_handler.fstore") as mock_fstore: - mock_fstore.doc_update = AsyncMock() + mock_fstore.doc_get_cached = AsyncMock(return_value=None) + mock_fstore.doc_set = AsyncMock() await handler._on_call_end("node-01", payload) - updates = mock_fstore.doc_update.call_args[0][2] + updates = mock_fstore.doc_set.call_args[0][2] assert updates["audio_url"] == "https://storage.example.com/call.mp3" diff --git a/drb-c2-core/tests/test_node_sweeper.py b/drb-c2-core/tests/test_node_sweeper.py index caeab98..4f06cfb 100644 --- a/drb-c2-core/tests/test_node_sweeper.py +++ b/drb-c2-core/tests/test_node_sweeper.py @@ -35,8 +35,16 @@ def _node_naive(node_id, status, age_seconds): async def test_stale_online_node_marked_offline(): nodes = [_node("node-01", "online", age_seconds=120)] + # A stale node also triggers app.routers.tokens.release_token(node_id) — + # added by the PulseAudio/Discord-token work (commit 2a690ec). It's + # imported inline inside _sweep, so it must be patched at its source + # module rather than relying on the global asyncio.to_thread patch above, + # which is scoped to the node-query call and would otherwise feed + # release_token's own internal to_thread call the wrong shape of data + # (raw node dicts instead of Firestore doc snapshots with .id). with patch("asyncio.to_thread", new=AsyncMock(return_value=nodes)), \ - patch("app.internal.node_sweeper.fstore") as mock_fstore: + patch("app.internal.node_sweeper.fstore") as mock_fstore, \ + patch("app.routers.tokens.release_token", new=AsyncMock()): mock_fstore.doc_update = AsyncMock() await _sweep() @@ -50,7 +58,8 @@ async def test_stale_recording_node_marked_offline(): nodes = [_node("node-02", "recording", age_seconds=200)] with patch("asyncio.to_thread", new=AsyncMock(return_value=nodes)), \ - patch("app.internal.node_sweeper.fstore") as mock_fstore: + patch("app.internal.node_sweeper.fstore") as mock_fstore, \ + patch("app.routers.tokens.release_token", new=AsyncMock()): mock_fstore.doc_update = AsyncMock() await _sweep() @@ -106,7 +115,8 @@ async def test_tz_naive_last_seen_is_handled(): nodes = [_node_naive("node-06", "online", age_seconds=120)] with patch("asyncio.to_thread", new=AsyncMock(return_value=nodes)), \ - patch("app.internal.node_sweeper.fstore") as mock_fstore: + patch("app.internal.node_sweeper.fstore") as mock_fstore, \ + patch("app.routers.tokens.release_token", new=AsyncMock()): mock_fstore.doc_update = AsyncMock() await _sweep() @@ -141,10 +151,16 @@ async def test_only_stale_nodes_updated_in_batch(): ] with patch("asyncio.to_thread", new=AsyncMock(return_value=nodes)), \ - patch("app.internal.node_sweeper.fstore") as mock_fstore: + patch("app.internal.node_sweeper.fstore") as mock_fstore, \ + patch("app.routers.tokens.release_token", new=AsyncMock()) as mock_release: mock_fstore.doc_update = AsyncMock() await _sweep() assert mock_fstore.doc_update.call_count == 2 updated_ids = {call.args[1] for call in mock_fstore.doc_update.call_args_list} assert updated_ids == {"node-08", "node-11"} + + # Both newly-offline nodes should have their Discord token freed. + assert mock_release.call_count == 2 + released_ids = {call.args[0] for call in mock_release.call_args_list} + assert released_ids == {"node-08", "node-11"}