core Refactor prometheus integration tests

Proposed change

I was initially inspired by a closed pull request here: https://github.com/home-assistant/core/pull/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

[ ] Dependency upgrade
[ ] Bugfix (non-breaking change which fixes an issue)
[ ] New integration (thank you!)
[ ] New feature (which adds functionality to an existing integration)
[ ] Deprecation (breaking change to happen in the future)
[ ] Breaking change (fix/feature causing existing functionality to break)
[x] Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

Checklist

[x] The code change is tested and works locally.
[x] Local tests pass. Your PR cannot be merged unless tests pass
[x] There is no commented out code in this PR.
[x] I have followed the development checklist
[x] I have followed the perfect PR recommendations
[x] The code has been formatted using Ruff (ruff format homeassistant tests)
[x] Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

[ ] Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

[ ] The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
[ ] New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
[ ] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
[ ] Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

[x] I have reviewed two other open pull requests in this repository.

Mar 19 '24 23:03 jzucker2

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:

Learn more about our pull request process.

Mar 19 '24 23:03 home-assistant[bot]

Hey there @knyar, mind taking a look at this pull request as it has been labeled with an integration (prometheus) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of prometheus can trigger bot actions by commenting:

@home-assistant close Closes the pull request.
@home-assistant rename Awesome new title Renames the pull request.
@home-assistant reopen Reopen the pull request.
@home-assistant unassign prometheus Removes the current integration label and assignees on the pull request, add the integration domain after the command.
@home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
@home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

Mar 19 '24 23:03 home-assistant[bot]

Signed!

Hi @jzucker2

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

Signed!

Mar 19 '24 23:03 jzucker2

I fixed up all the tests! I'm not quite sure what the preferred process for test helpers is but I think it needs a little clean up. Tests do pass though

Mar 29 '24 18:03 jzucker2

Hi! Thank you for the earlier feedback.

I added the labels (except for zone) and also updated the unit tests. It ended up being a huge effort to update the unit tests, they are unintentionally brittle for label changes. I refactored things into a helper class. I know that kind of reduces the legibility of a single test and abstracts a bit, but it should make other label changes much easier going forward.

As of result of finding the helper necessary, I did add some simple smoke tests for the helper. I also do think the helper class could be cleaner, but it does work. And I was hoping to propose some other changes going forward specifically around prometheus and could clean it up then.

Let me know what you think! It's exciting to try and contribute back to a project I use so much.

Mar 29 '24 20:03 jzucker2

I added the labels (except for zone) and also updated the unit tests. It ended up being a huge effort to update the unit tests, they are unintentionally brittle for label changes. I refactored things into a helper class. I know that kind of reduces the legibility of a single test and abstracts a bit, but it should make other label changes much easier going forward.

Thank you!

I don't mind refactoring tests, but I would advise against combining such refactoring in a single PR with other changes.

At the moment it's hard to see the impact of the code change you have made, because all tests have changed as well. Coupling two changes like this makes reviewing (and potentially reverting) each of them more difficult.

May I suggest splitting test refactoring into a separate PR? I don't mind which of the two you'd like to discuss first. I shared some feedback on the specifics of the test refactoring here, and will be happy to discuss more in a separate issue or PR.

I'm willing to split this into 2 prs and keep test refactoring and logic changes separate. How about let's focus on test refactoring first, because simply adding "area" as a label to prometheus will break every single test and I'd love to address that first (I'm also interested in a later pass to include the new tagging and labeling features in Home Assistant in prometheus metrics as well, but let's discuss that later).

Maybe I can make this PR the test refactoring PR since most of the work is around that. And I break out the logic changes into a follow up PR?

Thanks so much for the responses so far! I'm really looking forward to having these features in my Home Assistant set up.

Apr 08 '24 20:04 jzucker2

Maybe I can make this PR the test refactoring PR since most of the work is around that. And I break out the logic changes into a follow up PR?

That sounds good! I've left a few comments about tests in my previous message. In general I think we should be careful about over-abstraction here: the purpose of these tests is to verify that exported metrics have the format that we expect, and to allow folks to understand and debug them I would suggest erring on the side of keeping them simple & explicit even if they seem verbose.

Do you have a list of goals for this refactoring or problems you've identified and want to fix? I think this might be helpful to describe (even if at a high level).

For example, if you find the current approach of hand-crafting full metric strings annoying, I think an alternative might be to parse metric output and examine it in a more structured way.

One thing that I find pretty inconvenient in our current testing approach is that most tests are broken into two functions: a fixture that creates Home Assistant entities, and the test function itself. These two functions are located in different parts of the same file, making it difficult to understand and modify a single test. Many fixtures are only used once. Perhaps moving fixtures closer to the test functions, or maybe combining them in some cases would be simpler? Not sure what's common here in Home Assistant ecosystem - I've not looked at how other components structure their testing, but I think it's worth examining and trying to be consistent with if we are making large changes here.

Apr 10 '24 06:04 knyar

Maybe I can make this PR the test refactoring PR since most of the work is around that. And I break out the logic changes into a follow up PR?

That sounds good! I've left a few comments about tests in my previous message. In general I think we should be careful about over-abstraction here: the purpose of these tests is to verify that exported metrics have the format that we expect, and to allow folks to understand and debug them I would suggest erring on the side of keeping them simple & explicit even if they seem verbose.

Do you have a list of goals for this refactoring or problems you've identified and want to fix? I think this might be helpful to describe (even if at a high level).

For example, if you find the current approach of hand-crafting full metric strings annoying, I think an alternative might be to parse metric output and examine it in a more structured way.

One thing that I find pretty inconvenient in our current testing approach is that most tests are broken into two functions: a fixture that creates Home Assistant entities, and the test function itself. These two functions are located in different parts of the same file, making it difficult to understand and modify a single test. Many fixtures are only used once. Perhaps moving fixtures closer to the test functions, or maybe combining them in some cases would be simpler? Not sure what's common here in Home Assistant ecosystem - I've not looked at how other components structure their testing, but I think it's worth examining and trying to be consistent with if we are making large changes here.

So, long term goal: I wanted to add 2/3 labels here and ended up needing to adjust ~800 lines of tests. But the tests themselves are pretty good. So just hoping that we can have test helpers that are still legible and human-readable but flexible enough to make prometheus label changes reasonable going forward.

I do agree that I overcomplicated this first pass. I do see what you mean about fixtures and stuff.

I also saw your comment about following patterns elsewhere-- it seems this project is so big there's not exactly one way of doing things. But this approach did seem to make sense for the most part, except I definitely think I over-abstracted the refactor.

Apr 10 '24 06:04 jzucker2

I've marked this PR a draft, as changes are requested that need to be processed. Please un-draft it once it is ready for review again by clicking the "Ready for review" button.

Thanks! 👍

../Frenck

Learn more about our pull request process.

Jul 07 '24 21:07 frenck

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. If you are the author of this PR, please leave a comment if you want to keep it open. Also, please rebase your PR onto the latest dev branch to ensure that it's up to date with the latest changes. Thank you for your contribution!

Sep 05 '24 23:09 github-actions[bot]

@jzucker2 do you plan to do split the PR as you mentioned here https://github.com/home-assistant/core/pull/113849#issuecomment-2043612104 ?

Sep 16 '24 08:09 emontnemery

@jzucker2 do you plan to do split the PR as you mentioned here #113849 (comment) ?

Yes! I was thinking maybe this should be the refactor the tests pr because most of the comments and changes really focus on that, especially compared to the logic. And I can split out logic changes into a follow up PR (that hopefully won't require this major amount of test refactoring again)

Sep 17 '24 20:09 jzucker2

As I said before, if this PR is meant to include only the refactor, then you should rename it and remove references to ATTR_AREA_ID, etc.

Sep 17 '24 22:09 llamafilm

@jzucker2 do you plan to do split the PR as you mentioned here #113849 (comment) ?

Yes! I was thinking maybe this should be the refactor the tests pr because most of the comments and changes really focus on that, especially compared to the logic. And I can split out logic changes into a follow up PR (that hopefully won't require this major amount of test refactoring again)

That sounds like a plan! I agree with this comment: https://github.com/home-assistant/core/pull/113849#issuecomment-2357024202

Sep 25 '24 07:09 emontnemery

@jzucker2 do you plan to do split the PR as you mentioned here #113849 (comment) ?

Yes! I was thinking maybe this should be the refactor the tests pr because most of the comments and changes really focus on that, especially compared to the logic. And I can split out logic changes into a follow up PR (that hopefully won't require this major amount of test refactoring again)

That sounds like a plan! I agree with this comment: #113849 (comment)

Awesome. I'll continue forward on that route. I should have a bunch of free time finally starting next week.

Sep 25 '24 19:09 jzucker2

I removed all the logic changes and reworked the helpers to be more in line with the discussions and previous comments. I am ready for a review!

I added a few simple test for the helper class I created.

I chose to keep the helper class in this file because it was pretty small but happy to break it out or change things up if that would be preferred.

I am ready for a review!

Also, as a counterpoint, I could put the new stuff into a test helper class and make the tests scoped around that. I didn't want to overcomplicate things again, but this is pretty flexible at this point. And isolated.

And I also realize, if this PR lands first I'll need to make a few adjustments for one of the added tests on the merge conflict. But nothing changes in the overall approach.

Sep 25 '24 23:09 jzucker2

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

Oct 01 '24 19:10 jzucker2