Add Plugwise scan_interval config option by bouwew · Pull Request #37229 · home-assistant/core

bouwew · 2020-06-29T17:31:22Z

Proposed change

Add OPTIONS to the config-flow: make it possible to change the interval between data-updates from the Smile(s).

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

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

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:

Documentation added/updated for www.home-assistant.io:

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

probot-home-assistant · 2020-06-29T17:31:29Z

Hey there @CoMPaTech, mind taking a look at this pull request as its been labeled with an integration (plugwise) you are listed as a codeowner for? Thanks!
_{^{(message by CodeOwnersMention)}}

CoMPaTech

OK from me as co-author

bouwew · 2020-07-04T09:54:00Z

Ok, ready for rereview now, sorry, I'm still learning...

CoMPaTech

Looking good and functional on test and devcontainer

frenck · 2020-07-04T17:37:03Z

The tests need to be extended as the config flow requires a 100% test coverage.

Furthermore, do we really need this to be user-configurable? As in, in general we should have a sane default for the scan interval. Is there a specific reason for adding this?

bouwew · 2020-07-04T17:48:39Z

@frenck Clear on the test-coverage requirement.

Several users have requested this feature, they would prefer to use different interval-values than were set as the defaults.
Also, there was no clear preference from the users which default scan_interval to set, so we've set them to the Core-default for Climate (60 secs) and to 10 sec for the Smile P1, as this is the DSMR scan interval. And promised to provide a settable scan_interval per Smile integration.

CoMPaTech · 2020-07-14T12:02:30Z

@frenck do you consider this a viable PR (the testing requirement is clear, but I guess we didn't trigger the 100% on config_flow in #37289 if I'm honest)

As in ... we can provide the required testing, either in this PR or the pending full(er) testing PR that's still open to solve the already merged zeroconf one.

frenck · 2020-07-14T14:13:20Z

I'm fine with a scan interval, just saying it is general better to avoid it. Hence my questioning.

The test requirements have to be met. Config flows require a 100% coverage.

CoMPaTech · 2020-07-18T19:51:01Z

Still working on that, not much progress yet, aiming for next week

CoMPaTech · 2020-07-27T19:29:15Z

Additional tests added

bouwew · 2020-08-08T14:52:51Z

@frenck What is the status of this PR from your point-of-view? Can this one be merged?

We are working on several fixes for the Plugwise integration, related to the recently reported issues.
This PR is kind of blocking our progress :)

CoMPaTech · 2020-08-20T17:38:28Z

Catch-up with dev

CoMPaTech · 2020-09-02T20:52:53Z

Catchup with dev and conflict resolve

CoMPaTech · 2020-09-04T18:58:43Z

Tested locally against newer pending release of the module as requested in plugwise/Plugwise-Smile#86

bdraco · 2020-09-04T19:22:20Z

    if single_master_thermostat is None:
        platforms = SENSOR_PLATFORMS

+    entry.add_update_listener(_update_listener)


Please store and unsubscribe the listener when the config entry is unloaded to prevent it leaking. Example in nut

Adding/testing

Hopefully implemented as indicated, passing local functionality and commit-checks.

CoMPaTech · 2020-09-05T18:11:17Z

Looks like we've made some progress ... food and snacks tend to help. Also tested locally with newer module version so we can add a version-bump PR once this merges to solve plugwise/Plugwise-Smile#86 (module dependencies module vs core)

CoMPaTech · 2020-09-06T07:19:09Z

Seems CI checks and codecov concur :)

CoMPaTech · 2020-09-06T17:34:32Z

@bdraco Do we still want to pass this for 0.115 beta including a quick additional PR for module version bump as per @MartinHjelmare for plugwise/Plugwise-Smile#86?

bdraco · 2020-09-06T18:44:35Z

 tests/components/plugwise/test_config_flow.py ✓✓✓✓✓✓✓✓                                                                                                                                                                                                              100% ██████████

----------- coverage: platform linux, python 3.8.1-final-0 -----------
Name                                               Stmts   Miss  Cover   Missing
--------------------------------------------------------------------------------
homeassistant/components/plugwise/config_flow.py      80      0   100%
homeassistant/components/plugwise/const.py            31      0   100%
--------------------------------------------------------------------------------
TOTAL                                                111      0   100%

CoMPaTech · 2020-09-06T20:27:31Z

Tnx @bdraco

MartinHjelmare · 2020-09-06T21:50:09Z

+    hass.data[DOMAIN] = {entry.entry_id: {"api": MagicMock(smile_type="power")}}
+    entry.add_to_hass(hass)
+
+    result = await hass.config_entries.options.async_init(entry.entry_id)


We should set up the config entry before starting the options flow.

The options flow requires the config flow module to be loaded. This will happen when setting up the config entry. If CI splits testing the config flow might not be loaded by the other tests when the options flow tests run.

Tnx, will look into this and the below comment and get an PR started later today

#39736 it is

MartinHjelmare · 2020-09-06T21:52:37Z

+
+async def test_show_zeroconf_form(hass, mock_smile) -> None:
+    """Test that the zeroconf confirmation form is served."""
+    flow = config_flow.PlugwiseConfigFlow()


Always start the tests with the core interfaces. Here use hass.config_entries.flow.async_init.

Overlooked when refitting between PRs, that def is redundant now that 78: async def test_zeroconf_form(hass): is in. Update PR will be in later today

homeassistant added the cla-signed label Jun 29, 2020

probot-home-assistant Bot added integration: plugwise new-feature by-code-owner labels Jun 29, 2020

CoMPaTech approved these changes Jun 29, 2020

View reviewed changes

This was referenced Jun 29, 2020

Plugwise: include info w.r.t. Zeroconf discovery home-assistant/home-assistant.io#13868

Merged

Add Plugwise zeroconf discovery #37289

Merged

bouwew force-pushed the plugwise-config_options branch from 6896f9a to bf9809f Compare July 4, 2020 08:52

CoMPaTech approved these changes Jul 4, 2020

View reviewed changes

MartinHjelmare changed the title ~~Plugwise - Add config options - scan_interval~~ Add Plugwise scan_interval config option Jul 5, 2020

bouwew mentioned this pull request Jul 7, 2020

Plugwise - describe how to use OPTIONS home-assistant/home-assistant.io#13940

Merged

8 tasks

CoMPaTech mentioned this pull request Jul 27, 2020

Allow plugwise port number as IP address in config flow #38276

Closed

20 tasks

CoMPaTech force-pushed the plugwise-config_options branch from 664e2ee to 504a284 Compare July 27, 2020 18:53

CoMPaTech mentioned this pull request Aug 4, 2020

Add tests for Plugwise integration #36371

Merged

20 tasks

CoMPaTech force-pushed the plugwise-config_options branch from b0f90ad to 4f96695 Compare August 20, 2020 17:37

CoMPaTech force-pushed the plugwise-config_options branch from 4f96695 to 1aa7bae Compare September 2, 2020 17:23

CoMPaTech force-pushed the plugwise-config_options branch from f5715c3 to f0a21e0 Compare September 4, 2020 18:38

bdraco reviewed Sep 4, 2020

View reviewed changes

Tom Scholten and others added 14 commits September 6, 2020 12:41

Allow scan_interval adjustments using config_flow

ea6d085

Update to latest beta-master-code

44475ae

Update to the latest beta-master-code

1c110b7

Add config_flow options - scan_interval

05976a2

final touches

afd8d15

Remove left-over VSC comments

bf77c3d

Fix errors in string.json

2bb1f8e

92% coverage on config_flow

c2c70c5

Use consts properly

9d61827

Added nut way of undo as suggested

4acea8c

Add exception testing, still missing last bits

53b9040

Fixing line 97 coverage

6aacd6a

Overlooked early return, fixed to 100% coverage

44dff35

Align with home-assistant#36371 / 976d422

bca3ce8

CoMPaTech force-pushed the plugwise-config_options branch from b0dff64 to bca3ce8 Compare September 6, 2020 10:42

bdraco reviewed Sep 6, 2020

View reviewed changes

Comment thread homeassistant/components/plugwise/config_flow.py Outdated

bdraco reviewed Sep 6, 2020

View reviewed changes

Comment thread homeassistant/components/plugwise/__init__.py Outdated

CoMPaTech added 3 commits September 6, 2020 21:03

Deflate amount of interval statements

78f00bf

Adjust test accordingly

44a0201

Empty push retrigger tests (light failing)

cacbcb4

bdraco approved these changes Sep 6, 2020

View reviewed changes

bdraco merged commit 61e8ab1 into home-assistant:dev Sep 6, 2020

MartinHjelmare reviewed Sep 6, 2020

View reviewed changes

CoMPaTech mentioned this pull request Sep 7, 2020

Improve Plugwise config_options testing #39736

Merged

16 tasks

CoMPaTech deleted the plugwise-config_options branch September 7, 2020 12:43

Uh oh!

Conversation

bouwew commented Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed change

Type of change

Additional information

Checklist

Uh oh!

probot-home-assistant Bot commented Jun 29, 2020

Uh oh!

CoMPaTech left a comment

Choose a reason for hiding this comment

Uh oh!

bouwew commented Jul 4, 2020

Uh oh!

CoMPaTech left a comment

Choose a reason for hiding this comment

Uh oh!

frenck commented Jul 4, 2020

Uh oh!

bouwew commented Jul 4, 2020

Uh oh!

CoMPaTech commented Jul 14, 2020

Uh oh!

frenck commented Jul 14, 2020

Uh oh!

CoMPaTech commented Jul 18, 2020

Uh oh!

CoMPaTech commented Jul 27, 2020

Uh oh!

bouwew commented Aug 8, 2020

Uh oh!

CoMPaTech commented Aug 20, 2020

Uh oh!

CoMPaTech commented Sep 2, 2020

Uh oh!

CoMPaTech commented Sep 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CoMPaTech commented Sep 5, 2020

Uh oh!

CoMPaTech commented Sep 6, 2020

Uh oh!

CoMPaTech commented Sep 6, 2020

Uh oh!

bdraco commented Sep 6, 2020

Uh oh!

Uh oh!

Uh oh!

CoMPaTech commented Sep 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bouwew commented Jun 29, 2020 •

edited

Loading