mirror of
https://github.com/craigerl/aprsd.git
synced 2026-08-30 15:55:57 -04:00
fix: WatchList.stats() — remove early return that bypassed lock (#258)
stats() had a bare 'return self.data' on the first line that:
- returned the raw internal dict without holding self.lock (race condition)
- made the locked loop below unreachable dead code
- returned the wrong shape (callers expect age/old/packet/last keys, not
the raw was_old_before_update internal key)
Remove the early return so the existing with self.lock: loop executes.
Tests added (tests/packets/test_watch_list.py):
- test_stats_empty: empty watch list returns {}
- test_stats_returns_enriched_shape: verifies the four expected keys are
present and the raw-internal 'was_old_before_update' key is absent
- test_stats_not_raw_internal_dict: returned dict must not be wl.data itself
- setUp/tearDown: reset class-level data and initialized to prevent leakage
Closes #237
This commit is contained in:
+1
-3
@@ -8,9 +8,7 @@ Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog).
|
||||
|
||||
##### Bug Fixes
|
||||
|
||||
- Add tests for APRSDClient._checks initialisation and first-call keepalive behaviour [`20da2ad`](https://github.com/craigerl/aprsd/commit/20da2ad)
|
||||
|
||||
- Fix APRSDClient._checks AttributeError on first keepalive_check() call — initialise to False in __init__ [`52e1346`](https://github.com/craigerl/aprsd/commit/52e1346)
|
||||
- Fix WatchList.stats() and add regression tests — remove early return that bypassed lock [`481a78a`](https://github.com/craigerl/aprsd/commit/481a78a)
|
||||
|
||||
- Fix APRSISDriver.is_configured() always returning True when driver is disabled [`a9ef65f`](https://github.com/craigerl/aprsd/commit/a9ef65f)
|
||||
|
||||
|
||||
@@ -51,7 +51,6 @@ class WatchList(objectstore.ObjectStoreMixin):
|
||||
@trace.no_trace
|
||||
def stats(self, serializable=False) -> dict:
|
||||
stats = {}
|
||||
return self.data
|
||||
with self.lock:
|
||||
for callsign in self.data:
|
||||
stats[callsign] = {
|
||||
|
||||
@@ -14,8 +14,10 @@ class TestWatchList(unittest.TestCase):
|
||||
|
||||
def setUp(self):
|
||||
"""Set up test fixtures."""
|
||||
# Reset singleton instance
|
||||
# Reset singleton AND class-level state fully between tests
|
||||
watch_list.WatchList._instance = None
|
||||
watch_list.WatchList.data = {}
|
||||
watch_list.WatchList.initialized = False
|
||||
# Mock config
|
||||
CONF.watch_list.enabled = True
|
||||
CONF.watch_list.callsigns = ['TEST*']
|
||||
@@ -24,6 +26,8 @@ class TestWatchList(unittest.TestCase):
|
||||
def tearDown(self):
|
||||
"""Clean up after tests."""
|
||||
watch_list.WatchList._instance = None
|
||||
watch_list.WatchList.data = {}
|
||||
watch_list.WatchList.initialized = False
|
||||
|
||||
def test_singleton_pattern(self):
|
||||
"""Test that WatchList is a singleton."""
|
||||
@@ -47,6 +51,58 @@ class TestWatchList(unittest.TestCase):
|
||||
self.assertIn('TEST1', wl.data)
|
||||
self.assertIn('TEST2', wl.data)
|
||||
|
||||
def test_stats_empty(self):
|
||||
"""stats() returns an empty dict when the watch list is empty."""
|
||||
watch_list.WatchList._instance = None
|
||||
CONF.watch_list.callsigns = []
|
||||
wl = watch_list.WatchList()
|
||||
self.assertEqual(wl.stats(), {})
|
||||
|
||||
def test_stats_returns_enriched_shape(self):
|
||||
"""stats() must return the enriched dict shape, not the raw internal data.
|
||||
|
||||
Regression test for the early-return bug: 'return self.data' was the
|
||||
first statement in stats(), bypassing the lock and returning the raw
|
||||
internal dict instead of the expected {callsign: {last, packet, age, old}}
|
||||
shape that callers rely on.
|
||||
"""
|
||||
watch_list.WatchList._instance = None
|
||||
CONF.watch_list.callsigns = ['ENRICHED*']
|
||||
wl = watch_list.WatchList()
|
||||
|
||||
# Populate with a seen packet so there is something to report
|
||||
from tests import fake
|
||||
|
||||
packet = fake.fake_packet(fromcall='ENRICHED')
|
||||
wl.rx(packet)
|
||||
|
||||
stats = wl.stats()
|
||||
|
||||
self.assertIn('ENRICHED', stats)
|
||||
entry = stats['ENRICHED']
|
||||
# These keys are built by the locked loop — absent if early return fires
|
||||
self.assertIn('last', entry)
|
||||
self.assertIn('packet', entry)
|
||||
self.assertIn('age', entry)
|
||||
self.assertIn('old', entry)
|
||||
# Confirm we are NOT getting the raw internal key that only exists there
|
||||
self.assertNotIn('was_old_before_update', entry)
|
||||
|
||||
def test_stats_not_raw_internal_dict(self):
|
||||
"""stats() must not return the raw self.data reference.
|
||||
|
||||
If stats() returns self.data directly, mutations via rx() after the
|
||||
call would silently alter the returned value — and any consumer
|
||||
that sees 'was_old_before_update' knows it got the raw dict.
|
||||
"""
|
||||
watch_list.WatchList._instance = None
|
||||
CONF.watch_list.callsigns = ['RAW*']
|
||||
wl = watch_list.WatchList()
|
||||
|
||||
stats = wl.stats()
|
||||
# stats() should return a freshly-built dict, not the live self.data ref
|
||||
self.assertIsNot(stats, wl.data)
|
||||
|
||||
def test_stats(self):
|
||||
"""Test stats() method."""
|
||||
wl = watch_list.WatchList()
|
||||
|
||||
Reference in New Issue
Block a user