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 more HomeKit device enumeration tests #22194

Merged
merged 3 commits into from
Mar 30, 2019

Conversation

Jc2k
Copy link
Member

@Jc2k Jc2k commented Mar 19, 2019

Description:

Another test only branch for homekit_controller - hopefully the last before the remaining config entry work.

These are all the remaining tests from my homekit_controller config entry branch tests that can be merged ahead of the remaining config entry code. Like #22141 these are tests that ensure devices we know should more or less work now continue to more or less work - espsecially that supported_features continue to be detected correctly and that all the supported accessories on multi-device hubs are detected.

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.

@Jc2k Jc2k force-pushed the homekit_device_specific_tests branch from 497bfeb to cd235ff Compare March 20, 2019 15:05
@Jc2k
Copy link
Member Author

Jc2k commented Mar 20, 2019

(Rebased to get fix from #22207)

@Jc2k
Copy link
Member Author

Jc2k commented Mar 23, 2019

Sorry to bump but if someone could look at this it would be great, and would unblock me to work on some more homekit_controller PR's from my configentry branch. It doesn't change any HA code, just add homekit_controller tests in the same style as a PR that already landed (#22141).

Jc2k added a commit to Jc2k/home-assistant that referenced this pull request Mar 24, 2019
Test that Aqara Gateway, Ecobee 3 and Lennox E30 is correctly enumerated
Jc2k added a commit to Jc2k/home-assistant that referenced this pull request Mar 24, 2019
Test that Aqara Gateway, Ecobee 3 and Lennox E30 is correctly enumerated
Jc2k added a commit to Jc2k/home-assistant that referenced this pull request Mar 25, 2019
Test that Aqara Gateway, Ecobee 3 and Lennox E30 is correctly enumerated
Jc2k added a commit to Jc2k/home-assistant that referenced this pull request Mar 25, 2019
Test that Aqara Gateway, Ecobee 3 and Lennox E30 is correctly enumerated
Jc2k added a commit to Jc2k/home-assistant that referenced this pull request Mar 25, 2019
Test that Aqara Gateway, Ecobee 3 and Lennox E30 is correctly enumerated
@Jc2k Jc2k mentioned this pull request Mar 26, 2019
3 tasks
Jc2k added a commit to Jc2k/home-assistant that referenced this pull request Mar 26, 2019
Test that Aqara Gateway, Ecobee 3 and Lennox E30 is correctly enumerated
@@ -0,0 +1,488 @@
[
Copy link
Member

Choose a reason for hiding this comment

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

I think we want the fixtures under tests/fixtures.

profile_path = os.path.join(
os.path.dirname(__file__), 'aqara_gateway.json'
)
accessories = setup_accessories_from_file(profile_path)
Copy link
Member

Choose a reason for hiding this comment

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

This does I/O and should be put on the executor. We shouldn't interfere with the event loop even if it's a test, I think.

await hass.async_add_executor_job(...)

@Jc2k
Copy link
Member Author

Jc2k commented Mar 28, 2019

Both requested changes have now been made.

I didn't push a rebase because of the breakage on dev, but i did test it still works on latest dev and it does.

Jc2k added a commit to Jc2k/home-assistant that referenced this pull request Mar 28, 2019
Test that Aqara Gateway, Ecobee 3 and Lennox E30 is correctly enumerated
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.

Good!

@Jc2k
Copy link
Member Author

Jc2k commented Mar 30, 2019

Not really sure what is going on here. This change only contains tests, so shouldn't be pulling coverage down., especially on files i haven't gone near! Best guess is that because its an old PR (11 days old) the coverage has gone up on dev in the mean time, so coveralls thinks it will reduce the coverage? When obviously it won't.

I would rebase to fix it but it looks like there are CI failures on dev still. So i'm not sure what the next steps are for this PR?

@MartinHjelmare MartinHjelmare merged commit 906f011 into home-assistant:dev Mar 30, 2019
@ghost ghost removed the in progress label Mar 30, 2019
@MartinHjelmare
Copy link
Member

Merged.

@Jc2k
Copy link
Member Author

Jc2k commented Mar 30, 2019

Ahh brilliant, tyvm!

@Jc2k Jc2k deleted the homekit_device_specific_tests branch March 30, 2019 16:42
unibeck pushed a commit to unibeck/home-assistant that referenced this pull request Apr 7, 2019
* Test that Aqara Gateway, Ecobee 3 and Lennox E30 is correctly enumerated

* Move json to fixtures directory

* Move IO to executor
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.

3 participants