improvement: show return type annotations in fixtures #13680
Conversation
psf-chronographer
bot
added
the
bot:chronographer:provided
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
src/_pytest/fixtures.py
Outdated
| if annotation is not sig.empty and annotation != inspect._empty: | ||
| return inspect.formatannotation(annotation).replace("'", "") |
There was a problem hiding this comment.
This seems odd to me, for various reasons:
inspect._emptyis private so it shouldn't be usedinspect.formatannotation, while public, is undocumented so probably shouldn't be used either- Why remove
'from it, which is somethinginspectitself doesn't seem to do?
There was a problem hiding this comment.
Yes, that inspect._empty condition is actually redundant. I investigated the logic further and there were lots of inconsistent behaviors in the test (that's why I previously did replace("'", "")) and corner cases. However, the new solution (though a bit hacky) is the most general one and hopefully covering everything.
changelog/13676.improvement.rst
Outdated
| @@ -0,0 +1 @@ | |||
| Added return type annotations in ``fixtures`` and ``fixtures-per-test``. | |||
There was a problem hiding this comment.
| Added return type annotations in ``fixtures`` and ``fixtures-per-test``. | |
| Added return type annotations in ``--fixtures`` and ``--fixtures-per-test``. |
| if annotation.__module__ == "typing": | ||
| return str(annotation).replace("typing.", "") | ||
| return annotation.__name__ | ||
| except (ValueError, TypeError): |
There was a problem hiding this comment.
Why would ValueError and TypeError happen here? The only thing that could happen I think is AttributeError, which already happens with e.g. -> None: as annotation.
That being said, None should definitively handled correctly by the code and tested properly as well.
There was a problem hiding this comment.
TypeError happens when the argument is not a callable (e.g. a number). I doubt this case happens. ValueError happens when the passed argument is a callable but a signature can't be obtained (e.g. range). Also unlikely imo.
For -> None: on my local test didn't raise any error.
testing/python/fixtures.py
Outdated
| @@ -5068,3 +5088,45 @@ def test_method(self, /, fix): | |||
| ) | |||
| result = pytester.runpytest() | |||
| result.assert_outcomes(passed=1) | |||
|
|
|||
|
|
|||
| def test_get_return_annotation() -> None: | |||
There was a problem hiding this comment.
Maybe turn this into a test class and split the individual asserts into separate test functions? That way, if one of them fails, the rest of the tests will still run.
testing/python/fixtures.py
Outdated
|
|
||
| assert get_return_annotation(no_annotation) == "" | ||
|
|
||
| def none_return() -> None: |
There was a problem hiding this comment.
Ah, this test happens to work (despite the bug above) because this module uses from __future__ import annotations, so the annotation already is 'None' rather than None.
Not sure how to best get around this. Maybe those tests should be in a separate module which deliberately doesn't use that?
There was a problem hiding this comment.
On my local test (3.13) with or without from __future__ import annotations it worked the same. I might be very wrong but I think it behaves differently in different Python versions. Still, I added a exclusive check for none. Do you think moving it to another module is worth it? Seems too much for a humble helper function!
PS: The reason I didn't use `isinstance(annotation, types.NoneType) was that pylance was complaining.
closes #13676