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 refactor testcases in async pytest style #60935

Merged

Conversation

ThomDietrich
Copy link
Contributor

@ThomDietrich ThomDietrich commented Dec 3, 2021

Proposed change

As discussed in #59205 (comment)

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)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

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

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.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@probot-home-assistant
Copy link

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

@ThomDietrich
Copy link
Contributor Author

@emontnemery @MartinHjelmare I'm struggling to get the testcases linked to the recorder to work. I suppose I need to use async_wait_recording_done but a recorder instance is needed as parameter and I have no idea how to retrieve it. Any help appreciated!

Please ignore changes to sensor.py! These changes will vanish after #59205 is merged.

@emontnemery
Copy link
Contributor

There's a helper async_wait_recording_done_without_instance, have a look at some tests using that.
Please contact me on discord, emontnemery#6618, if you need some help making the tests run.

@ThomDietrich ThomDietrich mentioned this pull request Dec 4, 2021
22 tasks
@ThomDietrich ThomDietrich marked this pull request as ready for review December 4, 2021 14:44
@ThomDietrich
Copy link
Contributor Author

@emontnemery thanks for your help. Ready for review, don't hold back to suggest improvements to the general method

Copy link
Contributor

@emontnemery emontnemery left a comment

Choose a reason for hiding this comment

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

Let's try to clean up the excessive use of await hass.async_block_till_done() to make sure we don't hide bugs.
If the tests don't pass unless we call await hass.async_block_till_done() in the loops with state updates there may be something wrong with the statistics sensor.

I commented on the first few occasions, but the pattern seems to be repeated.

Also, please restore the order of the test cases to make the PR easier to review.
Reordering of the tests can be done in a follow-up if that's called for 👍

tests/components/statistics/test_sensor.py Outdated Show resolved Hide resolved
tests/components/statistics/test_sensor.py Outdated Show resolved Hide resolved
tests/components/statistics/test_sensor.py Outdated Show resolved Hide resolved
tests/components/statistics/test_sensor.py Outdated Show resolved Hide resolved
tests/components/statistics/test_sensor.py Outdated Show resolved Hide resolved
@ThomDietrich
Copy link
Contributor Author

Agree. I pretty much inherited these await hass.async_block_till_done() but they are now tided up. Also, I did fix some inconsistent use of async_fire_time_changed to simulate the expiry of values in buffer.

Original order restored, even though that doesn't help GitHub or delta to understand the changes :)

Best!

@emontnemery
Copy link
Contributor

Original order restored, even though that doesn't help GitHub or delta to understand the changes

Thanks a lot! It doesn't help in github, but it helps a lot in external tools.

Copy link
Contributor

@emontnemery emontnemery left a comment

Choose a reason for hiding this comment

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

Thanks, @ThomDietrich 👍

@emontnemery emontnemery merged commit bbe6d3c into home-assistant:dev Dec 8, 2021
@ThomDietrich ThomDietrich deleted the statistics-pytest-testcases branch December 8, 2021 16:00
@github-actions github-actions bot locked and limited conversation to collaborators Dec 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rewrite statistics unittest tests to pytest style test functions
4 participants