Make sure to clean register callbacks when mobile_app reloads #156028
Conversation
|
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
13efccd to
30bed06
Compare
| async_dispatcher_send( | ||
| hass, | ||
| "mobile_app_binary_sensor_register", | ||
| { | ||
| CONF_WEBHOOK_ID: webhook_id, | ||
| ATTR_SENSOR_NAME: "Test After Unload", | ||
| ATTR_SENSOR_STATE: False, | ||
| ATTR_SENSOR_TYPE: "binary_sensor", | ||
| ATTR_SENSOR_UNIQUE_ID: "test_after_unload", | ||
| ATTR_SENSOR_ICON: "mdi:test", | ||
| ATTR_SENSOR_ATTRIBUTES: {}, | ||
| }, | ||
| ) | ||
| await hass.async_block_till_done() |
There was a problem hiding this comment.
Ideally we don't touch the dispatcher in the tests, let's use the webhook to create a (binary) sensor
There was a problem hiding this comment.
Not done, we can't use the webhook easilly since we unload the component so the webhook returns a 201.
There was a problem hiding this comment.
@joostlek I think it could be ok to use the dispatcher helper directly here since the alternative is to check for logs, per my suggestion below, which isn't great either. What do you think?
There was a problem hiding this comment.
I guess that makes sense. I kinda wish there was a different way to test that we unregistered these
|
|
||
| # This binary sensor should be created successfully after reload | ||
| assert hass.states.get("binary_sensor.test_1_test_after_reload") is not None | ||
| assert hass.states.get("binary_sensor.test_1_test_after_reload").state == "on" |
There was a problem hiding this comment.
I think we should try to check that the dispatch target is properly unregistered between reloads, by checking that there's not a unique id error when adding the entity. Set the same unique id in the webhook data to try to provoke this. There should be an error log about duplicate unique id.
Verify that the test works manually by running it without the fix.
There was a problem hiding this comment.
core/homeassistant/helpers/entity_platform.py
Line 831 in 6a07b46
This reverts commit 108866a.
MartinHjelmare
left a comment
There was a problem hiding this comment.
Let's merge here. Joost and me have been discussing how to improve tests like this in general but we should do that separately.
Lemme know when you agree on something so I can update the test 👍🏻 |
Proposed change
When the component is created we register a callback for the
registersignal but it is never clear, which could lead to memory leaks especially since the component reloads each time a user enable/disable a sensor on a device. This PR makes sure that we clear when the component is unloaded.Type of change
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: