Logan CusanoandClaude Opus 5
6dfa5bc66d
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 <noreply@anthropic.com>
167 lines
6.2 KiB
Python
167 lines
6.2 KiB
Python
"""
|
|
Unit tests for node_sweeper — datetime comparison logic and Firestore update gating.
|
|
Firestore and asyncio.to_thread are mocked — no real DB required.
|
|
"""
|
|
import pytest
|
|
import asyncio
|
|
from datetime import datetime, timezone, timedelta
|
|
from unittest.mock import AsyncMock, patch, MagicMock
|
|
from app.internal.node_sweeper import _sweep
|
|
|
|
|
|
def _node(node_id, status, age_seconds):
|
|
"""Helper: build a node dict with last_seen age_seconds ago (tz-aware)."""
|
|
return {
|
|
"node_id": node_id,
|
|
"status": status,
|
|
"last_seen": datetime.now(timezone.utc) - timedelta(seconds=age_seconds),
|
|
}
|
|
|
|
|
|
def _node_naive(node_id, status, age_seconds):
|
|
"""Helper: tz-naive last_seen (simulates some Firestore SDK versions)."""
|
|
return {
|
|
"node_id": node_id,
|
|
"status": status,
|
|
"last_seen": datetime.utcnow() - timedelta(seconds=age_seconds),
|
|
}
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Core sweep logic
|
|
# ---------------------------------------------------------------------------
|
|
|
|
@pytest.mark.asyncio
|
|
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.routers.tokens.release_token", new=AsyncMock()):
|
|
mock_fstore.doc_update = AsyncMock()
|
|
await _sweep()
|
|
|
|
mock_fstore.doc_update.assert_called_once_with(
|
|
"nodes", "node-01", {"status": "offline"}
|
|
)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
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.routers.tokens.release_token", new=AsyncMock()):
|
|
mock_fstore.doc_update = AsyncMock()
|
|
await _sweep()
|
|
|
|
mock_fstore.doc_update.assert_called_once_with(
|
|
"nodes", "node-02", {"status": "offline"}
|
|
)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_fresh_node_not_touched():
|
|
nodes = [_node("node-03", "online", age_seconds=10)]
|
|
|
|
with patch("asyncio.to_thread", new=AsyncMock(return_value=nodes)), \
|
|
patch("app.internal.node_sweeper.fstore") as mock_fstore:
|
|
mock_fstore.doc_update = AsyncMock()
|
|
await _sweep()
|
|
|
|
mock_fstore.doc_update.assert_not_called()
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_already_offline_node_skipped():
|
|
"""Offline nodes must be skipped even if last_seen is ancient."""
|
|
nodes = [_node("node-04", "offline", age_seconds=9999)]
|
|
|
|
with patch("asyncio.to_thread", new=AsyncMock(return_value=nodes)), \
|
|
patch("app.internal.node_sweeper.fstore") as mock_fstore:
|
|
mock_fstore.doc_update = AsyncMock()
|
|
await _sweep()
|
|
|
|
mock_fstore.doc_update.assert_not_called()
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_node_with_no_last_seen_skipped():
|
|
nodes = [{"node_id": "node-05", "status": "online", "last_seen": None}]
|
|
|
|
with patch("asyncio.to_thread", new=AsyncMock(return_value=nodes)), \
|
|
patch("app.internal.node_sweeper.fstore") as mock_fstore:
|
|
mock_fstore.doc_update = AsyncMock()
|
|
await _sweep()
|
|
|
|
mock_fstore.doc_update.assert_not_called()
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Timezone edge cases
|
|
# ---------------------------------------------------------------------------
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_tz_naive_last_seen_is_handled():
|
|
"""Firestore may return tz-naive datetimes; sweeper must not crash."""
|
|
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.routers.tokens.release_token", new=AsyncMock()):
|
|
mock_fstore.doc_update = AsyncMock()
|
|
await _sweep()
|
|
|
|
mock_fstore.doc_update.assert_called_once_with(
|
|
"nodes", "node-06", {"status": "offline"}
|
|
)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_tz_naive_fresh_node_not_touched():
|
|
nodes = [_node_naive("node-07", "online", age_seconds=5)]
|
|
|
|
with patch("asyncio.to_thread", new=AsyncMock(return_value=nodes)), \
|
|
patch("app.internal.node_sweeper.fstore") as mock_fstore:
|
|
mock_fstore.doc_update = AsyncMock()
|
|
await _sweep()
|
|
|
|
mock_fstore.doc_update.assert_not_called()
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Batch behaviour
|
|
# ---------------------------------------------------------------------------
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_only_stale_nodes_updated_in_batch():
|
|
nodes = [
|
|
_node("node-08", "online", age_seconds=200), # stale → offline
|
|
_node("node-09", "online", age_seconds=5), # fresh → skip
|
|
_node("node-10", "offline", age_seconds=500), # already offline → skip
|
|
_node("node-11", "recording", age_seconds=150), # stale recording → offline
|
|
]
|
|
|
|
with patch("asyncio.to_thread", new=AsyncMock(return_value=nodes)), \
|
|
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"}
|