From a82c240ab0099a6369af205423ca91b49fd5154d Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 13 May 2026 01:46:25 +0000 Subject: [PATCH] test(vcr): tighten OVERFLOW classification and switch respx detection to AST MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address two greptile P2 review concerns on PR #27795: 1. MISS:OVERFLOW was firing whenever total > MAX_EPISODES_PER_CASSETTE regardless of cassette state. A cassette that grew past the cap historically but this run only *replayed* (dirty=False) is healthy — the persister never tries to save, so the cache state is stable and the next run will replay too. Only flag OVERFLOW when dirty=True (new episodes were recorded that the persister would refuse to save). Add a regression test covering the dirty=False + large-total case. 2. _module_uses_respx did substring matching on the module source, which false-positives on comments / docstrings / string literals. A comment like # Previously tried respx.mock but switched to vcrpy would keep a file pinned on the opt-out list, defeating the dead-import pruning goal of this PR. Replace the substring scan with an ast.NodeVisitor (_RespxUsageVisitor) that only counts: - @pytest.mark.respx / @respx.mock decorators - with respx.mock(): ... (sync + async) context managers - respx.mock(...) calls outside a with/decorator - function parameters / fixture names equal to respx_mock Add tests for the comment / docstring / string-literal cases plus each real-usage pattern. Co-authored-by: Mateo Wang --- tests/_vcr_conftest_common.py | 149 ++++++++++++++++-- .../test_vcr_classification.py | 66 +++++++- 2 files changed, 196 insertions(+), 19 deletions(-) diff --git a/tests/_vcr_conftest_common.py b/tests/_vcr_conftest_common.py index f90cfa600a..5cc544187b 100644 --- a/tests/_vcr_conftest_common.py +++ b/tests/_vcr_conftest_common.py @@ -5,6 +5,7 @@ See ``tests/llm_translation/Readme.md`` for the full design and from __future__ import annotations +import ast import atexit import hashlib import json @@ -555,18 +556,126 @@ _LIVE_CALL_LOCAL_PREFIXES = ( ) +class _RespxUsageVisitor(ast.NodeVisitor): + """AST visitor that flags real respx wiring in a test module. + + Substring scans of the source text are unreliable: a comment like + ``# Previously used respx.mock`` or a docstring referencing respx + would falsely flag the module. We only count: + + * ``@pytest.mark.respx`` / ``@respx.mock`` decorators + * ``with respx.mock(): ...`` context managers + * ``respx.mock(...)`` / ``respx.mock`` attribute access + * function parameters / fixture arguments named ``respx_mock`` + """ + + def __init__(self) -> None: + self.uses_respx = False + + def _decorator_is_respx(self, dec: ast.expr) -> bool: + # ``@respx.mock`` (Attribute) or ``@respx.mock(...)`` (Call wrapping Attribute) + if isinstance(dec, ast.Call): + dec = dec.func + if isinstance(dec, ast.Attribute): + return ( + isinstance(dec.value, ast.Name) + and dec.value.id == "respx" + and dec.attr == "mock" + ) + return False + + def _is_pytest_mark_respx(self, dec: ast.expr) -> bool: + # ``@pytest.mark.respx`` or ``@pytest.mark.respx(...)``. + if isinstance(dec, ast.Call): + dec = dec.func + if ( + isinstance(dec, ast.Attribute) + and dec.attr == "respx" + and isinstance(dec.value, ast.Attribute) + and dec.value.attr == "mark" + and isinstance(dec.value.value, ast.Name) + and dec.value.value.id == "pytest" + ): + return True + return False + + def _check_decorators(self, decs: list[ast.expr]) -> None: + for d in decs: + if self._decorator_is_respx(d) or self._is_pytest_mark_respx(d): + self.uses_respx = True + + def _check_args(self, args: ast.arguments) -> None: + # ``def test_foo(respx_mock): ...`` — pytest supplies the fixture + # whenever the parameter name appears, regardless of marker. + all_args = ( + list(args.args) + + list(args.kwonlyargs) + + (list(args.posonlyargs) if hasattr(args, "posonlyargs") else []) + ) + for a in all_args: + if a.arg == "respx_mock": + self.uses_respx = True + return + + def visit_FunctionDef(self, node: ast.FunctionDef) -> None: + self._check_decorators(node.decorator_list) + self._check_args(node.args) + self.generic_visit(node) + + def visit_AsyncFunctionDef(self, node: ast.AsyncFunctionDef) -> None: + self._check_decorators(node.decorator_list) + self._check_args(node.args) + self.generic_visit(node) + + def visit_ClassDef(self, node: ast.ClassDef) -> None: + self._check_decorators(node.decorator_list) + self.generic_visit(node) + + def _is_respx_mock_attr(self, node: ast.expr) -> bool: + return ( + isinstance(node, ast.Attribute) + and isinstance(node.value, ast.Name) + and node.value.id == "respx" + and node.attr == "mock" + ) + + def visit_With(self, node: ast.With) -> None: + for item in node.items: + ctx = item.context_expr + if isinstance(ctx, ast.Call): + ctx = ctx.func + if self._is_respx_mock_attr(ctx): + self.uses_respx = True + self.generic_visit(node) + + def visit_AsyncWith(self, node: ast.AsyncWith) -> None: + for item in node.items: + ctx = item.context_expr + if isinstance(ctx, ast.Call): + ctx = ctx.func + if self._is_respx_mock_attr(ctx): + self.uses_respx = True + self.generic_visit(node) + + def visit_Call(self, node: ast.Call) -> None: + # ``respx.mock(...)`` invocation outside a ``with``/decorator — + # e.g. ``mock = respx.mock()`` at module scope. + if self._is_respx_mock_attr(node.func): + self.uses_respx = True + self.generic_visit(node) + + def _module_uses_respx(item) -> bool: """Return True if the test's *module* actually wires up respx. - A bare ``from respx import MockRouter`` import (with no actual usage) - does not patch the httpx transport, so it does not conflict with vcrpy. - We confirm by checking the module's source for any of: - - ``@pytest.mark.respx`` - - ``@respx.mock`` / ``with respx.mock`` - - ``respx_mock`` fixture name + Uses an ``ast`` walk (not substring matching) so comments and + docstrings that mention respx don't count as real usage. A bare + ``from respx import MockRouter`` import with no other respx + references therefore won't flag the module — that's exactly the + dead-import case this PR is trying to surface. """ module = getattr(item, "module", None) - src_file = getattr(module, "__file__", None) + src_file = getattr(module, "__file__", None) or str(getattr(item, "path", "") or "") if not src_file or not os.path.isfile(src_file): return False try: @@ -574,13 +683,16 @@ def _module_uses_respx(item) -> bool: src = f.read() except OSError: return False - if "respx_mock" in src: - return True - if "@pytest.mark.respx" in src or "@respx.mock" in src: - return True - if "respx.mock" in src or "with respx" in src: - return True - return False + try: + tree = ast.parse(src, filename=src_file) + except SyntaxError: + # If the test file itself is broken, fall back to "no respx" — + # the test will fail collection on its own and we don't want + # the auto-marker to mask that with a misleading skip reason. + return False + visitor = _RespxUsageVisitor() + visitor.visit(tree) + return visitor.uses_respx def _item_uses_respx(item) -> bool: @@ -735,8 +847,13 @@ def _classify_marked_test(cassette) -> str: # "OVERFLOW" mirrors ``_RedisPersister.save_cassette``'s # ``> MAX_EPISODES_PER_CASSETTE`` guard. Cassettes that hit this # threshold are refused for save, so the test re-records live every - # run. - if total > MAX_EPISODES_PER_CASSETTE: + # run. Only flag when ``dirty=True`` — if a cassette grew past the + # cap historically but this run replayed it without adding new + # episodes, the persister never tries to save (no recording + # happened), so the cache state is stable and the next run will + # replay too. Flagging that case as OVERFLOW would tag healthy + # cached tests as cost leaks. + if total > MAX_EPISODES_PER_CASSETTE and dirty: return VERDICT_MISS_OVERFLOW if played == 0 and not dirty: return VERDICT_NOOP_NO_TRAFFIC diff --git a/tests/llm_translation/test_vcr_classification.py b/tests/llm_translation/test_vcr_classification.py index 3b08ef5d37..9ab0fedbbf 100644 --- a/tests/llm_translation/test_vcr_classification.py +++ b/tests/llm_translation/test_vcr_classification.py @@ -239,10 +239,13 @@ def test_should_classify_mixed_replay_and_record_as_partial(): ) -def test_should_classify_overflow_as_miss_overflow_regardless_of_play_state(): +def test_should_classify_overflow_only_when_dirty_episodes_were_recorded(): """Cassettes that exceed ``MAX_EPISODES_PER_CASSETTE`` (50) are - refused for save — they will hit live every CI run, so the verdict - must override HIT/PARTIAL classification.""" + refused for save — but only when ``dirty=True`` (new episodes were + actually recorded that the persister would refuse). Replaying an + already-large cassette with no new traffic is healthy: the persister + never tries to save, so the cache state is stable and the next run + will replay too.""" assert ( _classify_marked_test(_cassette(played=0, dirty=True, total=51)) == VERDICT_MISS_OVERFLOW @@ -253,6 +256,20 @@ def test_should_classify_overflow_as_miss_overflow_regardless_of_play_state(): ) +def test_should_classify_large_cassette_with_no_new_episodes_as_hit(): + """``total > 50`` + ``dirty=False`` means everything was replayed + from cache; no save attempt happens, so this is a healthy HIT, not + OVERFLOW.""" + assert ( + _classify_marked_test(_cassette(played=51, dirty=False, total=51)) + == VERDICT_HIT + ) + assert ( + _classify_marked_test(_cassette(played=60, dirty=False, total=60)) + == VERDICT_HIT + ) + + # --------------------------------------------------------------------------- # apply_vcr_auto_marker_to_items: skip-reason tagging # --------------------------------------------------------------------------- @@ -327,6 +344,49 @@ def test_should_tag_skip_files_with_file_opt_out_when_module_does_not_use_respx( assert getattr(item, VCR_SKIP_REASON_USER_ATTR) == SKIP_REASON_FILE_OPT_OUT +def test_should_not_flag_respx_mentioned_in_comment_or_docstring(vcr_enabled, tmp_path): + """Substring scans of source text false-positive on + ``# Previously used respx.mock`` and similar — defeats the dead + skip-list pruning goal. AST-based detection ignores comments and + string literals.""" + src = ( + '"""Module docstring mentions respx.mock and @pytest.mark.respx and respx_mock."""\n' + "# Previously tried respx.mock but switched to vcrpy\n" + "# Old code did `with respx.mock(): ...`\n" + "x = '@respx.mock' # string literal, not a real decorator\n" + "def test_x():\n" + " pass\n" + ) + mod, p = _make_module_with_source(tmp_path, src, "comment_respx") + item = _StubItem("comment_respx.py::test_x", p, module=mod) + apply_vcr_auto_marker_to_items([item], skip_files={"comment_respx.py"}) + assert getattr(item, VCR_SKIP_REASON_USER_ATTR) == SKIP_REASON_FILE_OPT_OUT + + +def test_should_flag_real_respx_mark_decorator_via_ast(vcr_enabled, tmp_path): + src = "import pytest\n" "@pytest.mark.respx\n" "def test_x(respx_mock): pass\n" + mod, p = _make_module_with_source(tmp_path, src, "real_respx_mark") + item = _StubItem("real_respx_mark.py::test_x", p, module=mod) + apply_vcr_auto_marker_to_items([item], skip_files={"real_respx_mark.py"}) + assert getattr(item, VCR_SKIP_REASON_USER_ATTR) == SKIP_REASON_RESPX_MODULE + + +def test_should_flag_real_respx_with_block_via_ast(vcr_enabled, tmp_path): + src = "import respx\n" "def test_x():\n" " with respx.mock():\n" " pass\n" + mod, p = _make_module_with_source(tmp_path, src, "real_respx_with") + item = _StubItem("real_respx_with.py::test_x", p, module=mod) + apply_vcr_auto_marker_to_items([item], skip_files={"real_respx_with.py"}) + assert getattr(item, VCR_SKIP_REASON_USER_ATTR) == SKIP_REASON_RESPX_MODULE + + +def test_should_flag_respx_mock_call_at_module_scope_via_ast(vcr_enabled, tmp_path): + src = "import respx\nmock = respx.mock()\ndef test_x(): pass\n" + mod, p = _make_module_with_source(tmp_path, src, "real_respx_call") + item = _StubItem("real_respx_call.py::test_x", p, module=mod) + apply_vcr_auto_marker_to_items([item], skip_files={"real_respx_call.py"}) + assert getattr(item, VCR_SKIP_REASON_USER_ATTR) == SKIP_REASON_RESPX_MODULE + + def test_should_tag_nodeid_suffix_skips_as_incompatible(vcr_enabled, tmp_path): mod, p = _make_module_with_source(tmp_path, "def test_x(): pass\n", "incompat") item = _StubItem("incompat.py::test_prompt_caching", p, module=mod)