Skip to content

Add tests for Plugwise integration#36371

Merged
MartinHjelmare merged 21 commits intohome-assistant:devfrom
plugwise:plugwise-testing
Sep 8, 2020
Merged

Add tests for Plugwise integration#36371
MartinHjelmare merged 21 commits intohome-assistant:devfrom
plugwise:plugwise-testing

Conversation

@CoMPaTech
Copy link
Copy Markdown
Member

Proposed change

Add testing fixtures and tests (besides the mandatory config_flow for the Plugwise integration. Earlier I prepared a version with aiohttp mocking, but that would generate a hefty amount of XML data in fixtures (even gzipped), rewrote using pickles written out by our API module from that same data.

I'm trying to keep the amount of fixtures as low as possible - I could tidy up a bit more by using a smaller environment, though the selected three environments to run the tests against are about the most diverse we have test data available for from our API module development.

Comments or recommendations welcome (here or on discord)

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][dev-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][docs-repository]

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

  • The [manifest file][manifest-docs] 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][quality-scale]:

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

@probot-home-assistant
Copy link
Copy Markdown

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

@CoMPaTech CoMPaTech marked this pull request as ready for review June 2, 2020 11:29
@CoMPaTech CoMPaTech marked this pull request as draft June 2, 2020 13:58
@CoMPaTech
Copy link
Copy Markdown
Member Author

Set to draft so #36378 can take presedence and we can beef up this one with binary_sensors and switch tests.

@CoMPaTech CoMPaTech force-pushed the plugwise-testing branch 2 times, most recently from b360a5f to c0dd48e Compare June 2, 2020 21:25
@CoMPaTech
Copy link
Copy Markdown
Member Author

Not sure why codecov is complaining (in more detail, it's complaining on coverage differences outside my changes).

Local coverage report shows:

collecting ...
 tests/components/plugwise/test_binary_sensor.py ✓✓                                                                                                                                                                             11% █▏
 tests/components/plugwise/test_climate.py ✓✓✓✓                                                                                                                                                                                 32% ███▎
 tests/components/plugwise/test_config_flow.py ✓✓✓                                                                                                                                                                              47% ████▊
 tests/components/plugwise/test_init.py ✓✓✓✓✓                                                                                                                                                                                   74% ███████▍
 tests/components/plugwise/test_sensor.py ✓✓✓                                                                                                                                                                                   89% ████████▉
 tests/components/plugwise/test_switch.py ✓✓                                                                                                                                                                                   100% ██████████

---------- coverage: platform darwin, python 3.7.3-final-0 -----------
Name                                                 Stmts   Miss  Cover   Missing
----------------------------------------------------------------------------------
homeassistant/components/plugwise/__init__.py           99      6    94%   63, 108, 120-131
homeassistant/components/plugwise/binary_sensor.py      55     11    80%   68, 81-83, 86-87, 97-101
homeassistant/components/plugwise/climate.py           165     24    85%   106, 108, 110-113, 142, 159, 191-194, 200-205, 212-213, 222-223, 263-265
homeassistant/components/plugwise/config_flow.py        42      6    86%   61-63, 66-69
homeassistant/components/plugwise/const.py              29      0   100%
homeassistant/components/plugwise/sensor.py            128     34    73%   209-222, 284-286, 304-308, 313, 318-339, 365-367
homeassistant/components/plugwise/switch.py             49      7    86%   57-58, 66-67, 77-79
----------------------------------------------------------------------------------
TOTAL                                                  567     88    84%

@CoMPaTech
Copy link
Copy Markdown
Member Author

Low score on sensor.py is largely affecting all of them (Question: how to trigger a DataUpdateCoordinator callback through testing?)

@CoMPaTech CoMPaTech marked this pull request as ready for review June 4, 2020 21:28
Copy link
Copy Markdown
Contributor

@bouwew bouwew left a comment

Choose a reason for hiding this comment

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

Ok, as fellow code-owner

@CoMPaTech
Copy link
Copy Markdown
Member Author

If someone is open for discussion on xml versus pickle, please ping me on discord (or mention me in the appropriate dev-channel).

@CoMPaTech
Copy link
Copy Markdown
Member Author

Updated pickles according to newer release of the module (on PyPi). The improved testing of config_flow (which were oversighted in #37289 and are PRed for fix in #37229) still need to be rebased.

@CoMPaTech
Copy link
Copy Markdown
Member Author

@MartinHjelmare just re-checked myself (using the famous websearch) and looks like https://pypi.org/project/jsonpickle/ does provide a solution for storing sets and 'it works on my laptop'. I'm not sure how to get (if allowed anyway) the jsonpickle module in onto testing (reading https://developers.home-assistant.io/docs/development_testing points towards a list in the script, but I'm not clear which list/dict is for testing only).

@CoMPaTech
Copy link
Copy Markdown
Member Author

(jsonpickle as a module worked out of the box?)

@MartinHjelmare
Copy link
Copy Markdown
Member

Add it to the integration manifest requirements.

Comment thread tests/components/plugwise/conftest.py Outdated
@CoMPaTech
Copy link
Copy Markdown
Member Author

CoMPaTech commented Sep 7, 2020

Add it to the integration manifest requirements.

Too bad we cant select for testing purposes only, not in the least since we have to pin requirements :(

@MartinHjelmare
Copy link
Copy Markdown
Member

We don't have any infrastructure for integration specific test dependencies. Then we would need to add it to our common test requirements. I can check if we want that.

@MartinHjelmare
Copy link
Copy Markdown
Member

We have a go ahead to add jsonpickle to our common test requirements. Please add it to requirements_test.txt, sorted 🔡.

Copy link
Copy Markdown
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.

Great!

@CoMPaTech
Copy link
Copy Markdown
Member Author

Having these in place will ensure the code is better and other people will be able to modify/extend with more ease and security :)

@MartinHjelmare MartinHjelmare merged commit bb9ea7c into home-assistant:dev Sep 8, 2020
@balloob balloob added this to the 0.115.0 milestone Sep 13, 2020
balloob pushed a commit that referenced this pull request Sep 13, 2020
@CoMPaTech CoMPaTech deleted the plugwise-testing branch October 8, 2020 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants