-
-
Notifications
You must be signed in to change notification settings - Fork 31.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix history stats count update immediately after change #131856
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
It's flaky, and it was flaky since your previous PR (I think #126271).
|
12dc85e
to
5ce3354
Compare
Flaky test was fixed in #131869 |
@@ -186,8 +190,13 @@ def _async_compute_seconds_and_changes( | |||
current_state_matches = history_state.state in self._entity_states | |||
state_change_timestamp = history_state.last_changed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't you do the flooring here? or else, you will calculate with the not-floored version moving forward ? (ie elapsed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did actually try that but it breaks three tests that calculate the time with microsecond precision.
I'm not sure why that's important, but I don't think there's anything wrong with calculating elapsed with the non-floored time?
FAILED tests/components/history_stats/test_sensor.py::test_end_time_with_microseconds_zeroed[Europe/Berlin] - AssertionError: assert '16.0' == '16.0002388888929'
FAILED tests/components/history_stats/test_sensor.py::test_end_time_with_microseconds_zeroed[America/Chicago] - AssertionError: assert '16.0' == '16.0002388888929'
FAILED tests/components/history_stats/test_sensor.py::test_end_time_with_microseconds_zeroed[US/Hawaii] - AssertionError: assert '16.0' == '16.0002388888929'
But if you think we should change that I guess we can change the tests also, but seems more disruptive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was a drive by comment, checking if you knew why. I didn't verify it myself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix looks fine 👍
* Fix history stats count update immediately after change * rerun CI
…nt#131856) * Fix history stats count update immediately after change * rerun CI
Proposed change
Fix history stats to update count immediately after a change.
What was broken was when we were checking to reject state changes in the future, we were accidentally comparing a floored value of
now
against the non-floored timestamp from the database, leading to think that the changes were in the future instead of the current change.Type of change
Additional information
count
option not updating immediately #131847Checklist
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: