Forecast fix for Home Assistant >= 2024.4.0

fcastilloec commented 6 months ago

Breaking change

Proposed change

Fix support for new service request for forecast

Type of change

[x] Dependency upgrade
[x] 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)
[x] Code quality improvements to existing code or addition of tests

Example entry for `configuration.yaml`:

# Example configuration.yaml

Additional information

This PR fixes or closes issue: fixes #149

Checklist

[x] The code change is tested and works locally.
[x] There is no commented out code in this PR.
[x] 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 commented 6 months ago

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 commented 6 months ago

@Limych any chance for a review? 😃

Limych commented 6 months ago

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

fcastilloec commented 6 months ago

@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 commented 6 months ago

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 commented 5 months ago

@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 commented 5 months ago

@Limych please merge it until the end of October 😅

Edit: Also please approve the workflow

Limych commented 5 months ago

Thanks :heart:

Limych / ha-snowtire