Skip to content
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

Statistics component fix device_class for incremental source sensors #88096

Merged
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
56 changes: 37 additions & 19 deletions homeassistant/components/statistics/sensor.py
Expand Up @@ -13,7 +13,8 @@

from homeassistant.components.binary_sensor import DOMAIN as BINARY_SENSOR_DOMAIN
from homeassistant.components.recorder import get_instance, history
from homeassistant.components.sensor import (
from homeassistant.components.sensor import ( # pylint: disable=hass-deprecated-import
DEVICE_CLASS_STATE_CLASSES,
frenck marked this conversation as resolved.
Show resolved Hide resolved
PLATFORM_SCHEMA,
SensorDeviceClass,
SensorEntity,
Expand Down Expand Up @@ -144,7 +145,7 @@
}

# Statistics which retain the unit of the source entity
STAT_NUMERIC_RETAIN_UNIT = {
STATS_NUMERIC_RETAIN_UNIT = {
STAT_AVERAGE_LINEAR,
STAT_AVERAGE_STEP,
STAT_AVERAGE_TIMELESS,
Expand All @@ -166,7 +167,7 @@
}

# Statistics which produce percentage ratio from binary_sensor source entity
STAT_BINARY_PERCENTAGE = {
STATS_BINARY_PERCENTAGE = {
STAT_AVERAGE_STEP,
STAT_AVERAGE_TIMELESS,
STAT_MEAN,
Expand Down Expand Up @@ -296,15 +297,9 @@ def __init__(
self.ages: deque[datetime] = deque(maxlen=self._samples_max_buffer_size)
self.attributes: dict[str, StateType] = {}

self._state_characteristic_fn: Callable[[], StateType | datetime]
if self.is_binary:
self._state_characteristic_fn = getattr(
self, f"_stat_binary_{self._state_characteristic}"
)
else:
self._state_characteristic_fn = getattr(
self, f"_stat_{self._state_characteristic}"
)
self._state_characteristic_fn: Callable[
[], StateType | datetime
] = self._callable_characteristic_fn(self._state_characteristic)

self._update_listener: CALLBACK_TYPE | None = None

Expand Down Expand Up @@ -368,11 +363,11 @@ def _add_state_to_queue(self, new_state: State) -> None:
def _derive_unit_of_measurement(self, new_state: State) -> str | None:
base_unit: str | None = new_state.attributes.get(ATTR_UNIT_OF_MEASUREMENT)
unit: str | None
if self.is_binary and self._state_characteristic in STAT_BINARY_PERCENTAGE:
if self.is_binary and self._state_characteristic in STATS_BINARY_PERCENTAGE:
unit = PERCENTAGE
elif not base_unit:
unit = None
elif self._state_characteristic in STAT_NUMERIC_RETAIN_UNIT:
elif self._state_characteristic in STATS_NUMERIC_RETAIN_UNIT:
unit = base_unit
elif self._state_characteristic in STATS_NOT_A_NUMBER:
unit = None
Expand All @@ -393,11 +388,22 @@ def _derive_unit_of_measurement(self, new_state: State) -> str | None:
@property
def device_class(self) -> SensorDeviceClass | None:
"""Return the class of this device."""
if self._state_characteristic in STAT_NUMERIC_RETAIN_UNIT:
_state = self.hass.states.get(self._source_entity_id)
return None if _state is None else _state.attributes.get(ATTR_DEVICE_CLASS)
if self._state_characteristic in STATS_DATETIME:
return SensorDeviceClass.TIMESTAMP
if self._state_characteristic in STATS_NUMERIC_RETAIN_UNIT:
_source_state = self.hass.states.get(self._source_entity_id)
if _source_state is None:
return None
source_device_class = _source_state.attributes.get(ATTR_DEVICE_CLASS)
if source_device_class is None:
return None
sensor_device_class = SensorDeviceClass(source_device_class)
ThomDietrich marked this conversation as resolved.
Show resolved Hide resolved
sensor_state_classes = DEVICE_CLASS_STATE_CLASSES.get(
sensor_device_class, set()
)
Comment on lines +404 to +406
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The creation of an empty set() here is not needed.

if SensorStateClass.MEASUREMENT not in sensor_state_classes:
Comment on lines +404 to +407
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wallrus could have been used.

return None
return sensor_device_class
return None

@property
Expand Down Expand Up @@ -472,8 +478,8 @@ async def async_update(self) -> None:
if timestamp := self._next_to_purge_timestamp():
_LOGGER.debug("%s: scheduling update at %s", self.entity_id, timestamp)
if self._update_listener:
self._update_listener()
self._update_listener = None
self._update_listener() # pragma: no cover
self._update_listener = None # pragma: no cover
Comment on lines +484 to +485
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When does this happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't exactly tell you. I did not develop this part of the code. Pytest always showed missing coverage for these two lines, hence the comment. Maybe the code can be removed or changed, but I didn't feel the need.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There shouldn't be any pragma: no cover in homeassistant code.
It is better to have reduced coverage than invalid coverage.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #88173

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not agree and don't see how it would make sense in this particular case. Anyhow, understand the incentive generally and won't fight with you.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the code is there, there must be a reason...

  • If it is dead code that can't be reached, then it should be removed
  • If it is not dead code, then we need to be reminded that coverage should be improved

The only time when no cover is in use is when we raise an exception for unreachable code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is not reason to introduce fake coverage. Epenet is right here, it should not have had these flags. I've therefore merged the PR to remove them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought @epenet I agree with you. I did not develop this code nor did I ever touch it. I believe it's "dead code" as you called it and will do some tests and provide a fix in a future PR.


@callback
def _scheduled_update(now: datetime) -> None:
Expand Down Expand Up @@ -563,6 +569,18 @@ def _update_value(self) -> None:
value = int(value)
self._value = value

def _callable_characteristic_fn(
self, characteristic: str
) -> Callable[[], StateType | datetime]:
"""Return the function callable of one characteristic function."""
function: Callable[[], StateType | datetime] = getattr(
self,
f"_stat_binary_{characteristic}"
if self.is_binary
else f"_stat_{characteristic}",
Comment on lines +582 to +583
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't allow the use of multi-line inline if-statements; as they become hard to read. Please adjust this in a followup PR to have a regular if statement.

)
return function

# Statistics for numeric sensor

def _stat_average_linear(self) -> StateType:
Expand Down
120 changes: 112 additions & 8 deletions tests/components/statistics/test_sensor.py
Expand Up @@ -21,6 +21,7 @@
SERVICE_RELOAD,
STATE_UNAVAILABLE,
STATE_UNKNOWN,
UnitOfEnergy,
UnitOfTemperature,
)
from homeassistant.core import HomeAssistant
Expand Down Expand Up @@ -250,6 +251,63 @@ async def test_sensor_source_with_force_update(hass: HomeAssistant):
assert state_force.attributes.get("buffer_usage_ratio") == round(9 / 20, 2)


async def test_sampling_boundaries_given(hass: HomeAssistant):
"""Test if either sampling_size or max_age are given."""
assert await async_setup_component(
hass,
"sensor",
{
"sensor": [
{
"platform": "statistics",
"name": "test_boundaries_none",
"entity_id": "sensor.test_monitored",
"state_characteristic": "mean",
},
{
"platform": "statistics",
"name": "test_boundaries_size",
"entity_id": "sensor.test_monitored",
"state_characteristic": "mean",
"sampling_size": 20,
},
{
"platform": "statistics",
"name": "test_boundaries_age",
"entity_id": "sensor.test_monitored",
"state_characteristic": "mean",
"max_age": {"minutes": 4},
},
{
"platform": "statistics",
"name": "test_boundaries_both",
"entity_id": "sensor.test_monitored",
"state_characteristic": "mean",
"sampling_size": 20,
"max_age": {"minutes": 4},
},
]
},
)
await hass.async_block_till_done()

hass.states.async_set(
"sensor.test_monitored",
str(VALUES_NUMERIC[0]),
{ATTR_UNIT_OF_MEASUREMENT: UnitOfTemperature.CELSIUS},
)
await hass.async_block_till_done()

state = hass.states.get("sensor.test_boundaries_none")
assert state is None
state = hass.states.get("sensor.test_boundaries_size")
assert state is not None
state = hass.states.get("sensor.test_boundaries_age")
assert state is not None
state = hass.states.get("sensor.test_boundaries_both")
assert state is not None


async def test_sampling_size_reduced(hass: HomeAssistant):
"""Test limited buffer size."""
assert await async_setup_component(
Expand Down Expand Up @@ -514,9 +572,9 @@ async def test_device_class(hass: HomeAssistant):
{
"sensor": [
{
# Device class is carried over from source sensor for characteristics with same unit
# Device class is carried over from source sensor for characteristics which retain unit
"platform": "statistics",
"name": "test_source_class",
"name": "test_retain_unit",
"entity_id": "sensor.test_monitored",
"state_characteristic": "mean",
"sampling_size": 20,
Expand All @@ -537,6 +595,14 @@ async def test_device_class(hass: HomeAssistant):
"state_characteristic": "datetime_oldest",
"sampling_size": 20,
},
{
# Device class is set to None for any source sensor with TOTAL state class
"platform": "statistics",
"name": "test_source_class_total",
"entity_id": "sensor.test_monitored_total",
"state_characteristic": "mean",
"sampling_size": 20,
},
]
},
)
Expand All @@ -549,11 +615,21 @@ async def test_device_class(hass: HomeAssistant):
{
ATTR_UNIT_OF_MEASUREMENT: UnitOfTemperature.CELSIUS,
ATTR_DEVICE_CLASS: SensorDeviceClass.TEMPERATURE,
ATTR_STATE_CLASS: SensorStateClass.MEASUREMENT,
},
)
hass.states.async_set(
"sensor.test_monitored_total",
str(value),
{
ATTR_UNIT_OF_MEASUREMENT: UnitOfEnergy.WATT_HOUR,
ATTR_DEVICE_CLASS: SensorDeviceClass.ENERGY,
ATTR_STATE_CLASS: SensorStateClass.TOTAL,
},
)
await hass.async_block_till_done()

state = hass.states.get("sensor.test_source_class")
state = hass.states.get("sensor.test_retain_unit")
assert state is not None
assert state.attributes.get(ATTR_DEVICE_CLASS) == SensorDeviceClass.TEMPERATURE
state = hass.states.get("sensor.test_none")
Expand All @@ -562,6 +638,9 @@ async def test_device_class(hass: HomeAssistant):
state = hass.states.get("sensor.test_timestamp")
assert state is not None
assert state.attributes.get(ATTR_DEVICE_CLASS) == SensorDeviceClass.TIMESTAMP
state = hass.states.get("sensor.test_source_class_total")
assert state is not None
assert state.attributes.get(ATTR_DEVICE_CLASS) is None


async def test_state_class(hass: HomeAssistant):
Expand All @@ -572,17 +651,28 @@ async def test_state_class(hass: HomeAssistant):
{
"sensor": [
{
# State class is None for datetime characteristics
"platform": "statistics",
"name": "test_nan",
"entity_id": "sensor.test_monitored",
"state_characteristic": "datetime_oldest",
"sampling_size": 20,
},
{
# State class is MEASUREMENT for all other characteristics
"platform": "statistics",
"name": "test_normal",
"entity_id": "sensor.test_monitored",
"state_characteristic": "count",
"sampling_size": 20,
},
{
# State class is MEASUREMENT, even when the source sensor
# is of state class TOTAL
"platform": "statistics",
"name": "test_nan",
"entity_id": "sensor.test_monitored",
"state_characteristic": "datetime_oldest",
"name": "test_total",
"entity_id": "sensor.test_monitored_total",
"state_characteristic": "count",
"sampling_size": 20,
},
]
Expand All @@ -596,14 +686,28 @@ async def test_state_class(hass: HomeAssistant):
str(value),
{ATTR_UNIT_OF_MEASUREMENT: UnitOfTemperature.CELSIUS},
)
hass.states.async_set(
"sensor.test_monitored_total",
str(value),
{
ATTR_UNIT_OF_MEASUREMENT: UnitOfTemperature.CELSIUS,
ATTR_STATE_CLASS: SensorStateClass.TOTAL,
},
)
await hass.async_block_till_done()

state = hass.states.get("sensor.test_nan")
assert state is not None
assert state.attributes.get(ATTR_STATE_CLASS) is None
state = hass.states.get("sensor.test_normal")
assert state is not None
assert state.attributes.get(ATTR_STATE_CLASS) is SensorStateClass.MEASUREMENT
state = hass.states.get("sensor.test_nan")
state = hass.states.get("sensor.test_monitored_total")
assert state is not None
assert state.attributes.get(ATTR_STATE_CLASS) is None
assert state.attributes.get(ATTR_STATE_CLASS) is SensorStateClass.TOTAL
state = hass.states.get("sensor.test_total")
assert state is not None
assert state.attributes.get(ATTR_STATE_CLASS) is SensorStateClass.MEASUREMENT


async def test_unitless_source_sensor(hass: HomeAssistant):
Expand Down