Repository navigation
Fix wirelesstag KeyError when a tag disappears from a reload - #172600
Conversation
WirelessTagBaseSensor.update() looked up the reloaded tags with updated_tags[self._uuid], which raises KeyError if the tag is no longer returned (the following `is None` guard only handles a present None value). Use .get() so a missing tag takes the existing graceful error path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
home-assistant
Bot
added
bugfix
cla-signed
has-tests
integration: wirelesstag
small-pr
|
Hey there @sergeymaysak, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds regression coverage and hardens Wireless Sensor Tags entity updates so missing tags on reload don’t raise errors.
Changes:
- Avoid
KeyErrorduring entityupdate()when a tag disappears fromload_tags(). - Add a new sensor test ensuring the missing-tag path logs and leaves the entity intact.
- Assign CODEOWNERS for the new WirelessTag test directory.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/components/wirelesstag/test_sensor.py | Adds regression test for missing tag on reload/update. |
| tests/components/wirelesstag/init.py | Initializes the WirelessTag tests package. |
| homeassistant/components/wirelesstag/entity.py | Switches dict access to .get() to prevent KeyError and log gracefully. |
| CODEOWNERS | Adds ownership for WirelessTag tests directory. |
| async def test_poll_handles_tag_missing_from_reload( | ||
| hass: HomeAssistant, | ||
| caplog: pytest.LogCaptureFixture, | ||
| ) -> None: | ||
| """Test a poll where the tag is no longer returned is handled gracefully. | ||
|
|
||
| If a reload no longer contains the entity's tag, the update must log and | ||
| return instead of raising a KeyError. | ||
| """ | ||
| tag = _mock_tag() | ||
| with patch("homeassistant.components.wirelesstag.WirelessTags") as mock_api_class: | ||
| mock_api = mock_api_class.return_value | ||
| mock_api.load_tags.return_value = {tag.uuid: tag} | ||
|
|
||
| assert await async_setup_component(hass, "wirelesstag", CONFIG) | ||
| await hass.async_block_till_done() | ||
| assert await async_setup_component(hass, "sensor", CONFIG) | ||
| await hass.async_block_till_done() | ||
| assert hass.states.get(ENTITY_ID) is not None | ||
|
|
||
| # The tag is no longer returned by a reload. | ||
| mock_api.load_tags.return_value = {} | ||
| await async_update_entity(hass, ENTITY_ID) | ||
| await hass.async_block_till_done() | ||
|
|
||
| # The entity survives and the graceful error path is taken (no KeyError). | ||
| assert hass.states.get(ENTITY_ID) is not None | ||
| assert "Unable to update tag" in caplog.text |
There was a problem hiding this comment.
Good call — cleared caplog right before the update and switched to asserting on an ERROR-level record, so it only passes for the missing-tag path. Done in 3e73c26.
| return tag | ||
|
|
||
|
|
||
| async def test_poll_handles_tag_missing_from_reload( |
There was a problem hiding this comment.
Agreed — renamed to test_update_handles_tag_missing_from_reload and updated the docstring to reflect the explicit update. Done in 3e73c26.
Address review feedback: rename the test to reflect that it triggers an explicit update (not a scheduled poll), and clear caplog before the update and assert on an ERROR-level record so the assertion only passes for the missing-tag path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
|
||
| updated_tags = self._api.load_tags() | ||
| if (updated_tag := updated_tags[self._uuid]) is None: | ||
| if (updated_tag := updated_tags.get(self._uuid)) is None: |
There was a problem hiding this comment.
This bugfix is reasonable, however the below log might be over-logging at error. Consider lowering the level, or setting unavailable and clearing state if actually gone.
There was a problem hiding this comment.
Good catch — the error was firing on every poll, and worse, the entity kept serving its last value while the tag was gone.
Went with the second option in bd61e8d: update() now records the missing tag, available returns False for it, and the log drops to debug since the condition is visible to the user as unavailable. The state is cleared, and the entity recovers if the tag comes back. The test asserts the unavailable/recover transitions instead of scraping the log.
The polling path logged at error level every scan interval when the tag was missing from a reload, while the entity kept serving the last known value. Track the missing tag, report it through availability, and drop the log to debug now that the condition is visible to the user. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| # poll. Reported through availability rather than a log message | ||
| # repeated on every poll. | ||
| _LOGGER.debug('Unable to update tag: "%s"', self.name) | ||
| self._tag_missing = True |
There was a problem hiding this comment.
Correct, and thanks — that was a real hole in the flag I just added. A push proves the tag exists, but neither callback cleared _tag_missing, so the entity stayed unavailable until the next successful poll.
Fixed in 4d4a80d by routing all three paths through one helper on the base entity rather than repeating the reset:
def _apply_tag(self, tag: SensorTag) -> None:
"""Adopt a freshly received tag and refresh the state from it."""
self._tag_missing = False
self._tag = tag
self._state = self.updated_state_value()update(), WirelessTagSensor._update_tag_info_callback and WirelessTagBinarySensor._on_binary_event_callback all call it, so a future platform cannot forget the reset. The test is now parametrized over both recovery paths (poll and push); I confirmed the push case fails against the previous commit and passes with this one.
The push callbacks replaced the tag without resetting the flag, so an entity marked missing by one poll stayed unavailable until the next successful poll even though the push proved the tag was back. Funnel both callbacks and the poll path through a single helper that adopts the tag. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| ) -> None: | ||
| """Make the tag available again through the polling path.""" | ||
| mock_api.load_tags.return_value = {tag.uuid: tag} | ||
| await async_update_entity(hass, ENTITY_ID) |
There was a problem hiding this comment.
we shouldn't call async_update_entity directly. Instead use the mock to fetch a registered callback method or skip time using the freezer to trigger a "natural" refresh
There was a problem hiding this comment.
Done in 417ddb4 — replaced with freezer.tick(SCAN_INTERVAL) + async_fire_time_changed.
| async_dispatcher_send( | ||
| hass, SIGNAL_TAG_UPDATE.format(tag.tag_id, tag.tag_manager_mac), tag | ||
| ) |
There was a problem hiding this comment.
don't use the dispatcher directly
There was a problem hiding this comment.
Done in 417ddb4 — the test now calls the push callback the integration registered via mock_api.start_monitoring.call_args, from an executor job as the library does.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Replace the direct async_update_entity call with a frozen-time tick over the platform scan interval, and drive push recovery through the callback the integration registered with the library mock instead of firing the dispatcher signal by hand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@joostlek both points from your review are addressed in 417ddb4:
The test is parametrized over both recovery paths ( |
Resolve an add/add conflict in tests/components/wirelesstag/test_sensor.py: the wirelesstag test module also arrived on dev via home-assistant#172590, home-assistant#172592 and home-assistant#172598. Keep both suites, taking the shared _mock_tag helper and CONFIG from dev since they cover temperature and humidity, and point the missing-tag test at the existing entity id constant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| # The tag is gone from the account, so there is nothing left to | ||
| # poll. Reported through availability rather than a log message | ||
| # repeated on every poll. | ||
| _LOGGER.debug('Unable to update tag: "%s"', self.name) |
There was a problem hiding this comment.
Fair point — the debug line does fire on every poll, so that sentence described the change from error to debug rather than the code under it.
Replaced in 1faac3c with a single line stating the constraint that actually matters, which is why this is debug and not a warning:
# A tag can be removed from the account, so this is not an error.The comment described the move from error to debug rather than the code in front of it, and claimed the condition was reported through availability instead of a per-poll log message while the debug line still fires on every poll. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tests/components/wirelesstag/test_sensor.py:158
- Remove this comment because it only restates that the following mock response omits the tag.
# The tag is no longer returned by a reload.
There was a problem hiding this comment.
🟢 Approval recommended
No blocking issues remain in the focused fix or regression coverage; tests were not executed.
4 open findings
_tag_missingwhen a push update supplies the tag. Both push callbacks replace_tagand…caplogcontains records emitted during setup, or if…poll, but the test triggers an explicit entity update via…
🧠 Review effort: Balanced
Breaking change
Proposed change
WirelessTagBaseSensor.update()(the polling path) looked up the reloaded tagswith
updated_tags[self._uuid]. If a reload no longer contains the entity's tag(for example the tag was removed from the account), this raises
KeyError.The following
is Noneguard only handles a present-but-Nonevalue, not amissing key.
Use
updated_tags.get(self._uuid)so a missing tag no longer raises. Instead oflogging an error on every poll, the entity is marked unavailable and the message
is logged at debug level, since a tag being removed from the account is not an
error.
The push callbacks in
sensor.pyandbinary_sensor.pynow share an_apply_taghelper with the polling path, so the entity becomes available againas soon as the tag shows up through either a poll or a push notification.
Found while reviewing the integration; no separate issue was filed.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: