Closed sodul closed 4 years ago
Maybe a mypy plugin could help here somehow?
(What already works of course is using the returned type of the fixture directly - but it does not make it obvious that it is a fixture then, of course.)
/cc @bluetech
I haven't actually tested it, but you should be able to do something like:
T = typing.TypeVar('T')
class Fixture(typing.Generic[T]):
pass
def test_bar(bob: Fixture[str]):
...
Why not just write it as def test_bar(bob: str): ...
at that point? IMO the Fixture
just makes it more confusing.
I just finally have to write pytest support in Jedi. I'm using both tools daily so I should really just do myself a favor :).
def test_bar(bob: str):
is what we are doing today but while pytest is common, its use of magical fixtures is confusing for the majority of python developers that do not know about them.
I was refactoring some code to comply with PEP-8 recently so that pylint and other tools can better expose potential bugs but I had to double and triple check which arguments were safe to rename and which were not. The Fixture annotation would also allow us to better target which arguments need to be updated when renaming a fixture. For example to rename the bob
fixture we could grep for bob: Fixture
and have a much higher confidence that all the code that needed updating got updated.
Florian's solution passes mypy but then it does not allow PyCharm to detect that bob
is a string and we have no type hinting, similar result as the Fixture = Union
approach.
This would have to be a mypy
plugin... and even then, it's going to be full of edge cases. IMO if you need that level of visibility fixtures are just not suitable.
I second @davidhalter's message -- I don't think a special syntax improves the situation (and would require a mypy plugin to undo the outer Fixture[...]
indirection to "unwrap" the inner type)
Support for fixtures just landed in Jedi's master branch so it is only a matter of time until Jedi is released and you all get to have IDE support for fixtures. Annotations are not necessary (but might help in very complicated cases).
Thanks everyone for chiming in here, I think it was an useful discussion, but there's nothing really to be done on pytest AFAIU.
I know this is closed, but this is important for posterity.
pytest
itself has type annotations for fixtures that you can reuse (see pytest-dev/pytest/src/_pytest/fixtures.py). You can import these from _pytest.fixtures
even though the leading underscore indicates the package is intended to be private.
Namely:
# The value of the fixture -- return/yield of the fixture function (type variable).
_FixtureValue = TypeVar("_FixtureValue")
# The type of the fixture function (type variable).
_FixtureFunction = TypeVar("_FixtureFunction", bound=Callable[..., object])
# The type of a fixture function (type alias generic in fixture value).
_FixtureFunc = Union[
Callable[..., _FixtureValue], Callable[..., Generator[_FixtureValue, None, None]]
]
...
class FixtureRequest:
"""A request for a fixture from a test or fixture function.
A request object gives access to the requesting test context and has
an optional ``param`` attribute in case the fixture is parametrized
indirectly.
"""
...
...
@final
@attr.s(frozen=True)
class FixtureFunctionMarker:
...
The signature of fixture
itself is:
def fixture(
fixture_function: Optional[_FixtureFunction] = None,
..., # Other irrelevant args excluded by me for brevity
) -> Union[FixtureFunctionMarker, _FixtureFunction]:
fixture_marker = FixtureFunctionMarker(
scope=scope,
params=params,
autouse=autouse,
ids=ids,
name=name,
)
# Direct decoration.
if fixture_function:
return fixture_marker(fixture_function)
return fixture_marker
Since pytest
defines these directly, you don't have to guess at what they should be. For instance, use type SubRequest
for request
objects declared as function arguments. Note also that when a test function uses a fixture as an argument, the fixture instance returned from the fixture function will be injected, so the type should be the type of the fixture instance.
If that's what they use, I would take it from the horse's mouth.
An example conftest.py
for sphinx
testing is:
# region docstring
"""Configuration file for ``pytest`` tests
This module contains integration test fixtures. This code was adapted from Paul Everitt's
GitHub repo (see [1]_).
References:
.. _[1]: https://github.com/pauleveritt/customizing_sphinx/blob/master/tests/integration/conftest.py
"""
# endregion
from typing import Any, Callable, Generator, List, Sequence, Union
import pytest
from _pytest.fixtures import SubRequest
from bs4 import BeautifulSoup
from sphinx.testing.path import path
from sphinx.testing.util import SphinxTestApp
pytest_plugins: Union[str, Sequence[str]] = [
"sphinx.testing.fixtures",
]
"""A ``pytest`` global variable that registers plugins for use in testing."""
# Exclude 'roots' dirs for pytest test collector
collect_ignore: List[str] = ["roots", "__init__.py"]
"""A ``pytest`` global variable that excludes directories or modules from being collected by the ``pytest`` runner"""
@pytest.fixture()
def rootdir() -> Generator[str, None, None]:
# region docstring
"""Root directory for sphinx tests.
Yields:
Generator[str, None, None]: Generator that yields test root directories
"""
# endregion
yield path(".\\docs\\source").abspath()
@pytest.fixture()
def content(
make_app: Callable[..., Any], rootdir: str
) -> Generator[SphinxTestApp, None, None]:
# region docstring
"""Content of generated html pages.
Args:
make_app (Callable[..., Any]): Function that creates a sphinx app for testing purposes
rootdir (str): Test root directory
Yields:
Generator[SphinxTestApp, None, None]: Generator that yields sphinx apps for testing purposes
"""
# endregion
app = make_app("html")
app.build()
yield app
@pytest.fixture()
def page(
content: SphinxTestApp, request: SubRequest
) -> Generator[BeautifulSoup, None, None]:
# region docstring
"""Generated HTML page of docs
Args:
content (SphinxTestApp): A sphinx app for testing purposes
request (SubRequest): A sub request for handling getting a fixture from a test function/fixture
Yields:
Generator[BeautifulSoup, None, None]: Generator of beautiful soup web scraper instances using the html5lib parser
"""
# endregion
page_name = request.param
page_content = (content.outdir / page_name).read_text()
yield BeautifulSoup(page_content, "html5lib")
@nicoddemus Thanks everyone for chiming in here, I think it was an useful discussion, but there's nothing really to be done on pytest AFAIU.
I think this decision should be revisited now that Python 3.9 has PEP 593, flexible function and variable annotations.
Solving this no longer requires a MyPy plugin or Union
abuse.
Quick sketch:
import typing
import pytest
@pytest.fixture
def my_fixture() -> str:
return "abc123"
def test_my_test_function(my_fixture: typing.Annotated[str, pytest.fixture]):
assert my_fixture == "abc123"
For MyPy and other tools that are unaware of PyTest, my_fixture: typing.Annotated[str, pytest.fixture]
is read exactly like my_fixture: str
.
But for humans reading the code, the annotation's mention of pytest.fixture
signals that this is a PyTest thing.
Seems like the best of both worlds.
We could even move towards something even more explicit:
import typing
import pytest
@pytest.fixture
def my_fixture() -> str:
return "abc123"
def test_my_test_function(my_fixture_provided_str: typing.Annotated[str, pytest.from_fixture(my_fixture)]):
assert my_fixture_provided_str == "abc123"
My team would find this explicitness helpful. When a given test has fixtures defined internally, and fixtures defined externally in a conftest.py
, it sometimes gets confusing keeping track of what everything is and where it comes from.
Edit: I haven't had a chance to read through it yet, but the discussion in #3834 also looks relevant.
you're free to do whatever you want with Annotated
of course, but I don't think we should recommend this as it's redundant (of course it's a fixture where else could it come from?) and noisy (a bunch of keyboard-typing for metadata nobody needs or reads). IDEs are already fairly good at identifying pytest fixtures without it and anyone familiar with pytest would recognize such arguments as necessarily coming from fixtures
IDEs are already fairly good at identifying pytest fixtures without it and anyone familiar with pytest would recognize such arguments as necessarily coming from fixtures
That's true. This would be redundant for those people.
I'm instead concerned instead about people who don't benefit from those IDE features, or who aren't familiar with Pytest. The current syntax is a barrier to entry for those people, and I think it's solvable.
(of course it's a fixture where else could it come from?)
If you're unfamiliar with Pytest, and you're just reading a test file ("oh, test_foo.py
! This looks relevant! What's in here?"), you really have no clue. The current syntax doesn't give you an obvious thing to Google.
You would need to either:
To clarify, is it your position that this isn't a problem, or that it is a problem but none of the syntaxes proposed so far are a good enough solution for it?
I mean, if you see
def test_thing(some_thing):
...
you can very easily git grep some_thing
and be at your answer
Just to share my opinion:
I agree with @asottile that pytest shouldn't recommend it: it is metadata no tooling is using, just a bunch of extra typing. Of course people are free to adopt it as a standard in their own projects, but I don't think we should recommend it in the documentation (even less make it mandatory).
About people who are unfamiliar with pytest: I understand this might be a bit mysterious for people who don't know pytest at first, but not understanding something immediately is not that big of a deal I think, and it happens with a lot of frameworks and language features too.
I understand the desire to have something to google for, but if you know it is a test file, pytest is the runner, and you want to know more, that can be found quite easily in any tutorial. It is the same with other frameworks, IMHO.
So in summary I'm not opposed to the idea itself, but I don't think Annotated
is the solution (again, to add to pytest as a recommendation, people can use it if they like of course).
When a given test has fixtures defined internally, and fixtures defined externally in a conftest.py, it sometimes gets confusing keeping track of what everything is and where it comes from.
Indeed that is a problem, unfortunately. You can get more information about that by calling pytest --fixtures
.
Also it is worth mentioning that ward uses a explicit mechanism to declare where fixtures come from.
I'm instead concerned instead about people who don't benefit from those IDE features, or who aren't familiar with Pytest. The current syntax is a barrier to entry for those people, and I think it's solvable.
(of course it's a fixture where else could it come from?)
If you're unfamiliar with Pytest, and you're just reading a test file ("oh,
test_foo.py
! This looks relevant! What's in here?"), you really have no clue. The current syntax doesn't give you an obvious thing to Google.
I also agree with @SyntaxColoring that typing should be better handled for readability concerns. From Pep-484:
It is recommended but not required that checked functions have annotations for all arguments and the return type.
To be honest, I do think that pytest should use type annotations everywhere, especially on documentation. In 2022 their benefits a far greater than the downsides. If our tests would be harder to write or more verbose that is a small price to pay.
The lack of documentation around how to write pytest tests using full types is the primary issue AFAIK, as sometimes you have to dig a lot online to find the right syntax to use. Also some of the types moved around and lots of them used to be importable only with private imports, which is not ok. I hope we will slowly address this.
I also agree with @SyntaxColoring that typing should be better handled for readability concerns. From Pep-484:
It is recommended but not required that checked functions have annotations for all arguments and the return type.
I fail to see how that quote is related to any of the previous discussion - you're free to already annotate all of your test functions arguments, as well as the return type (though annotating every test function with -> None
seems redundant to me, but each to their own).
To be honest, I do think that pytest should use type annotations everywhere, especially on documentation. In 2022 their benefits a far greater than the downsides. If our tests would be harder to write or more verbose that is a small price to pay.
That again seems totally unrelated to what this issue and discussion was about? I'd be happy with some more documentation about how to use type annotations with pytest (not sure about using it in examples - keep in mind there are many people new to Python and programming using pytest, those might not care about or benefit from type annotations for fixtures in the same way we do).
The lack of documentation around how to write pytest tests using full types is the primary issue AFAIK, as sometimes you have to dig a lot online to find the right syntax to use.
Do you have some concrete examples?
Also some of the types moved around and lots of them used to be importable only with private imports, which is not ok. I hope we will slowly address this.
See #7469 on that. To my knowledge, with pytest 6.2.0 exposing fixture types and 7.0.0 exposing plugin API types there is public API for 99% of the cases where you need to annotate things coming from pytest. Again, what's there to address?
Even on first page of documentation the example is not using types, https://docs.pytest.org/en/7.1.x/ -- while --> None
might seem redundant it does have a very useful side effect: it does enable type checks, so when the user will try to add the first argument, they will be asked to add a type to it.
Going deeper, to the first page about how to start we find another example without types https://docs.pytest.org/en/7.1.x/getting-started.html#request-a-unique-temporary-directory-for-functional-tests
My personal opinion is that we should not "make it easier" to write untypes tests by giving incomplete examples. If we do this we give the impression that that is the correct way to do it and it would take a long time for those users to unlearn a bad habit.
When we supported ancient versions of python, adding types was a blocker as the code snippets would not have being usable on all supported python versions. That is no longer the case.
I would go so far to suggest us to even ensure we run mypy in strict mode on examples so we prevent giving any examples without types.
I mention this because over the years I encountered engineers that used the "but that is based on official documentation" as an excuse for not doing something, adding types being a notable example. If we document them everywhere it would also make much easier for users to learn how to use them as a considerable number of them start with copy/paste and example.
If nobody is against this, I could spend some time and start adding more types.
I'm personally opposed to typing tests and find it a waste of time (though I do check_untyped_defs for tests which I do think is valuable). tests are going to fail anyway if there's a typing error so it does not provide value for me
but that's entirely off topic for this issue. this issue is about changing the way fixtures work to not be dependency injected by name
To avoid going off-topic I created two polls: Should all pytest docs examples use types? and Should pytest fixtures stop being dependency injected? so we use them to survey what people think and avoid spamming the issue tracker. Obviously that the results are expected to be informative only but it should help us measure what users value more.
@adam-grant-hendry Regarding your comment, when the fixture return type is a Generator[Something]
, what is the test argument type, Generator[Something]
or Something
@adam-grant-hendry Regarding your comment, when the fixture return type is a
Generator[Something]
, what is the test argument type,Generator[Something]
orSomething
The test argument type is Something
as the generator is consumed by Pytest and not the test.
My 2b: typing is great as it reduces the need for handwritten tests and defensive assertions. But there's no protection against an incorrectly typed test:
def test_something(my_fixture: UnrelatedType) -> None:
...
This can escape attention if it passes in pytest and mypy, and in future nobody will remember if it's the fixture that's wrong or the type. It'll help if Pytest treats a mismatched type declaration (based on the fixture's declared return type) as a test collection error.
In our group we are using typing pretty extensively. I don't completely love its bloatiness but it has pinpointed actual bugs in code which is pretty valuable. So I am reviewing some tests now where the annotation for fixtures is the returned type of the fixtures, and that does not seem adequate. My concerns are the same as @sodul opened this ticket for. And happily, the syntax suggested by @The-Compiler back when this thread was opened does the trick. If pytest does go the route of annotating its own API, I'd suggest using that.
My 2b: typing is great as it reduces the need for handwritten tests and defensive assertions. But there's no protection against an incorrectly typed test:
I've solved this for myself with a hook in my top-level conftest.py
:
from typing import Any, get_type_hints
import warnings
import pytest
import typeguard
# Adapted from https://github.com/untitaker/pytest-fixture-typecheck
def pytest_runtest_call(item: pytest.Function) -> None:
try:
annotations = get_type_hints(
item.obj,
globalns=item.obj.__globals__,
localns={'Any': Any}, # pytest-bdd appears to insert an `Any` annotation
)
except TypeError:
# get_type_hints may fail on Python <3.10 because pytest-bdd appears to have
# `dict[str, str]` as a type somewhere, and builtin type subscripting isn't
# supported yet
warnings.warn(
f"Type annotations could not be retrieved for {item.obj!r}", RuntimeWarning
)
return
for attr, type_ in annotations.items():
if attr in item.funcargs:
typeguard.check_type(item.funcargs[attr], type_)
As reported by others, the fact that fixture can exist magically is confusing for many users unfamiliar with pytest and it can be gruesome to track in larger projects with many fixtures.
Since type annotation is working well in python 3 I was thinking that one explicit way would be to have a new
Fixture
type to help annotate the fixture arguments.In this example I 'abuse'
Union
so that existing tools are taking the hinting without any issue. For the person coming in and reading the code, especially in larger projects, the fact that the arguments are fixtures becomes very explicit and in the spirit of the Zen of Python:Unfortunately mypy and other type checking tools don't seem to 'alias'
Union
since it is a special case.This on the other hand works but I would prefer the annotation of the first example:
IDEs such as PyCharm then now able to hint on bob being a string and as a user I can tell that it is a fixture argument.