Adjust async_setup_entry in config_flow scaffold#88319
Conversation
emontnemery
left a comment
There was a problem hiding this comment.
Nice!
Thanks, @epenet 👍
| @pytest.fixture(autouse=True, name="mock_setup_entry") | ||
| def override_async_setup_entry() -> Generator[AsyncMock, None, None]: |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I have opened #88481 to rename the function.
There was a problem hiding this comment.
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 toconftest, and then add a specificpytestmark = pytest.mark.usefixtures("mock_setup_entry")at the top of the test file - we have to add a new file to the scaffold
There was a problem hiding this comment.
I dunno, this will now result in a review comment 9 out of 10 times asking to move it, which is also not great 🤷
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Proposed change
Linked to #88315
Type of change
Additional information
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.To help with the load of incoming pull requests: