Fix ring chimes data update #109220
Fix ring chimes data update #109220
Conversation
home-assistant
bot
added
bugfix
cla-signed
has-tests
integration: ring
small-pr
sdb9696
changed the title
tests/components/ring/test_sensor.py
Outdated
| error_logs = [record for record in caplog.records if record.levelname == "ERROR"] | ||
| assert len(error_logs) == 0 |
There was a problem hiding this comment.
Isn't having the update trigger enough?
There was a problem hiding this comment.
Not completely sure what you mean but the bug would cause the error in the logs so the test is checking the logs do not have errors. Otherwise the test passes.
There was a problem hiding this comment.
Wouldn't having
assert UnboundLocalError not in caplog.textBe a better approach?
There was a problem hiding this comment.
Wouldn't having
assert "UnboundLocalError" not in caplog.textBe a better approach?
There was a problem hiding this comment.
I've added a check for "UnboundLocalError" (and referenced the specific issue causing it). However as this test will be useful to pick up other errors in the future I've kept a check that there are no errors and also introduced a check that there's a success message. All three checks I tested with and without the fix and they call pickup the error.
| requests_mock.get( | ||
| "https://api.ring.com/clients_api/ring_devices", | ||
| text=load_fixture("chime_devices.json", "ring"), | ||
| ) |
There was a problem hiding this comment.
We usually patch the library instead of mocking http requests done by the integration
There was a problem hiding this comment.
All of the tests in the integration use this approach for mocking the library so it could be a quite lift to change that for this fix . I can look at seeing how to change this for all the tests in a subsequent PR if that's ok.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
Comments addressed/replied to |
|
Failing CI due to codecov upload, @joostlek are you able to re-trigger that step please? |
joostlek
changed the title
|
Many thanks @joostlek ! |
Proposed change
Fix coordinator bug with chimes introduced by #107088.
Reported in issue #109210.
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..coveragerc.To help with the load of incoming pull requests: