Skip to content

Adjust async_setup_entry in config_flow scaffold #88319

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.

Sign up for GitHub

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

Merged
emontnemery merged 1 commit into from
Feb 17, 2023
Merged

Adjust async_setup_entry in config_flow scaffold #88319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 15 additions & 6 deletions script/scaffold/templates/config_flow/tests/test_config_flow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
"""Test the NEW_NAME config flow."""
from unittest.mock import patch
from collections.abc import Generator
from unittest.mock import AsyncMock, patch

import pytest

from homeassistant import config_entries
from homeassistant.components.NEW_DOMAIN.config_flow import CannotConnect, InvalidAuth
Expand All @@ -8,7 +11,16 @@
from homeassistant.data_entry_flow import FlowResultType


async def test_form(hass: HomeAssistant) -> None:
@pytest.fixture(autouse=True, name="mock_setup_entry")
def override_async_setup_entry() -> Generator[AsyncMock, None, None]:
Comment on lines +14 to +15
Copy link
Copy Markdown
Member

@frenck frenck Feb 17, 2023

Choose a reason for hiding this comment

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

Not sure why the name of the fixture is set, we also don't use the override_* naming generally... so that looks odd to me.

Why not just name the function mock_setup_entry ?

Additionally, I think it is better practice (and better readable) if these things are in conftest.py and not in top of this file (it distracts from the actual tests in this module).

../Frenck

Copy link
Copy Markdown
Contributor Author

@epenet epenet Feb 20, 2023

Choose a reason for hiding this comment

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

I have opened #88481 to rename the function.

Copy link
Copy Markdown
Contributor Author

@epenet epenet Feb 20, 2023

Choose a reason for hiding this comment

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

Regarding conftest.py, I think it is clearer in this case to have it defined at the top of test_config_flow module.

  • for new contributors, it makes it an easier introduction to fixtures
  • it usually only applies to config flow tests
  • we can't use autouse=True: we have to add the fixture to conftest, and then add a specific pytestmark = pytest.mark.usefixtures("mock_setup_entry") at the top of the test file
  • we have to add a new file to the scaffold

Copy link
Copy Markdown
Member

@frenck frenck Feb 20, 2023

Choose a reason for hiding this comment

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

I dunno, this will now result in a review comment 9 out of 10 times asking to move it, which is also not great 🤷

Copy link
Copy Markdown
Contributor Author

@epenet epenet Feb 20, 2023

Choose a reason for hiding this comment

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

I have opened an alternative PR #88484

Copy link
Copy Markdown
Contributor

@elupus elupus Feb 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

For reference, name is set so function def can differ from fixture name. If fixture name is in same as function name, you get a linting warning on the test cases that a testcase parameter hides a function.

Same issue will occur if a fixture uses another fixture inside conftest.

Ps. Since that lint warning can easily be ignore and is not enforced anywhere, im perfectly fine with either changes for my modules.

Copy link
Copy Markdown
Member

@frenck frenck Feb 20, 2023

Choose a reason for hiding this comment

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

For reference, name is set so function def can differ from fixture name.

I'm aware, although, it isn't an issue and we have explicitly disabled those linting checks. Nevertheless, in general we use the _fixture postfix to resolve such cases.

"""Override async_setup_entry."""
with patch(
"homeassistant.components.NEW_DOMAIN.async_setup_entry", return_value=True
) as mock_setup_entry:
yield mock_setup_entry


async def test_form(hass: HomeAssistant, mock_setup_entry: AsyncMock) -> None:
"""Test we get the form."""
result = await hass.config_entries.flow.async_init(
DOMAIN, context={"source": config_entries.SOURCE_USER}
Expand All @@ -19,10 +31,7 @@ async def test_form(hass: HomeAssistant) -> None:
with patch(
"homeassistant.components.NEW_DOMAIN.config_flow.PlaceholderHub.authenticate",
return_value=True,
), patch(
"homeassistant.components.NEW_DOMAIN.async_setup_entry",
return_value=True,
) as mock_setup_entry:
):
result2 = await hass.config_entries.flow.async_configure(
result["flow_id"],
{
Expand Down
30 changes: 19 additions & 11 deletions script/scaffold/templates/config_flow_helper/tests/test_config_flow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"""Test the NEW_NAME config flow."""
from unittest.mock import patch
from collections.abc import Generator
from unittest.mock import AsyncMock, patch

import pytest

Expand All @@ -11,8 +12,19 @@
from tests.common import MockConfigEntry


@pytest.fixture(autouse=True, name="mock_setup_entry")
def override_async_setup_entry() -> Generator[AsyncMock, None, None]:
"""Override async_setup_entry."""
with patch(
"homeassistant.components.NEW_DOMAIN.async_setup_entry", return_value=True
) as mock_setup_entry:
yield mock_setup_entry


@pytest.mark.parametrize("platform", ("sensor",))
async def test_config_flow(hass: HomeAssistant, platform) -> None:
async def test_config_flow(
hass: HomeAssistant, mock_setup_entry: AsyncMock, platform
) -> None:
"""Test the config flow."""
input_sensor_entity_id = "sensor.input"

Expand All @@ -22,15 +34,11 @@ async def test_config_flow(hass: HomeAssistant, platform) -> None:
assert result["type"] == FlowResultType.FORM
assert result["errors"] is None

with patch(
"homeassistant.components.NEW_DOMAIN.async_setup_entry",
return_value=True,
) as mock_setup_entry:
result = await hass.config_entries.flow.async_configure(
result["flow_id"],
{"name": "My NEW_DOMAIN", "entity_id": input_sensor_entity_id},
)
await hass.async_block_till_done()
result = await hass.config_entries.flow.async_configure(
result["flow_id"],
{"name": "My NEW_DOMAIN", "entity_id": input_sensor_entity_id},
)
await hass.async_block_till_done()

assert result["type"] == FlowResultType.CREATE_ENTRY
assert result["title"] == "My NEW_DOMAIN"
Expand Down