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

Fix racy homekit_controller platform setup caused by #22368 #22655

Merged
merged 1 commit into from Apr 2, 2019

Conversation

Projects
None yet
4 participants
@Jc2k
Copy link
Contributor

commented Apr 2, 2019

Description:

#22194 introduced some new homekit_controller tests and @balloob and @awarecan pointed out at least one of them was flaky. This doesn't happen in my config entry branch and i've now traced its root cause to #22368.

Essentially, HKDevice.__init__ on dev has for some time triggered the setup_platform for its sub-domains like light/climate/etc. But there is a race where this code hasn't run yet:

hass.data[KNOWN_DEVICES][hkid] = device

If the setup_platform runs before that happens you get the error in the test. This fix moves where we store the current HKDevice into hass.data so that it is always set before any further setup_platform is called.

This race is probably a real bug, not just a flaky test. So if #22368 is already in a tag or in an rc branch it might be worth applying the fix there too.

This is something of a temporary band aid - the buggy code is actually removed in my config entry branch and doesn't have this problem. The config entry branch won't trigger side effects in HKDevice.__init__ any more. So the config entry work will be the 'real' fix, but this will do until then.

**Related issue (if applicable): #22650

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.
@codecov

This comment has been minimized.

Copy link

commented Apr 2, 2019

Codecov Report

Merging #22655 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev   #22655   +/-   ##
=======================================
  Coverage   92.57%   92.57%           
=======================================
  Files         457      457           
  Lines       36977    36977           
=======================================
  Hits        34232    34232           
  Misses       2745     2745

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 48189dd...11c00e9. Read the comment docs.

@MartinHjelmare MartinHjelmare added this to the 0.91.0 milestone Apr 2, 2019

@MartinHjelmare MartinHjelmare merged commit 16e0953 into home-assistant:dev Apr 2, 2019

14 checks passed

Hound No violations found. Woof!
build Workflow: build
Details
ci/circleci: pre-install-all-requirements Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.5.5 Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.6 Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.7 Your tests passed on CircleCI!
Details
ci/circleci: pylint Your tests passed on CircleCI!
Details
ci/circleci: static-check Your tests passed on CircleCI!
Details
ci/circleci: test 3.5.5 Your tests passed on CircleCI!
Details
ci/circleci: test 3.6 Your tests passed on CircleCI!
Details
ci/circleci: test 3.7 Your tests passed on CircleCI!
Details
cla-bot Everyone involved has signed the CLA
codecov/patch Coverage not affected when comparing 48189dd...11c00e9
Details
codecov/project 92.57% remains the same compared to 48189dd
Details

@ghost ghost removed the in progress label Apr 2, 2019

balloob added a commit that referenced this pull request Apr 2, 2019

mxworm added a commit to mxworm/home-assistant that referenced this pull request Apr 2, 2019

Merge branch 'dev' into current
* dev: (203 commits)
  Add color support to emulated hue (home-assistant#19590)
  Add missing properties and scenes support to Osram Lightify (home-assistant#22597)
  Axis discovery updates host address (home-assistant#22632)
  Add battery sensor to Homematic IP (home-assistant#22630)
  Return 0 for failed Foscam streams (home-assistant#22651)
  Ignore code coverages for component without test (home-assistant#22653)
  Admin service to automatically add empty schema (home-assistant#22637)
  Fix pytest durations parameter (home-assistant#22658)
  Don't force updates on ZHA Electrical Measurement sensor. (home-assistant#22647)
  Improve evohome exception handling and fix bugs (home-assistant#22140)
  Fix racy homekit_controller platform setup caused by home-assistant#22368 (home-assistant#22655)
  Run PyLint under Python 3.5 (home-assistant#22642)
  Qwikswitch fix listen loop (home-assistant#22600)
  Add codecov (home-assistant#22649)
  Add trusted networks deprecating warning (home-assistant#22487)
  Require static-check success first for rest of workflow (home-assistant#22635)
  Support GET params for websocket ingress path (home-assistant#22638)
  change library to georss_generic_client (home-assistant#22615)
  Amcrest: Add on/off support & attributes. Bump amcrest to 1.3.0 (home-assistant#22418)
  Add support for Dyson Purecool 2018 Air Purifiers models TP04 and DP04 (home-assistant#22215)
  ...

@balloob balloob referenced this pull request Apr 3, 2019

Merged

0.91.0 #22688

@Jc2k Jc2k deleted the Jc2k:homekit_fix_flaky_test branch Apr 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.