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

Split out geofency with a component and platform #17933

Merged
merged 7 commits into from Nov 6, 2018

Conversation

@rohankapoorcom
Contributor

rohankapoorcom commented Oct 29, 2018

Description:

Splitting out the geofency device tracker platform to a separate component/platform. This is a prerequisite for #17925.

I don't have an iPhone (so don't have geofency itself to test with), but I'm using postman to simulate the api calls.

Breaking Change: The geofency device_tracker platform no longer takes any configuration. The configuration needs to be applied to the component geofency instead. The platform will be automatically loaded with the component.

This is my first time splitting a component out of a platform, so I'd appreciate a review before I document it.

Unit tests are currently broken (and I have no idea why). I'm assuming it has something to do with the async state change happening after the unit tests are checking the state machine, but I would appreciate some advice there. Disregard, I totally forgot to block until the async call finished....

Related issue (if applicable): Relates to #17925

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#7218

Example entry for configuration.yaml (if applicable):

geofency:
    mobile_beacons:
      - car
      - keys

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

If user exposed functionality or configuration variables are added/changed:

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

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
@houndci-bot

Some files could not be reviewed due to errors:

Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/linters/.local/bin/flake8", line 7, in 
    from flake8.main.cli import main
ModuleNotFoundError: No module named 'flake8'
@houndci-bot

Some files could not be reviewed due to errors:

Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/linters/.local/bin/flake8", line 7, in 
    from flake8.main.cli import main
ModuleNotFoundError: No module named 'flake8'
@houndci-bot

Some files could not be reviewed due to errors:

Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/linters/.local/bin/flake8", line 7, in 
    from flake8.main.cli import main
ModuleNotFoundError: No module named 'flake8'
@houndci-bot

Some files could not be reviewed due to errors:

Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/linters/.local/bin/flake8", line 7, in 
    from flake8.main.cli import main
ModuleNotFoundError: No module named 'flake8'
@rohankapoorcom

This comment has been minimized.

Contributor

rohankapoorcom commented Oct 30, 2018

Hmm, seems I've run afoul of the changes made in #17952. I'll rebase and fix things up.

@rohankapoorcom rohankapoorcom force-pushed the rohankapoorcom:split-out-geofency branch from 04c395b to 1015b79 Oct 30, 2018

@rohankapoorcom

This comment has been minimized.

Contributor

rohankapoorcom commented Oct 30, 2018

I have no idea why that unit test is failing. I don't think I touched anything that had anything to do with it...

@balloob

One final comment

@rohankapoorcom

This comment has been minimized.

Contributor

rohankapoorcom commented Oct 30, 2018

@balloob @MartinHjelmare after the latest set of changes, it seems like I broke something. I'm not getting any updates on the device tracker and seeing the following error in the logs:

/usr/lib/python3.6/asyncio/events.py:145: RuntimeWarning: coroutine '_set_location' was never awaited

Any idea what I did wrong? As far as I can tell, this was caused by making the callback on line 21 of device_tracker/geofency.py a coroutine.

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Oct 30, 2018

I'm guessing we don't await hass.async_block_till_done somewhere after awaiting the client post.

@rohankapoorcom

This comment has been minimized.

Contributor

rohankapoorcom commented Oct 31, 2018

Only way I was able to resolve it was to make this not be a coroutine:

    @callback
    def _set_location(device, gps, location_name, attributes):
        """Fire HA event to set location."""
        hass.async_create_task(
            async_see(
                dev_id=device,
                gps=gps,
                location_name=location_name,
                attributes=attributes
            )
        )
@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Oct 31, 2018

You forgot to remove @callback when you made _set_location a coroutine:
0f33cc5#diff-739acf35f7305d6c70ee60de04c31ac4

@rohankapoorcom rohankapoorcom force-pushed the rohankapoorcom:split-out-geofency branch from 29da293 to 0b93fa7 Oct 31, 2018

@rohankapoorcom

This comment has been minimized.

Contributor

rohankapoorcom commented Oct 31, 2018

Nice catch, thanks! Changes pushed up.

@rohankapoorcom rohankapoorcom force-pushed the rohankapoorcom:split-out-geofency branch from 0b93fa7 to fb9648c Oct 31, 2018

@rohankapoorcom

This comment has been minimized.

Contributor

rohankapoorcom commented Oct 31, 2018

Apparently it still doesn't work! The strange part is if I run the geofency tests alone via pytest they pass locally...

@balloob

This comment has been minimized.

Member

balloob commented Nov 1, 2018

Somehow in your tests async_load_config returns None, it should always return a list

@balloob

This comment has been minimized.

Member

balloob commented Nov 6, 2018

Able to repro this with py.test tests/components/device_tracker tests/components/geofency

Some test is not cleaning up !

@balloob

This comment has been minimized.

Member

balloob commented Nov 6, 2018

fixed tests

@balloob balloob merged commit bdba385 into home-assistant:dev Nov 6, 2018

4 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wafflebot wafflebot bot removed the in progress label Nov 6, 2018

@rohankapoorcom rohankapoorcom deleted the rohankapoorcom:split-out-geofency branch Nov 7, 2018

@rohankapoorcom

This comment has been minimized.

Contributor

rohankapoorcom commented Nov 7, 2018

Thanks for the assistance!

zxdavb added a commit to zxdavb/home-assistant that referenced this pull request Nov 13, 2018

Split out geofency with a component and platform (home-assistant#17933)
* Split out geofency with a component and platform

* Make geofency component/device_tracker more async

* Move geofency tests to new package

* Remove coroutine in geofency callback

* Lint

* Fix coroutine in geofency callback

* Fix incorrect patch

@balloob balloob referenced this pull request Nov 29, 2018

Merged

0.83 #18776

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment