fix: SensorUpdate leaked mutable state across updates; add API test coverage - #137
Draft
bluetoothbot wants to merge 2 commits into
Draft
fix: SensorUpdate leaked mutable state across updates; add API test coverage#137bluetoothbot wants to merge 2 commits into
bluetoothbot wants to merge 2 commits into
Conversation
Adds behavioral coverage for the previously-untested SensorData surface: title, per-device info aggregation, primary_device_id, get_device_name, supported(), binary sensors, and device_id propagation. Three tests intentionally FAIL against the current code, demonstrating that SensorUpdate (frozen=True) leaks mutable instance state: _finish_update() returns the live per-update dicts by reference, so a later update() clears events and rewrites values on an already-returned snapshot. The next commit fixes this. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
_finish_update() returned the live per-update dicts by reference. Because those dicts are reused across calls — update() clears _events_updates and update_sensor() reassigns value/description entries — a previously returned SensorUpdate would silently lose its events and have its values rewritten by the next update(). SensorUpdate is frozen=True, so callers reasonably treat it as an immutable snapshot. Copy the dicts on return so each SensorUpdate is a stable point-in-time view. The copies are shallow; the contained value/description objects are themselves frozen dataclasses. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #137 +/- ##
==========================================
+ Coverage 94.37% 99.37% +5.00%
==========================================
Files 12 12
Lines 480 480
Branches 5 5
==========================================
+ Hits 453 477 +24
+ Misses 26 2 -24
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Make each
SensorUpdatean immutable snapshot, and add behavioral test coverage for the previously-untestedSensorDatapublic API.Why
SensorUpdateis declaredfrozen=True, but_finish_update()returned the live per-update dicts by reference. Those dicts are reused across calls —update()calls_events_updates.clear(), andupdate_sensor()/update_binary_sensor()reassign entries — so a previously-returnedSensorUpdatewould silently mutate when the nextupdate()ran:eventsgot cleared out from under the caller (events are transient and.clear()-ed each cycle);entity_values/ descriptions were rewritten to the newer reading.Any consumer that holds the prior update to diff against the next one (a common pattern) would read corrupted data. This came to light while adding coverage — the demonstrating tests fail against the pre-fix commit.
How
_finish_update()now returns shallowdict(...)copies of each per-update mapping. The contained value/description objects are themselves frozen dataclasses, so a shallow copy is a complete point-in-time snapshot. Cost is a few small dict copies per update — negligible vs. correctness.tests/test_data.pycovers title, per-device info aggregation,primary_device_id,get_device_name(name→type fallback),supported(), binary sensors,device_idpropagation, and two regression guards that pin the snapshot semantics.Structured as two commits (failing tests → fix) so the before/after is reviewable.
Testing
PYTHONPATH=src python3.12 -m pytest tests/→ 25 passed (was 8).ruff checkclean. The 3 snapshot tests fail on commit 1 and pass on commit 2.Quality Report
Changes: 2 files changed, 275 insertions(+), 6 deletions(-)
Code scan: clean
Tests: passed (25 passed)
Branch hygiene: clean
Generated by Kōan post-mission quality pipeline