Skip to content

Conversation

rouge8
Copy link
Contributor

@rouge8 rouge8 commented Apr 3, 2025

These are set in TestReport.__init__ but weren't being picked up by type checkers because of the annotations on BaseReport.

Fixes #12941.

These are set in `TestReport.__init__` but weren't being picked up by
type checkers because of the annotations on `BaseReport`.

Fixes pytest-dev#12941.
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Apr 3, 2025
@rouge8 rouge8 marked this pull request as ready for review April 3, 2025 00:50
Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @rouge8!

@nicoddemus nicoddemus merged commit 4056433 into pytest-dev:main Apr 3, 2025
43 of 52 checks passed
@The-Compiler
Copy link
Member

I don't think this actually works. I added a test:

diff --git i/testing/typing_checks.py w/testing/typing_checks.py
index d4d6a97ae..33180048e 100644
--- i/testing/typing_checks.py
+++ w/testing/typing_checks.py
@@ -8,12 +8,12 @@ none of the code triggers any mypy errors.
 from __future__ import annotations
 
 import contextlib
-from typing import Optional
+from typing import Optional, Literal
 
 from typing_extensions import assert_type
 
 import pytest
-from pytest import MonkeyPatch
+from pytest import MonkeyPatch, TestReport
 
 
 # Issue #7488.
@@ -51,3 +51,9 @@ def check_raises_is_a_context_manager(val: bool) -> None:
     with pytest.raises(RuntimeError) if val else contextlib.nullcontext() as excinfo:
         pass
     assert_type(excinfo, Optional[pytest.ExceptionInfo[RuntimeError]])
+
+
+# Issue #12941.
+def check_testreport_attributes(report: TestReport) -> None:
+    assert_type(report.when, Literal["setup", "call", "teardown"])
+    assert_type(report.location, tuple[str, int | None, str])

and mypy says:

typing_checks.py:58: error: Expression is of type "str | None", not "Literal['setup', 'call', 'teardown']"  [assert-type]

which means the .location issue seems already fixed (the class variable is set as self.location: tuple[str, int | None, str] = location), and the .when issue is still present.

IMHO this should be reverted and the issue reopened.

@rouge8
Copy link
Contributor Author

rouge8 commented Apr 3, 2025

Interesting, I didn't test with assert_type() but I used this test file:

from typing import assert_never
import pytest

def match_it(report: pytest.TestReport) -> None:
    match report.when:
        case "setup":
            pass
        case "call":
            pass
        case "teardown":
            pass
        case _:
            assert_never(report.when)

On main (without this merged), mypy errors with:

t.py:13: error: Argument 1 to "assert_never" has incompatible type "str | None"; expected "Never"  [arg-type]

On my branch, it doesn't have any errors.

@rouge8
Copy link
Contributor Author

rouge8 commented Apr 3, 2025

I don't think this actually works. I added a test:

diff --git i/testing/typing_checks.py w/testing/typing_checks.py
index d4d6a97ae..33180048e 100644
--- i/testing/typing_checks.py
+++ w/testing/typing_checks.py
@@ -8,12 +8,12 @@ none of the code triggers any mypy errors.
 from __future__ import annotations
 
 import contextlib
-from typing import Optional
+from typing import Optional, Literal
 
 from typing_extensions import assert_type
 
 import pytest
-from pytest import MonkeyPatch
+from pytest import MonkeyPatch, TestReport
 
 
 # Issue #7488.
@@ -51,3 +51,9 @@ def check_raises_is_a_context_manager(val: bool) -> None:
     with pytest.raises(RuntimeError) if val else contextlib.nullcontext() as excinfo:
         pass
     assert_type(excinfo, Optional[pytest.ExceptionInfo[RuntimeError]])
+
+
+# Issue #12941.
+def check_testreport_attributes(report: TestReport) -> None:
+    assert_type(report.when, Literal["setup", "call", "teardown"])
+    assert_type(report.location, tuple[str, int | None, str])

and mypy says:

typing_checks.py:58: error: Expression is of type "str | None", not "Literal['setup', 'call', 'teardown']"  [assert-type]

which means the .location issue seems already fixed (the class variable is set as self.location: tuple[str, int | None, str] = location), and the .when issue is still present.

IMHO this should be reverted and the issue reopened.

The assert_type() also passes for report.when when I run it with mypy 1.15.0.

@nicoddemus
Copy link
Member

Hmm thanks for checking @The-Compiler, I was away from the computer and it did seem right to me.

Just so we are all on the same page, which mypy version everyone is using?

@The-Compiler
Copy link
Member

Oh, my bad... I was accidentally running mypy against my system-wide pytest instead of the sources in the repo 😅

What about the location one though, which seems to be fine even without this change? And should when maybe be annotated as self.when: Literal[...] = when in __init__ for consistency with self.location?

rouge8 added a commit to rouge8/pytest that referenced this pull request Apr 3, 2025
rouge8 added a commit to rouge8/pytest that referenced this pull request Apr 3, 2025
rouge8 added a commit to rouge8/pytest that referenced this pull request Apr 3, 2025
@rouge8
Copy link
Contributor Author

rouge8 commented Apr 3, 2025

Good catch! I opened #13348 with that change and the test you suggested.

nicoddemus pushed a commit that referenced this pull request Apr 3, 2025
@rouge8 rouge8 deleted the gh-12491 branch April 3, 2025 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided (automation) changelog entry is part of PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TestReport.when/location have the wrong type reported by type checkers
3 participants