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

Forecast fix for Home Assistant >= 2024.4.0 #150

Merged
merged 6 commits into from
May 8, 2024
Merged

Forecast fix for Home Assistant >= 2024.4.0 #150

merged 6 commits into from
May 8, 2024

Conversation

fcastilloec
Copy link
Contributor

@fcastilloec fcastilloec commented Apr 6, 2024

Breaking change

Proposed change

Fix support for new service request for forecast

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (which adds functionality to an this integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml

Additional information

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • The code has been formatted using Black (black --fast custom_components)

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

  • Documentation added/updated to README.md

@fcastilloec
Copy link
Contributor Author

I tried my best for tests to pass, both pre-commit and CI tests but I had trouble with mypy and pylint local tests.
Also, testing the integration itself was challenging since implementing a mock weather integration is something I'm not familiar with. I've tested with various weather providers that implement the new get_forecasts service request and it's working without issues.

@phedoreanu
Copy link
Contributor

@Limych any chance for a review? 😃

Copy link
Contributor

@denysdovhan denysdovhan left a comment

Choose a reason for hiding this comment

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

Please, merge this PR

@Limych
Copy link
Owner

Limych commented Apr 18, 2024

Please, fix errors in requirements and add unit tests for your code.

@Limych Limych marked this pull request as draft April 18, 2024 10:25
@fcastilloec
Copy link
Contributor Author

@Limych Thanks for taking a look at this PR. As I mentioned earlier, I can't fix all the tests needed for this PR.
I've fixed the lint testing but I can't fix "Test Package" because I don't know how to create a mock weather integration for testing.
For the moment, I included @pytest.mark.skip(reason="Can't mock a weather integration") on the test_async_update so I can make sure that the other test, which I know how they work, can run and pass.

I hope you can take this PR and make the modifications yourself so the main test can pass. If somebody else knows how to mock a service call to a weather integration, and can help me with it, I'd appreciate it. At the moment, this is as far as I can go.

For anybody else following this PR and needs a fix immediately, check the two following files in this PR (or my fork), and copy/paste them under your setup:
custom_components/snowtire/binary_sensor.py
custom_components/snowtire/manifest.json
All the other files changed in this PR are only for the tests, pre-commit, dev-container, etc; and aren't needed for the actual integration.

@Limych
Copy link
Owner

Limych commented Apr 19, 2024

Ok, thanks for your work.

Unfortunately, I cannot accept PR without full-fledged unit tests, because then no one will do them and this will entail a deterioration in the quality of the code of the entire component.
And right now, unfortunately, I don’t have the opportunity to work on this component yet.

@fcastilloec fcastilloec marked this pull request as ready for review April 24, 2024 18:50
@fcastilloec
Copy link
Contributor Author

@Limych after some googling and reading, I found out how to mock a service call. The unit test has been fixed to implement it, and the tests should be passing. Can you approve it so the workflow runs? If there are any additional errors after that, I could fix them.

@phedoreanu
Copy link
Contributor

phedoreanu commented May 7, 2024

@Limych please merge it until the end of October 😅

Edit: Also please approve the workflow

@Limych Limych merged commit b00023a into Limych:dev May 8, 2024
8 checks passed
@Limych
Copy link
Owner

Limych commented May 8, 2024

Thanks ❤️

This was referenced May 8, 2024
Limych added a commit that referenced this pull request May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken on HA 2024.4.0b0
4 participants