-
-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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
Refactor prometheus integration tests #113849
base: dev
Are you sure you want to change the base?
Refactor prometheus integration tests #113849
Conversation
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.
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
|
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.
Thank you.
Most of the tests check label values, so you will need to adjust those 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), | |||
# TODO: how to get zone here? |
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.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
body
refers to, since this is so far away from the code that passes HTTP response body to these methods. Let's call itmetrics
instead? Ormetric_response
if 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: