Skip to content

Add config flow, use async loading, and restore brightness option to ISY994#35413

Merged
bdraco merged 5 commits intohome-assistant:devfrom
shbatm:isy994_v2_pr5
May 9, 2020
Merged

Add config flow, use async loading, and restore brightness option to ISY994#35413
bdraco merged 5 commits intohome-assistant:devfrom
shbatm:isy994_v2_pr5

Conversation

@shbatm
Copy link
Copy Markdown
Contributor

@shbatm shbatm commented May 9, 2020

Breaking change

The ISY994 integration now includes a restore_light_state option. In 0.109.0, a change was made to restore a light's brightness to the previous state when turned on with no brightness parameter. This was, in part, to fix an issue where the light to turn on to full brightness when no parameters were given, regardless of the physical device's On Level brightness setting. Using the On Level is now supported and is the default behavior. To keep the current behavior and use Home Assistant's last brightness, set the restore_light_state is set to True or enable the option in the new config flow options.

Proposed change

This is the fifth PR in a series (~10 total) to include migration to PyISYv2 and "modernization" of the isy994 integration based on testing done over the past year in the HACS custom component.

Full migration plan is captured in PR #35212

This specific PR includes add a config and options flow for the ISY994 integration and adds support for the restore_light_state option.

As part of the config flow changes, unique_ids will be migrated to a new format which includes the uuid of the ISY they are associated with. The config flow supports adding multiple ISYs; however, Z-Wave and NodeServer Nodes can share addresses across ISYs, so an additional identifier is required. This should be a seamless update and is not labelled as a breaking change. Entity IDs should not change.

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

Example entry for configuration.yaml:

# New Optional Setting in configuration.yaml
restore_light_state = false

Additional information

Local Tests Performed

  • Import from configuration.yaml
  • Unload and disconnect on removal from UI.
  • Re-add local connection from Config Flow and reconnect.
  • Add connection to ISY Portal with webroot in URL

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.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

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

Tag: @bdraco, @OverloadUT

shbatm and others added 2 commits May 9, 2020 08:08
Add tests for config flow


Fix test


Update Tests
@bdraco
Copy link
Copy Markdown
Member

bdraco commented May 9, 2020

FAILED tests/components/time_date/test_sensor.py::TestTimeDateSensor::test_states
FAILED tests/helpers/test_template.py::test_timestamp_custom[pyloop] - Assert...
FAILED tests/helpers/test_template.py::test_timestamp_local[pyloop] - Asserti...

Test failure is unrelated

@bdraco
Copy link
Copy Markdown
Member

bdraco commented May 9, 2020

/AzurePipelines Run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

Comment thread homeassistant/components/isy994/const.py
@shbatm
Copy link
Copy Markdown
Contributor Author

shbatm commented May 9, 2020

FAILED tests/components/time_date/test_sensor.py::TestTimeDateSensor::test_states
FAILED tests/helpers/test_template.py::test_timestamp_custom[pyloop] - Assert...
FAILED tests/helpers/test_template.py::test_timestamp_local[pyloop] - Asserti...

Test failure is unrelated

Failed tests are unrelated, but isy994 tests are showing on the list of long runs times. Can you double check and see if there's something I missed that may be resulting in the slow calls?

Comment thread homeassistant/components/isy994/strings.json Outdated
Comment thread homeassistant/components/isy994/strings.json Outdated
Comment thread homeassistant/components/isy994/strings.json Outdated
Comment thread homeassistant/components/isy994/strings.json Outdated
Comment thread homeassistant/components/isy994/strings.json Outdated
Comment thread homeassistant/components/isy994/strings.json Outdated
Comment thread tests/components/isy994/test_config_flow.py Outdated
@bdraco
Copy link
Copy Markdown
Member

bdraco commented May 9, 2020

FAILED tests/components/time_date/test_sensor.py::TestTimeDateSensor::test_states
FAILED tests/helpers/test_template.py::test_timestamp_custom[pyloop] - Assert...
FAILED tests/helpers/test_template.py::test_timestamp_local[pyloop] - Asserti...

Test failure is unrelated

Failed tests are unrelated, but isy994 tests are showing on the list of long runs times. Can you double check and see if there's something I missed that may be resulting in the slow calls?

The ones that will actually get to creating an entry need this in the with block so they don't actually startup. This is generally the one place where its ok to not mock the external library.

patch(
        "homeassistant.components.isy994.async_setup", return_value=True
    ) as mock_setup, patch(
        "homeassistant.components.isy994.async_setup_entry", return_value=True,
    ) as mock_setup_entry:

shbatm and others added 3 commits May 9, 2020 10:17
@shbatm
Copy link
Copy Markdown
Contributor Author

shbatm commented May 9, 2020

Fixed patches.

@bdraco
Copy link
Copy Markdown
Member

bdraco commented May 9, 2020

At this point the code looks good so I'm doing another manual test run though

@bdraco
Copy link
Copy Markdown
Member

bdraco commented May 9, 2020

Passes the 'siri, turn on all the lights test' in ~98 seconds.

148 context switches since requests are sync 🙈 When this is all merged up, we should look at converting at least the rest api requests to async.

Comment thread tests/components/isy994/test_config_flow.py
@bdraco bdraco merged commit d7f736e into home-assistant:dev May 9, 2020
@shbatm
Copy link
Copy Markdown
Contributor Author

shbatm commented May 9, 2020

Passes the 'siri, turn on all the lights test' in ~98 seconds.

148 context switches since requests are sync 🙈 When this is all merged up, we should look at converting at least the rest api requests to async.

I'm surprised the ISY didn't choke on that many requests. More motivation for automicus/PyISY#53 (comment)

Test will be updated for next PR.

Also have @MartinHjelmare suggested changed to sensors / binary sensor states queued for the end of this PR chain, but for best effect it will need automicus/PyISY#100 merged.

@shbatm shbatm deleted the isy994_v2_pr5 branch May 9, 2020 22:25
Copy link
Copy Markdown

@McGiverGim McGiverGim left a comment

Choose a reason for hiding this comment

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

I have detected this errors in lokalise.

Comment thread homeassistant/components/isy994/strings.json
Comment thread homeassistant/components/isy994/strings.json
@McGiverGim
Copy link
Copy Markdown

I know this PR has been merged, but I have detected this two errors in lokalise.

@shbatm
Copy link
Copy Markdown
Contributor Author

shbatm commented May 11, 2020

Fixed in open PR #35467.

@lock lock Bot locked and limited conversation to collaborators May 21, 2020
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.

4 participants