mirror of
https://github.com/scylladb/scylladb.git
synced 2026-04-23 18:10:39 +00:00
test/pylib: reject bare pytest.mark.skip and add codebase guards
Harden the skip_reason_plugin to reject bare @pytest.mark.skip at collection time with pytest.UsageError instead of warnings.warn(). Add test/pylib_test/test_no_bare_skips.py with three guard tests: - AST scan for bare pytest.skip() runtime calls - Real pytest --collect-only against all Python test directories
This commit is contained in:
@@ -23,7 +23,6 @@ convenience wrappers from :mod:`test.pylib.skip_types`::
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import warnings
|
||||
from collections.abc import Callable
|
||||
from enum import StrEnum
|
||||
|
||||
@@ -114,15 +113,14 @@ class SkipReasonPlugin:
|
||||
item.stash[SKIP_TYPE_KEY] = str(st)
|
||||
item.stash[SKIP_REASON_KEY] = reason
|
||||
|
||||
# Warn on bare pytest.mark.skip not added by typed markers.
|
||||
# Reject bare pytest.mark.skip not added by typed markers.
|
||||
# skip_mode sets SKIP_TYPE_KEY before this hook runs (trylast).
|
||||
if SKIP_TYPE_KEY not in item.stash:
|
||||
bare = [self._get_reason(m) for m in item.iter_markers("skip")]
|
||||
if bare:
|
||||
alternatives = ", ".join(
|
||||
f"@pytest.mark.{st.marker_name}" for st in self._skip_types)
|
||||
# TODO: Change to pytest.fail() after full migration.
|
||||
warnings.warn(
|
||||
raise pytest.UsageError(
|
||||
f"Untyped skip on {item.nodeid}: {'; '.join(bare)}. "
|
||||
f"Use {alternatives} instead.",
|
||||
)
|
||||
|
||||
87
test/pylib_test/test_no_bare_skips.py
Normal file
87
test/pylib_test/test_no_bare_skips.py
Normal file
@@ -0,0 +1,87 @@
|
||||
#
|
||||
# Copyright (C) 2026-present ScyllaDB
|
||||
#
|
||||
# SPDX-License-Identifier: LicenseRef-ScyllaDB-Source-Available-1.1
|
||||
#
|
||||
|
||||
"""Codebase guard tests: verify no bare pytest.skip usage remains.
|
||||
|
||||
These are project-specific tests that scan the ScyllaDB test tree
|
||||
for bare @pytest.mark.skip decorators and bare pytest.skip() calls.
|
||||
Any new skip must use the typed markers or the typed skip() helper.
|
||||
"""
|
||||
|
||||
import ast
|
||||
import os
|
||||
import pathlib
|
||||
import subprocess
|
||||
import sys
|
||||
|
||||
_TEST_ROOT = pathlib.Path(__file__).resolve().parent.parent
|
||||
|
||||
def _iter_test_py_files():
|
||||
"""Yield all .py files under test/ excluding pylib_test/, pylib/ and __pycache__."""
|
||||
for p in sorted(_TEST_ROOT.rglob("*.py")):
|
||||
rel = p.relative_to(_TEST_ROOT)
|
||||
parts = rel.parts
|
||||
if "__pycache__" in parts:
|
||||
continue
|
||||
if parts[0] in ("pylib_test", "pylib"):
|
||||
continue
|
||||
yield p
|
||||
|
||||
|
||||
def test_no_bare_pytest_skip_calls_in_codebase():
|
||||
"""Verify no test files use bare pytest.skip() (must use typed skip() helper)."""
|
||||
violations = []
|
||||
for path in _iter_test_py_files():
|
||||
source = path.read_text()
|
||||
try:
|
||||
tree = ast.parse(source, filename=str(path))
|
||||
except SyntaxError:
|
||||
continue
|
||||
for node in ast.walk(tree):
|
||||
if not isinstance(node, ast.Call):
|
||||
continue
|
||||
func = node.func
|
||||
if (isinstance(func, ast.Attribute) and func.attr == "skip"
|
||||
and isinstance(func.value, ast.Name)
|
||||
and func.value.id == "pytest"):
|
||||
violations.append(f" {path}:{node.lineno}")
|
||||
assert not violations, (
|
||||
"Found bare pytest.skip() — use the typed skip() helper instead "
|
||||
"(from test.pylib.skip_reason_plugin import skip):\n"
|
||||
+ "\n".join(violations)
|
||||
)
|
||||
|
||||
|
||||
def test_no_bare_skip_markers_in_collection():
|
||||
"""Collect all real Python tests and verify no bare @pytest.mark.skip exists.
|
||||
|
||||
The skip_reason_plugin raises pytest.UsageError during collection
|
||||
if any bare skip decorator is found, so --collect-only is enough.
|
||||
No Scylla binary is needed — only Python collection.
|
||||
"""
|
||||
|
||||
# When running under xdist (-n), workers inherit PYTEST_XDIST_WORKER.
|
||||
# If the subprocess inherits it, the runner plugin thinks it is a
|
||||
# worker and skips creating the log directory — causing a
|
||||
# FileNotFoundError. Strip xdist env vars so the subprocess runs
|
||||
# as a standalone main process.
|
||||
env = {k: v for k, v in os.environ.items()
|
||||
if not k.startswith("PYTEST_XDIST")}
|
||||
result = subprocess.run(
|
||||
[sys.executable, "-m", "pytest",
|
||||
"--collect-only",
|
||||
"--ignore=boost", "--ignore=raft",
|
||||
"--ignore=ldap", "--ignore=vector_search",
|
||||
"-p", "no:sugar"],
|
||||
capture_output=True, text=True,
|
||||
cwd=str(_TEST_ROOT),
|
||||
env=env,
|
||||
)
|
||||
# If a bare skip exists, plugin raises UsageError → non-zero exit.
|
||||
assert result.returncode == 0, (
|
||||
"Collection failed — a bare @pytest.mark.skip was found.\n"
|
||||
+ result.stdout + result.stderr
|
||||
)
|
||||
@@ -102,27 +102,26 @@ def test_missing_reason_is_rejected(skippytest, marker):
|
||||
assert result.ret != 0
|
||||
|
||||
|
||||
# -- Bare skip warning ------------------------------------------------------
|
||||
# -- Bare skip rejection -----------------------------------------------------
|
||||
|
||||
def test_bare_skip_warns_and_lists_alternatives(skippytest):
|
||||
"""Bare skip must warn and list all typed alternatives."""
|
||||
def test_bare_skip_rejected_and_lists_alternatives(skippytest):
|
||||
"""Bare skip must be rejected with UsageError listing all typed alternatives."""
|
||||
skippytest.makepyfile("""
|
||||
import pytest
|
||||
@pytest.mark.skip(reason="some bare reason")
|
||||
def test_bare():
|
||||
pass
|
||||
""")
|
||||
result = skippytest.runpytest("-W", "all")
|
||||
result.assert_outcomes(skipped=1)
|
||||
out = result.stdout.str()
|
||||
assert "Untyped skip" in out
|
||||
assert "some bare reason" in out
|
||||
result = skippytest.runpytest()
|
||||
result.stderr.fnmatch_lines(["*Untyped skip*some bare reason*"])
|
||||
assert result.ret != 0
|
||||
out = result.stderr.str()
|
||||
for m in ("skip_bug", "skip_not_implemented", "skip_slow",
|
||||
"skip_env"):
|
||||
assert m in out, f"expected '{m}' in warning output"
|
||||
assert m in out, f"expected '{m}' in error output"
|
||||
|
||||
|
||||
def test_bare_skip_in_pytest_param_warns(skippytest):
|
||||
def test_bare_skip_in_pytest_param_rejected(skippytest):
|
||||
skippytest.makepyfile("""
|
||||
import pytest
|
||||
@pytest.mark.parametrize("x", [
|
||||
@@ -133,21 +132,21 @@ def test_bare_skip_in_pytest_param_warns(skippytest):
|
||||
def test_p(x):
|
||||
pass
|
||||
""")
|
||||
result = skippytest.runpytest("-W", "all")
|
||||
result.assert_outcomes(passed=1, skipped=1)
|
||||
assert "Untyped skip" in result.stdout.str()
|
||||
result = skippytest.runpytest()
|
||||
result.stderr.fnmatch_lines(["*Untyped skip*bare in param*"])
|
||||
assert result.ret != 0
|
||||
|
||||
|
||||
def test_typed_skip_does_not_warn(skippytest):
|
||||
def test_typed_skip_does_not_reject(skippytest):
|
||||
skippytest.makepyfile("""
|
||||
import pytest
|
||||
@pytest.mark.skip_bug(reason="scylladb/scylladb#11111")
|
||||
def test_typed():
|
||||
pass
|
||||
""")
|
||||
result = skippytest.runpytest("-W", "error::UserWarning")
|
||||
result = skippytest.runpytest()
|
||||
result.assert_outcomes(skipped=1)
|
||||
assert "Untyped skip" not in result.stdout.str()
|
||||
assert "Untyped skip" not in result.stderr.str()
|
||||
|
||||
|
||||
# -- Runtime skip helper ----------------------------------------------------
|
||||
@@ -297,8 +296,8 @@ def test_skip_mode_prefix_populates_junit(skippytest, tmp_path):
|
||||
assert "not supported in release" in xml
|
||||
|
||||
|
||||
def test_bare_skip_with_skip_mode_no_warn(skippytest):
|
||||
"""When skip_mode uses skip_marker(), bare-skip warning is suppressed
|
||||
def test_bare_skip_with_skip_mode_no_rejection(skippytest):
|
||||
"""When skip_mode uses skip_marker(), bare-skip rejection is suppressed
|
||||
for the item even if it also has a bare @pytest.mark.skip. The
|
||||
skip_marker() call signals the item already has a typed skip.
|
||||
"""
|
||||
@@ -310,7 +309,6 @@ def test_bare_skip_with_skip_mode_no_warn(skippytest):
|
||||
def test_both_bare_and_mode():
|
||||
assert False
|
||||
""")
|
||||
result = skippytest.runpytest("-W", "all")
|
||||
result = skippytest.runpytest()
|
||||
result.assert_outcomes(skipped=1)
|
||||
out = result.stdout.str()
|
||||
assert "Untyped skip" not in out
|
||||
assert "Untyped skip" not in result.stderr.str()
|
||||
|
||||
Reference in New Issue
Block a user