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

Add laundrify integration #65090

Merged
merged 19 commits into from
May 21, 2022
Merged

Add laundrify integration #65090

merged 19 commits into from
May 21, 2022

Conversation

xLarry
Copy link
Contributor

@xLarry xLarry commented Jan 27, 2022

Proposed change

This PR adds a new integration for the laundrify WiFi power plugs. The purpose of the power plugs is to monitor the status of washing machines and dryers.

This integration will add a Binary Sensor for each power plug that has been set up in the laundrify App. To configure the integration, the user needs to generate an Auth Code in the laundrify App.

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:

Copy link
Contributor

@austinmroczek austinmroczek left a comment

Choose a reason for hiding this comment

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

This looks pretty good. A few minor changes requested.

I wish these were available with U.S. plugs.

homeassistant/components/laundrify/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/laundrify/binary_sensor.py Outdated Show resolved Hide resolved
tests/components/laundrify/__init__.py Outdated Show resolved Hide resolved
tests/components/laundrify/test_config_flow.py Outdated Show resolved Hide resolved
@austinmroczek
Copy link
Contributor

Also, please remove "breaking change" from the PR text at the top. Since this is a new integration it is not a breaking change.

@xLarry
Copy link
Contributor Author

xLarry commented Jan 30, 2022

Thanks for your feedback @austinmroczek. I addressed your comments in my last commit and changed the description of the PR.

Copy link
Contributor

@austinmroczek austinmroczek left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for the all of the work.

Copy link
Contributor

@iMicknl iMicknl left a comment

Choose a reason for hiding this comment

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

Had a quick look to get you started.

homeassistant/components/laundrify/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/laundrify/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/laundrify/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/laundrify/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/laundrify/binary_sensor.py Outdated Show resolved Hide resolved
homeassistant/components/laundrify/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/laundrify/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/laundrify/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/laundrify/binary_sensor.py Outdated Show resolved Hide resolved
homeassistant/components/laundrify/binary_sensor.py Outdated Show resolved Hide resolved
@xLarry
Copy link
Contributor Author

xLarry commented Mar 2, 2022

Thanks for your input, @iMicknl. I went through your comments and referenced the corresponding commit in my answer.

@mschabhuettl

This comment was marked as off-topic.

@mirkolenz

This comment was marked as off-topic.

Copy link
Contributor

@iMicknl iMicknl left a comment

Choose a reason for hiding this comment

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

Some more small remarks.

homeassistant/components/laundrify/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/laundrify/binary_sensor.py Outdated Show resolved Hide resolved
homeassistant/components/laundrify/binary_sensor.py Outdated Show resolved Hide resolved
homeassistant/components/laundrify/binary_sensor.py Outdated Show resolved Hide resolved
homeassistant/components/laundrify/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/laundrify/binary_sensor.py Outdated Show resolved Hide resolved
@xLarry
Copy link
Contributor Author

xLarry commented Apr 20, 2022

Sorry for the delay. @iMicknl: I addressed your feedback in the latest 2 commits. Let me know if anything is missing :)

@mschabhuettl

This comment was marked as off-topic.

@Kayinwonderland

This comment was marked as off-topic.

Copy link
Contributor

@raman325 raman325 left a comment

Choose a reason for hiding this comment

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

this looks good to me. Last thing to do is improve test coverage so coverage check will pass

@xLarry
Copy link
Contributor Author

xLarry commented May 13, 2022

Thanks for your time and valuable input, @raman325!

I added some tests and increased the coverage to 100%. Looking forward to hopefully pass the CI run now 😀

Copy link
Contributor

@raman325 raman325 left a comment

Choose a reason for hiding this comment

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

Apologies, I didn't look at your tests. While not technically wrong, making these changes will make your tests much cleaner and will avoid some of the indenting

tests/components/laundrify/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/laundrify/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/laundrify/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/laundrify/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/laundrify/test_config_flow.py Outdated Show resolved Hide resolved
@xLarry
Copy link
Contributor Author

xLarry commented May 16, 2022

Thanks for pointing out. I didn't know about fixtures (first python project), but it actually felt like I was missing something 😀

I created a fixture for the API responses that's using autouse=True and some others for whenever a side effect is required. Let me know if there's something else to improve :)

@xLarry xLarry requested a review from raman325 May 20, 2022 08:54
Copy link
Contributor

@raman325 raman325 left a comment

Choose a reason for hiding this comment

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

Great work on this, thanks for the contribution! Can be merged as soon as tests pass

@xLarry
Copy link
Contributor Author

xLarry commented May 21, 2022

Nice 🤩 Thanks for your feedback!

Looks like the tests failed due to some dependency conflicts. Is it something I can resolve?

@raman325
Copy link
Contributor

nope not your problem!

@raman325 raman325 merged commit abf9aab into home-assistant:dev May 21, 2022
@github-actions github-actions bot locked and limited conversation to collaborators May 22, 2022
@xLarry xLarry deleted the laundrify branch May 27, 2022 14:23
@xLarry xLarry mentioned this pull request May 27, 2022
22 tasks
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Please address the comments in a new PR. Thanks!

"""Handle a flow initialized by the user."""
return await self.async_step_init(user_input)

async def async_step_init(
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to define this init step. Just merge this with the user step.

from typing import TypedDict


class LaundrifyDevice(TypedDict):
Copy link
Member

Choose a reason for hiding this comment

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

The library should preferably define its model, eg with a dataclass, so we don't need to define the type here but can use the type from the library.

"unknown": "[%key:common::config_flow::error::unknown%]"
},
"abort": {
"single_instance_allowed": "[%key:common::config_flow::abort::single_instance_allowed%]"
Copy link
Member

Choose a reason for hiding this comment

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

single_instance_allowed abort reason is never used. We're missing a reason for already_configured though.

)
await hass.async_block_till_done()

assert result["type"] == RESULT_TYPE_FORM
Copy link
Member

Choose a reason for hiding this comment

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

Please continue the flow until the reauthentication is completed.


async def test_step_reauth(hass: HomeAssistant) -> None:
"""Test the reauth form is shown."""
result = await hass.config_entries.flow.async_init(
Copy link
Member

Choose a reason for hiding this comment

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

We need to add a MockConfigEntry before starting the reauth flow. At the end of the test we should assert that the entry data has been updated.

config_entry = create_entry(hass)
await hass.config_entries.async_setup(config_entry.entry_id)
await hass.async_block_till_done()
coordinator = hass.data[DOMAIN][config_entry.entry_id]["coordinator"]
Copy link
Member

Choose a reason for hiding this comment

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

Please don't access integration details like hass.data or the coordinator in the tests.

https://developers.home-assistant.io/docs/development_testing#writing-tests-for-integrations

await coordinator.async_refresh()
await hass.async_block_till_done()

assert coordinator.last_update_success
Copy link
Member

Choose a reason for hiding this comment

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

We can assert the state of an entity instead.

await hass.config_entries.async_setup(config_entry.entry_id)
await hass.async_block_till_done()
coordinator = hass.data[DOMAIN][config_entry.entry_id]["coordinator"]
await coordinator.async_refresh()
Copy link
Member

Choose a reason for hiding this comment

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

We can move time forward to make the coordinator do a refresh.

core/tests/common.py

Lines 375 to 379 in d59ecc4

@ha.callback
def async_fire_time_changed(
hass: HomeAssistant, datetime_: datetime = None, fire_all: bool = False
) -> None:
"""Fire a time changed event."""

@@ -0,0 +1,50 @@
"""Test the laundrify coordinator."""
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't test the coordinator directly. Test the binary sensor platform instead by setting up the integration and asserting the state of the entity in the core state machine.

@raman325
Copy link
Contributor

raman325 commented Jun 1, 2022

@xLarry did you see these additional comments from Martin? these should get addressed soon, particularly the missing string

@home-assistant home-assistant unlocked this conversation Jun 1, 2022
@xLarry
Copy link
Contributor Author

xLarry commented Jun 1, 2022

Sure! I'll create a new PR asap.

@raman325
Copy link
Contributor

raman325 commented Jun 1, 2022

Sure! I'll create a new PR asap.

I would submit the strings fix as a separate PR so that we can include it in a patch release

@github-actions github-actions bot locked and limited conversation to collaborators Jun 2, 2022
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.

9 participants