diff --git a/changelog/12582.improvement.rst b/changelog/12582.improvement.rst new file mode 100644 index 00000000000..02d8b55bcc8 --- /dev/null +++ b/changelog/12582.improvement.rst @@ -0,0 +1 @@ +pytest now warns when the reason given to ``pytest.mark.skip`` looks like a condition (for example ``"sys.platform == 'win32'"``), since ``pytest.mark.skipif`` was probably intended. diff --git a/src/_pytest/skipping.py b/src/_pytest/skipping.py index aa97e3b7dc6..51cfd4a3479 100644 --- a/src/_pytest/skipping.py +++ b/src/_pytest/skipping.py @@ -3,6 +3,7 @@ from __future__ import annotations +import ast from collections.abc import Generator from collections.abc import Mapping import dataclasses @@ -24,6 +25,7 @@ from _pytest.reports import TestReport from _pytest.runner import CallInfo from _pytest.stash import StashKey +from _pytest.warning_types import PytestWarning def pytest_addoption(parser: Parser) -> None: @@ -166,6 +168,32 @@ class Skip: reason: str = "unconditional skip" +warned_skip_reasons_key = StashKey[set[str]]() + + +def _looks_like_condition(reason: object) -> bool: + """Return whether a skip reason looks like a skipif condition, e.g. + ``"sys.platform == 'win32'"`` (#12582).""" + if not isinstance(reason, str): + return False + try: + tree = ast.parse(reason.strip(), mode="eval") + except (SyntaxError, ValueError): + return False + for node in ast.walk(tree): + # Attribute access or calls, for example ``sys.platform.startswith('win')``. + if isinstance(node, ast.Attribute | ast.Call): + return True + # Comparisons against values, for example ``os.name == 'nt'``; comparisons + # of plain names are ignored as they are also valid English, for example + # ``"nothing in particular"``. + if isinstance(node, ast.Compare) and not all( + isinstance(operand, ast.Name) for operand in [node.left, *node.comparators] + ): + return True + return False + + def evaluate_skip_marks(item: Item) -> Skip | None: """Evaluate skip and skipif marks on item, returning Skip if triggered.""" for mark in item.iter_markers(name="skipif"): @@ -186,6 +214,22 @@ def evaluate_skip_marks(item: Item) -> Skip | None: return Skip(reason) for mark in item.iter_markers(name="skip"): + reason = mark.args[0] if mark.args else mark.kwargs.get("reason") + # Marks applied to a module or class are seen by each of their items, + # so only warn once per reason. + warned_reasons = item.config.stash.setdefault(warned_skip_reasons_key, set()) + if ( + isinstance(reason, str) + and _looks_like_condition(reason) + and reason not in warned_reasons + ): + warned_reasons.add(reason) + item.warn( + PytestWarning( + f"The reason {reason!r} of pytest.mark.skip looks like a condition. " + "Did you mean pytest.mark.skipif?" + ) + ) try: return Skip(*mark.args, **mark.kwargs) except TypeError as e: diff --git a/testing/test_skipping.py b/testing/test_skipping.py index fbe196915e8..0118aed7c88 100644 --- a/testing/test_skipping.py +++ b/testing/test_skipping.py @@ -1567,3 +1567,76 @@ def test_exit_reason_only(): ) result = pytester.runpytest(p) result.stdout.fnmatch_lines("*_pytest.outcomes.Exit: foo*") + + +@pytest.mark.parametrize( + "reason", + [ + "sys.version_info < (3, 6)", + "sys.platform.startswith('win')", + "os.name == 'nt'", + ], +) +def test_skip_reason_looks_like_condition_warns( + pytester: Pytester, reason: str +) -> None: + pytester.makepyfile( + f""" + import pytest + + @pytest.mark.skip({reason!r}) + def test_func(): + pass + """ + ) + result = pytester.runpytest("-W", "default::pytest.PytestWarning") + result.stdout.fnmatch_lines( + ["*looks like a condition*Did you mean pytest.mark.skipif?"] + ) + result.assert_outcomes(skipped=1, warnings=1) + + +@pytest.mark.parametrize( + "reason", + [ + "flaky", + "not supported on windows", + "nothing in particular", + "Test is not isolated", + ], +) +def test_skip_reason_plain_text_does_not_warn(pytester: Pytester, reason: str) -> None: + pytester.makepyfile( + f""" + import pytest + + @pytest.mark.skip({reason!r}) + def test_func(): + pass + """ + ) + result = pytester.runpytest("-W", "default::pytest.PytestWarning") + result.assert_outcomes(skipped=1, warnings=0) + + +def test_skip_reason_looks_like_condition_warns_once_per_reason( + pytester: Pytester, +) -> None: + """A module- or class-level mark applies to every item, but warns only once.""" + pytester.makepyfile( + """ + import pytest + + pytestmark = pytest.mark.skip("sys.platform == 'win32'") + + def test_a(): pass + def test_b(): pass + + @pytest.mark.skip("os.name == 'nt'") + class TestClass: + def test_c(self): pass + def test_d(self): pass + """ + ) + result = pytester.runpytest("-W", "default::pytest.PytestWarning") + result.assert_outcomes(skipped=4, warnings=2)