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

Type annotation for fixtures #5981

Closed
sodul opened this issue Oct 17, 2019 · 25 comments
Closed

Type annotation for fixtures #5981

sodul opened this issue Oct 17, 2019 · 25 comments
Labels
topic: fixtures anything involving fixtures directly or indirectly type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature

Comments

@sodul
Copy link

sodul commented Oct 17, 2019

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.

from typing import Union
import pytest

Fixture = Union

@pytest.fixture
def bob() -> str:
    return '42'

@pytest.fixture
def alice() -> int:
    return 42

def test_foo(bob: Fixture[str], alice: Fixture[int]):
    assert bob != alice

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:

Explicit is better than implicit.

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:

from typing import Union
import pytest

@pytest.fixture
def bob() -> str:
    return '42'

@pytest.fixture
def alice() -> int:
    return 42

FixtureStr = Union[str]
FixtureInt = Union[int]

def test_bar(bob: FixtureStr, alice: FixtureInt):
    assert bob != alice

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.

@blueyed
Copy link
Contributor

blueyed commented Oct 17, 2019

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

@The-Compiler
Copy link
Member

The-Compiler commented Oct 17, 2019

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]):
    ...

@davidhalter
Copy link

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 :).

@sodul
Copy link
Author

sodul commented Oct 17, 2019

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.

@Zac-HD Zac-HD added topic: fixtures anything involving fixtures directly or indirectly type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature labels Oct 18, 2019
@Zac-HD
Copy link
Member

Zac-HD commented Oct 18, 2019

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.

@asottile
Copy link
Member

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)

@davidhalter
Copy link

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).

@nicoddemus
Copy link
Member

Thanks everyone for chiming in here, I think it was an useful discussion, but there's nothing really to be done on pytest AFAIU.

@adam-grant-hendry
Copy link

adam-grant-hendry commented Mar 4, 2021

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")

@SyntaxColoring
Copy link

SyntaxColoring commented Jul 28, 2021

@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.

@asottile
Copy link
Member

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

@SyntaxColoring
Copy link

SyntaxColoring commented Jul 29, 2021

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:

  • Have figured out that the Pytest documentation is the place where this is explained, and have read that documentation.
  • Have had Pytest explained to you by a coworker.

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?

@asottile
Copy link
Member

I mean, if you see

def test_thing(some_thing):
    ...

you can very easily git grep some_thing and be at your answer

@nicoddemus
Copy link
Member

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).

@nicoddemus
Copy link
Member

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.

@jeanchristopheruel
Copy link

jeanchristopheruel commented May 9, 2022

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.

@ssbarnea
Copy link
Member

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.

@The-Compiler
Copy link
Member

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?

@ssbarnea
Copy link
Member

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.

@asottile
Copy link
Member

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

@ssbarnea
Copy link
Member

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.

@asarkar
Copy link

asarkar commented Jul 19, 2022

@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

@jace
Copy link

jace commented Feb 28, 2023

@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

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.

@mcowpert
Copy link

mcowpert commented May 8, 2023

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.

@jace
Copy link

jace commented Jan 3, 2024

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_)

ivoire pushed a commit to Linaro/lava that referenced this issue May 2, 2024
Right now the that attribute can be None but only when running in
unit tests. Instead of having special behavior for unit tests
change the unit tests to properly initialize the Pipeline objects.

Changes to classes:

Pipeline __init__
  First argument is now `job` and is required to be job.
  `job` is assigned to the attribute.

Job __init__
  `device` and `timeout` are now required arguments.
  They are assigned to attributes.

To properly initialize the Job objects inside unit tests a new
method was added to the `LavaDispatcherTestCase` called
`create_simple_job`.

The eventual goal is to make dispatcher pass the `mypy --strict`
type checking. Unfortunately the pytest does not support static
typing so all the unit tests that had to be changed were converted
to unittest style tests.

See pytest this developer comment: pytest-dev/pytest#5981 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: fixtures anything involving fixtures directly or indirectly type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature
Projects
None yet
Development

No branches or pull requests