Feature/reorg recorder#6237
Conversation
| from .test_init import hass_recorder # noqa | ||
|
|
||
|
|
||
| def test_recorder_bad_commit(hass_recorder): |
There was a problem hiding this comment.
redefinition of unused 'hass_recorder' from line 8
| from homeassistant.const import MATCH_ALL | ||
| from homeassistant.components import recorder | ||
| from homeassistant.components.recorder.const import DATA_INSTANCE | ||
| from homeassistant.components.recorder.util import session_scope, execute |
There was a problem hiding this comment.
'homeassistant.components.recorder.util.execute' imported but unused
| from datetime import datetime, timedelta | ||
| import unittest | ||
| from unittest.mock import patch, call, MagicMock | ||
| from unittest.mock import patch, MagicMock |
There was a problem hiding this comment.
'unittest.mock.patch' imported but unused
'unittest.mock.MagicMock' imported but unused
| {recorder.DOMAIN: config}) | ||
| assert recorder.DOMAIN in hass.config.components | ||
| run_coroutine_threadsafe( | ||
| recorder.wait_connection_ready(hass), hass.loop).result() |
There was a problem hiding this comment.
One nice thing about threading.Event.wait() was the timeout, do you know if asyncio.Event.wait() has an equivalent?
Raising an exception after some seconds of waiting really helps with fault finding. Before that tests would block without any indication of what you did wrong.
There was a problem hiding this comment.
Maybe the same mechanism used with aiohttp can be used?
There was a problem hiding this comment.
There is a timeout but it's better to run tests with the timeout flag (like we do on travis), that way we don't have to add timeouts all over the place in our tests but instead have PyTest manage it for us:
py.test tests/components/recorder -x --timeout=2
|
|
||
| if event.event_type == EVENT_TIME_CHANGED: | ||
| elif event is purge_task: | ||
| purge.purge_old_data(self, self.purge_days) |
There was a problem hiding this comment.
👍 probably never got issues since this happens so infrequently, but much better as part of this worker loop
There was a problem hiding this comment.
We should make sure that any data mutation happens from 1 thread.
| session.add(work) | ||
| session.commit() | ||
| return True | ||
| except sqlalchemy.exc.OperationalError as err: |
There was a problem hiding this comment.
Never could really get to the bottom if repeating this 3 times had any value. Guess you are of the opinion it wont make a difference?
There was a problem hiding this comment.
I see the commit function does this, but not used everywhere
There was a problem hiding this comment.
I find it kinda weird that we do things 3 times and I think that it also added to the confusion of how SQLAlchemy works (instantiating sessions everywhere for example).
| try: | ||
| yield session | ||
| session.commit() | ||
| except SQLAlchemyError as err: |
There was a problem hiding this comment.
I prefer this as a general Exception, since it is important to log any error.
There was an issue aboit invalid codepage TypeError that this also fixed.
All errors will be passed on in any case, but if we log it at least users can fix these type of things
| continue | ||
| with session_scope(session=self.get_session()) as session: | ||
| dbevent = models.Events.from_event(event) | ||
| session.add(dbevent) |
There was a problem hiding this comment.
Should this one not use the commit function that retries?
| def test_recorder_bad_execute(hass_recorder): | ||
| """Bad execute, retry 3 times.""" | ||
| from sqlalchemy.exc import SQLAlchemyError | ||
| hass = hass_recorder() |
There was a problem hiding this comment.
local variable 'hass' is assigned to but never used
|
Merging this now so we can have the tests work again. We can iterate on it later if we want more changes. If recorder issues stay persistent on 0.39 we might want to consider adding this PR as a point release. |
ghost
added
integration: const
integration: history_stats
integration: migration
integration: purge
integration: util
and removed
platform: recorder.const
labels
6 participants
Description:
This is the successor of #6196 and builds on the work of @kellerza in #6167.
Thanks to @kellerza for all the amazing tests on the recorder component 👍
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io#<home-assistant.github.io PR number goes here>
Example entry for
configuration.yaml(if applicable):history:Checklist:
If the code does not interact with devices:
toxrun successfully. Your PR cannot be merged unless tests pass