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

Growatt sensor #21004

Open
wants to merge 35 commits into
base: dev
from

Conversation

@timvancann
Copy link
Contributor

commented Feb 12, 2019

Description:

Related issue (if applicable): fixes #

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#8496

Example entry for configuration.yaml (if applicable):

See documentation

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

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

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

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

timvancann added some commits Jan 30, 2019

Implement helper functions
- Add tests

@ghost ghost added the in progress label Feb 12, 2019

@timvancann timvancann referenced this pull request Feb 12, 2019

Open

Growatt sensor #8496

2 of 2 tasks complete

@timvancann timvancann force-pushed the timvancann:growatt-sensor branch from 638b2c6 to 19e110c Feb 15, 2019

Show resolved Hide resolved tests/components/sensor/test_growatt.py Outdated
Show resolved Hide resolved tests/components/sensor/test_growatt.py Outdated
Show resolved Hide resolved tests/components/sensor/test_growatt.py Outdated
Show resolved Hide resolved tests/components/sensor/test_growatt.py Outdated

@timvancann timvancann force-pushed the timvancann:growatt-sensor branch from 0ae27af to 7f168c1 Feb 15, 2019

@timvancann timvancann force-pushed the timvancann:growatt-sensor branch from 7f168c1 to 6d20850 Feb 15, 2019

@timvancann timvancann force-pushed the timvancann:growatt-sensor branch 2 times, most recently from 5de05a8 to 8e27d4e Feb 20, 2019

@timvancann timvancann force-pushed the timvancann:growatt-sensor branch 3 times, most recently from 9058f8c to ca4d3a0 Feb 20, 2019

@timvancann timvancann force-pushed the timvancann:growatt-sensor branch from ca4d3a0 to 3e2c1f7 Feb 22, 2019

@amelchio amelchio removed their assignment Mar 30, 2019

@timvancann timvancann requested a review from home-assistant/core as a code owner Mar 31, 2019

@timvancann timvancann force-pushed the timvancann:growatt-sensor branch from ca52aff to 127c0ef Mar 31, 2019

@timvancann timvancann force-pushed the timvancann:growatt-sensor branch from 127c0ef to 382caf4 Mar 31, 2019

@timvancann

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

@awarecan @dgomes Addressed the comments.

Merge branch 'dev' into growatt-sensor
# Conflicts:
#	requirements_all.txt
@andrewsayre

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

Are you still working on this PR? Our architecture has changed and the code will need to be updated if you wish to continue with this contribution.

timvancann added some commits Apr 27, 2019

@timvancann timvancann force-pushed the timvancann:growatt-sensor branch from f19e398 to e459c3e Apr 27, 2019

@timvancann timvancann force-pushed the timvancann:growatt-sensor branch from e3931e1 to ef72d32 Apr 27, 2019

@timvancann

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2019

@andrewsayre I wrapped up the implementation and PR about a month ago. Never got merged so I stopped resolving conflicts with the dev branch.

I'd love for it to get merged still; I've updated the component to the new architecture.

@timvancann timvancann force-pushed the timvancann:growatt-sensor branch from c87bb8d to 4d4c3a6 Apr 27, 2019

@awarecan
Copy link
Contributor

left a comment

Still have several small changes need made

client.login(username, password)
return True
except growatt.LoginError as error:
logging.error(error)

This comment has been minimized.

Copy link
@awarecan

awarecan Apr 27, 2019

Contributor

Not resolved yet

"Found an unsupported metric name {}"
"cannot convert safely to kWh."
).format(metric_name)
logging.error(message)

This comment has been minimized.

Copy link
@awarecan

awarecan Apr 27, 2019

Contributor

Need change to _LOGGER.error()



@mock.patch("growatt.GrowattApi.login", return_value={"userId": "1"})
@mock.patch("growatt.GrowattApi.plant_list", return_value=dummy_plant_info)

This comment has been minimized.

Copy link
@awarecan

awarecan Apr 27, 2019

Contributor

Still need change

outdated

timvancann added some commits May 1, 2019

@andrewsayre

This comment has been minimized.

Copy link
Member

commented May 5, 2019

You need to address the drop in code coverage. Either add tests to get to the target 93.89% or update .coveragerc.

@andrewsayre
Copy link
Member

left a comment

Let me know if you have any questions!

"cannot convert safely to kWh."
).format(metric_name)
_LOGGER.error(message)
raise ValueError(message)

This comment has been minimized.

Copy link
@andrewsayre

andrewsayre May 5, 2019

Member

Don't let a ValueError propagate up the chain. Raise a HomeAssistantError and catch it in update and then log the error message. (update shouldn't raise exceptions by design).

class GrowattPlant(Entity):
"""Base Growatt sensor class."""

def __init__(self, hass, client, username, password):

This comment has been minimized.

Copy link
@andrewsayre

andrewsayre May 5, 2019

Member

Do not pass in hass. It is available as self.hass after the entity is added. Please change here and in the derived classes.

Refreshes login to update the session.
"""
login(self._client, self._username, self._password)

This comment has been minimized.

Copy link
@andrewsayre

andrewsayre May 5, 2019

Member

Need to check if login is successful, otherwise the rest of the code shouldn't run, right?


def update(self):
"""Get the latest data from Growatt server."""
self._state = self._get_total_energy(self._metric_name)

This comment has been minimized.

Copy link
@andrewsayre

andrewsayre May 5, 2019

Member

As mentioned above, don't raise exceptions in update by design. The current code would allow a ValueError to propagate up. Instead, catch the HomeAssistantError here and log it as an error.

Refreshes login to update the session.
"""
login(self._client, self._username, self._password)

This comment has been minimized.

Copy link
@andrewsayre

andrewsayre May 5, 2019

Member

Need condition for when login returns false.

"""
login(self._client, self._username, self._password)
plant_info = self._client.plant_list()
new_state = float(

This comment has been minimized.

Copy link
@andrewsayre

andrewsayre May 5, 2019

Member

Remove float conversion here.

@@ -0,0 +1,107 @@
"""The tests for the Growatt platform."""

This comment has been minimized.

Copy link
@andrewsayre

andrewsayre May 5, 2019

Member

I highly suggest rewriting these tests and assert the states through the infrastructure of HomeAssistant. This will reflect the actual scenarios that the code is invoked. Take a look at these tests as an example. The general pattern is to setup the platform, which will load and add the entities, then assert the state through the state machine. Manipulate the conditions of the code by mocking the values returned in the API and fetching the updated state through the state machine.

timvancann added some commits May 6, 2019

@timvancann timvancann force-pushed the timvancann:growatt-sensor branch from 776f1c8 to aaaeed2 May 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.