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

Ptvsd debugger component. #23336

Merged
merged 12 commits into from Apr 30, 2019

Conversation

@Swamp-Ig
Copy link
Contributor

commented Apr 24, 2019

Description:

I've been using this custom component for a while to allow the ptsvd debugger to attach to my production hass system.

Time to submit it!

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

Example entry for configuration.yaml (if applicable):

ptvsd:

Options:

  • host - hostname to use for listening, defaults to 0.0.0.0 (all hosts)
  • port - port to listen on, defaults to 5678
  • wait - if true, wait for the debugger to connect before going further. The debugger starts up very early in the process, basically before anything else happens, so you should be able to put in breakpoints almost anywhere. Defaults to false.

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.

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

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

  • The manifest file has all fields filled out correctly (example).
  • New dependencies have been added to requirements in the manifest (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.

@Swamp-Ig Swamp-Ig requested a review from home-assistant/core as a code owner Apr 24, 2019

@Swamp-Ig Swamp-Ig referenced this pull request Apr 24, 2019
2 of 2 tasks complete
@Swamp-Ig

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

Can we just exclude it from code coverage? I have tested it by hand, test cases will be annoying to develop because they'll all need to be mocked, and won't really prove anything much.

Edit: actually will do a test case, mostly for the bootstrap code.

homeassistant/bootstrap.py Outdated Show resolved Hide resolved
homeassistant/bootstrap.py Outdated Show resolved Hide resolved

@Swamp-Ig Swamp-Ig force-pushed the Swamp-Ig:ptvsd-debug branch 2 times, most recently from 5f8eeaa to 3832722 Apr 24, 2019

@Swamp-Ig

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

OK submitted some test cases, that should get coverage now.

@Swamp-Ig Swamp-Ig force-pushed the Swamp-Ig:ptvsd-debug branch from 3832722 to 3b71240 Apr 24, 2019

@robbiet480

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

Seems mobile_app tests are consistently broken on this PR. Any ideas why, because I'm stumped.

@Swamp-Ig

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

@robbiet480 missing a async_block_till_done by the looks. IDK why it's suddenly popped up here, but eh.

@Swamp-Ig Swamp-Ig referenced this pull request Apr 24, 2019
@Swamp-Ig

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

The tests were failing because of an external issue, just a missing block_till_done in another test. I've pre-merged #23343 just to get it in because I'm out until tomorrow and might not have any time until next Tuesday or so. If this gets merged #23343 can get closed.

Swamp-Ig added some commits Apr 23, 2019

@Swamp-Ig Swamp-Ig force-pushed the Swamp-Ig:ptvsd-debug branch from 05baac0 to 639c1d3 Apr 24, 2019

@Swamp-Ig

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

Rebased against #23343.

Swamp-Ig added some commits Apr 25, 2019

@Swamp-Ig

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

OK I give up on getting coverage to work. You can't mock it properly, have just added to .coveragerc.

Anyhow, should be all good now 😁

Swamp-Ig added some commits Apr 25, 2019

@Swamp-Ig

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

Have figured out why coverage can't work: as soon as the ptvsd library is loaded, the coverage debugger is cut off. There's no real way to test coverage of the component without this happening because it happens as soon as ptvsd is imported.

The supplied test does cover the changes to bootstrap anyhow, so that's basically as good as it gets.

loader.async_component_dependencies(hass, domain)
for domain in debuggers
])
_LOGGER.debug("Starting up debuggers %s", debuggers)

This comment has been minimized.

Copy link
@balloob

balloob Apr 30, 2019

Member

Let's do this in info, in case one of the debuggers block?

@balloob
Copy link
Member

left a comment

Nice! Ok to merge when level of log message changed from debug to info.

@cgtobi cgtobi merged commit b0843f4 into home-assistant:dev Apr 30, 2019

12 of 13 checks passed

codecov/patch 85.71% of diff hit (target 94.31%)
Details
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/project 94.31% (target 90%)
Details

@Swamp-Ig Swamp-Ig deleted the Swamp-Ig:ptvsd-debug branch Apr 30, 2019

@balloob balloob referenced this pull request May 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.