Refactor prometheus integration tests #113849
Conversation
home-assistant
bot
added
cla-needed
integration: prometheus
small-pr
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
Hey there @knyar, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
home-assistant
bot
added
Quality Scale: No score
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
| @@ -345,6 +361,11 @@ def _labels(state: State) -> dict[str, Any]: | |||
| "entity": state.entity_id, | |||
| "domain": state.domain, | |||
| "friendly_name": state.attributes.get(ATTR_FRIENDLY_NAME), | |||
| "area": state.attributes.get(ATTR_AREA_ID), | |||
| # TODO: how to get zone here? | |||
There was a problem hiding this comment.
I just removed zone for now. We can address that in another pr (if the need arises, I'm not using it currently)
| "area": state.attributes.get(ATTR_AREA_ID), | ||
| # TODO: how to get zone here? | ||
| # "zone": state.attributes.get(ATTR_AREA_ID), | ||
| "entity_id": state.object_id, |
There was a problem hiding this comment.
If this is called object_id internally, could you please use object_id for the label name as well?
| @@ -345,6 +361,11 @@ def _labels(state: State) -> dict[str, Any]: | |||
| "entity": state.entity_id, | |||
| "domain": state.domain, | |||
| "friendly_name": state.attributes.get(ATTR_FRIENDLY_NAME), | |||
| "area": state.attributes.get(ATTR_AREA_ID), | |||
There was a problem hiding this comment.
When the attribute is unset, the label value should be empty rather than the string None.
| "area": state.attributes.get(ATTR_AREA_ID), | |
| "area": state.attributes.get(ATTR_AREA_ID) or "", |
| # TODO: how to get zone here? | ||
| # "zone": state.attributes.get(ATTR_AREA_ID), | ||
| "entity_id": state.object_id, | ||
| "device_class": state.attributes.get(ATTR_DEVICE_CLASS), |
There was a problem hiding this comment.
Not sure if device class can be unset, but just in case it is:
| "device_class": state.attributes.get(ATTR_DEVICE_CLASS), | |
| "device_class": state.attributes.get(ATTR_DEVICE_CLASS) or "", |
| def assertInBody(self, body: list[str]): | ||
| """Assert that this metric exists in the provided Prometheus output.""" | ||
| assert any(line.startswith(self._metric_name_string) for line in body) | ||
|
|
||
| def assertNotInBody(self, body: list[str]): | ||
| """Assert that this metric does not exist in Prometheus output.""" | ||
| assert self._metric_name_string not in body |
There was a problem hiding this comment.
Note that as written assertNotInBody will never match, because Prometheus output always includes values.
Perhaps add an internal method for the actual check (_in_metrics), and then use that from both assert functions? This will ensure that the logic always matches, and will allow EntityMetricWithValue to only override _in_metrics, using assert methods from the parent class.
Also a few cosmetic suggestions (sorry for not thinking about these earlier):
- it's not very intuitive what
bodyrefers to, since this is so far away from the code that passes HTTP response body to these methods. Let's call itmetricsinstead? Ormetric_responseif you want to be more explicit? - camelCase is not very idiomatic in Python, I suspect it's better use underscores (surprised that there is no linter checking this?)
| def assertInBody(self, body: list[str]): | |
| """Assert that this metric exists in the provided Prometheus output.""" | |
| assert any(line.startswith(self._metric_name_string) for line in body) | |
| def assertNotInBody(self, body: list[str]): | |
| """Assert that this metric does not exist in Prometheus output.""" | |
| assert self._metric_name_string not in body | |
| def _in_metrics(self, metrics: list[str]) -> bool: | |
| """Report whether this metric exists in the provided Prometheus output.""" | |
| return any(line.startswith(self._metric_name_string) for line in body) | |
| def assert_in_metrics(self, metrics: list[str]): | |
| """Assert that this metric exists in the provided Prometheus output.""" | |
| assert self._in_metrics(metrics) | |
| def assert_not_in_metrics(self, metrics: list[str]): | |
| """Assert that this metric does not exist in Prometheus output.""" | |
| assert not self._in_metrics(metrics) |
| domain="sensor", | ||
| friendly_name="Radio Energy", | ||
| entity="sensor.radio_energy", | ||
| ).withValue(1.0).assertInBody(body) |
There was a problem hiding this comment.
You don't need to change this, but I don't think you needed to append .0 to all integer values.
|
|
||
| assert ( |
There was a problem hiding this comment.
I think this test was actually broken because the metric is present in the body but it is written as 0.0 not 0 so that might be why it fails in upstream? I swapped it in my PR and am open to suggestions around this. I'm not quite sure what we are testing here, tbh
There was a problem hiding this comment.
I am addressing this in #127262 - would you mind reviewing it please? I don't mind if this PR gets merged first, will be happy to rebase mine.
|
Updated everything again and the tests are still passing, though I had to amend a single test and I left a note as to the reason |
|
|
||
| assert ( |
There was a problem hiding this comment.
I am addressing this in #127262 - would you mind reviewing it please? I don't mind if this PR gets merged first, will be happy to rebase mine.
| domain="sensor", | ||
| friendly_name="Text", | ||
| entity="sensor.text", | ||
| ).withValue(0.0).assert_not_in_metrics(body) |
There was a problem hiding this comment.
The assert_not_in_metrics method only exists on EntityMetric, which I think is good - if we don't want a metric to exist, we don't care what value it has.
But it means that the value passed here is unused, and not needed.
| ).withValue(0.0).assert_not_in_metrics(body) | |
| ).assert_not_in_metrics(body) |
There was a problem hiding this comment.
I know this might be silly. Should I add a simple test for each of the assert_in_metrics() and assert_not_in_metrics() helpers? I added some simple tests for everything else.
There was a problem hiding this comment.
Adding a test would definitely be good, and help prevent possible regressions when these helper classes are changed in the future.
There was a problem hiding this comment.
I added tests for the helpers for both classes, both for assert_in_metrics and assert_not_in_metrics (where appropriate)
Proposed change
I was initially inspired by a closed pull request here: #85553 to add some more labels but this pivoted into solely focusing on refactoring the prometheus integration tests to make them less brittle in anticipation of upcoming logic changes.
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: