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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Block tests from opening sockets #55516

Merged
merged 10 commits into from Oct 6, 2021
Merged

Block tests from opening sockets #55516

merged 10 commits into from Oct 6, 2021

Conversation

emontnemery
Copy link
Contributor

@emontnemery emontnemery commented Sep 1, 2021

Proposed change

Block tests from opening sockets

The blocking is implemented by using the pytest_socket package which is enabled in a pytest_runtest_setup in the top-level conftest.py.
Workarounds for two pytest_socket PRs, miketheman/pytest-socket#75, miketheman/pytest-socket#76 are added in the top-level conftest.py.

Related PRs

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

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
  • 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:

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

  • The manifest file 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.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 馃 Silver
  • 馃 Gold
  • 馃弳 Platinum

To help with the load of incoming pull requests:

@probot-home-assistant probot-home-assistant bot added the small-pr PRs with less than 30 lines. label Sep 1, 2021
@emontnemery emontnemery marked this pull request as draft September 1, 2021 08:52
Comment on lines 701 to 702
--disable-socket \
--allow-unix-socket \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not so nice.
There are other integration alternative: https://github.com/miketheman/pytest-socket#usage, the best option might be to do this in the top level conftest.py:

import pytest_socket

def pytest_runtest_setup():
    pytest_socket.disable_socket(allow_unix_socket=True)

Copy link
Member

Choose a reason for hiding this comment

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

Adding the limitation to the top-level conftest.py is clearly better.

But why do we allow unix_sockets, I thought the tests should operate with mock and sort of "offline".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unix sockets are used by asyncio, so we have to allow it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The limitation is now added to the top-level conftest.py instead

@emontnemery
Copy link
Contributor Author

A summary of tests failing in group 1:
almond: hits external API
api: connects to HA API@127.0.0.1 - allow, or update test to use the common API mock
august: connects to HA API@127.0.0.1 - allow, or update test to use the common API mock
auth: connects to HA API@127.0.0.1 - allow, or update test to use the common API mock
bosch_sch: sets up live zeroconf
broadlink: opens a socket, should probably be mocked out in test
devolo_home_control: sets up live zeroconf
dialogflow: connects to HA API@127.0.0.1 - allow, or update test to use the common API mock
emulated_hue: connects to HA API@127.0.0.1 - allow, or update test to use the common API mock
emulated_roku: uses homeassistant.components.network.util.async_get_source_ip, which opens a socket to find the local IP
epson: opens a socket, should probably be mocked out in test
esphome: sets up live zeroconf
freedompro: opens a socket, should probably be mocked out in test
fritz: uses homeassistant.components.network.util.async_get_source_ip, which opens a socket to find the local IP
frontend: connects to HA API@127.0.0.1 - allow, the tests can't use the common API mock
generic: broken tests, fixed in #55521

Common issues are:

  • Components relying on zeroconf, we should probably use a mocked out zeroconf in most cases
  • Components using homeassistant.components.network.util.async_get_source_ip, we may want to mock it out and just return a predetermined IP.
  • Tests connecting to the HA API, but not using the hass_client or hass_ws_client fixtures

@emontnemery
Copy link
Contributor Author

This fixes the zeroconf mock and enables it for group 1: #55526

@emontnemery emontnemery force-pushed the pytest-disable-sockets branch 4 times, most recently from aca9afb to 845e917 Compare September 19, 2021 08:20
@MartinHjelmare MartinHjelmare added this to Incoming in Dev Sep 27, 2021
bdraco added a commit to bdraco/home-assistant that referenced this pull request Oct 4, 2021
bdraco added a commit to bdraco/home-assistant that referenced this pull request Oct 4, 2021
MartinHjelmare pushed a commit that referenced this pull request Oct 4, 2021
MartinHjelmare pushed a commit that referenced this pull request Oct 4, 2021
@emontnemery emontnemery marked this pull request as ready for review October 5, 2021 09:11
Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

IMHO: Awesomeness!

Dev automation moved this from Incoming to Reviewer approved Oct 5, 2021
Copy link
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

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

Just awesome 馃憤

@bdraco
Copy link
Member

bdraco commented Oct 6, 2021

Let's merge this so we don't have to keep playing whack-a-mole.

@bdraco bdraco merged commit f6682ba into dev Oct 6, 2021
Dev automation moved this from Reviewer approved to Done Oct 6, 2021
@bdraco bdraco deleted the pytest-disable-sockets branch October 6, 2021 00:46
@github-actions github-actions bot locked and limited conversation to collaborators Oct 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed small-pr PRs with less than 30 lines.
Projects
Dev
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants