From a598a742ca2622ae45da760e9bc5448523ede117 Mon Sep 17 00:00:00 2001 From: Dvir Dukhan Date: Wed, 27 May 2026 14:08:40 +0300 Subject: [PATCH 1/3] test(mcp): sample-project fixture + assertion contract (T3 #650) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New `tests/mcp/fixtures/`: - `sample_project/python/` — canonical call graph `entrypoint -> service -> {UserRepo,OrderRepo}.repo -> db` plus a small class hierarchy (BaseRepo <- UserRepo, OrderRepo) and inter-file imports so IMPORTS edges exist. - `expected.yaml` — single source of truth for every per-tool ticket's integration assertions: minimum per-label counts, named callers / callees, known paths, prefix-search hits. New `tests/mcp/conftest.py`: - `expected_contract` (pure-Python, always available) loads the YAML once per session. - `indexed_fixture` (session-scoped) indexes the fixture into a unique `code:sample_project:test-` graph so parallel CI shards don't contend. Self-skips when FalkorDB is unreachable. Uses `SourceAnalyzer.analyze_local_folder` directly so the fixture doesn't need to be a git repository. New `tests/mcp/test_fixture_contract.py` — regression-tests the fixture itself: contract shape, on-disk files, and that the integration fixture indexes cleanly and meets the minimum count contract. Multilingual coverage (Java + C#) was dropped from the spec: both multilspy analyzers demand a Maven / .NET project layout at the indexed root, which would force this fixture into an awkward shape. Deferred to a follow-up ticket (likely T16 which adds languages). All 4 contract tests pass against FalkorDB on 6390. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pyproject.toml | 1 + tests/mcp/conftest.py | 116 ++++++++++++++++++ tests/mcp/fixtures/expected.yaml | 48 ++++++++ tests/mcp/fixtures/sample_project/README.md | 36 ++++++ .../sample_project/python/__init__.py | 1 + .../mcp/fixtures/sample_project/python/db.py | 6 + .../sample_project/python/entrypoint.py | 17 +++ .../fixtures/sample_project/python/repo.py | 23 ++++ .../fixtures/sample_project/python/service.py | 10 ++ tests/mcp/test_fixture_contract.py | 81 ++++++++++++ uv.lock | 2 + 11 files changed, 341 insertions(+) create mode 100644 tests/mcp/conftest.py create mode 100644 tests/mcp/fixtures/expected.yaml create mode 100644 tests/mcp/fixtures/sample_project/README.md create mode 100644 tests/mcp/fixtures/sample_project/python/__init__.py create mode 100644 tests/mcp/fixtures/sample_project/python/db.py create mode 100644 tests/mcp/fixtures/sample_project/python/entrypoint.py create mode 100644 tests/mcp/fixtures/sample_project/python/repo.py create mode 100644 tests/mcp/fixtures/sample_project/python/service.py create mode 100644 tests/mcp/test_fixture_contract.py diff --git a/pyproject.toml b/pyproject.toml index be0a3186..ab0c176e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -49,4 +49,5 @@ where = ["."] dev = [ "pytest>=9.0.2", "pytest-anyio>=0.0.0", + "pyyaml>=6.0.3", ] diff --git a/tests/mcp/conftest.py b/tests/mcp/conftest.py new file mode 100644 index 00000000..1b24abe1 --- /dev/null +++ b/tests/mcp/conftest.py @@ -0,0 +1,116 @@ +"""Shared fixtures for the MCP test suite. + +Every per-tool integration test from T4 onward reuses the +``indexed_fixture`` fixture below: it indexes ``fixtures/sample_project`` +into a uniquely named FalkorDB graph once per test session and yields a +descriptor (project name, branch, graph name) the test can pass straight +to the MCP tool under test. + +The integration fixture is opt-in — it requires a reachable FalkorDB +(see ``api/graph.py``) and the optional language analyzers. Tests that +only need the static contract (counts, named callers / callees / paths) +can depend on ``expected_contract`` alone, which is pure-Python and +always available. +""" + +from __future__ import annotations + +import os +import uuid +from dataclasses import dataclass +from pathlib import Path +from typing import Any + +import pytest +import yaml + + +FIXTURE_DIR = Path(__file__).parent / "fixtures" +SAMPLE_PROJECT = FIXTURE_DIR / "sample_project" +EXPECTED_PATH = FIXTURE_DIR / "expected.yaml" + + +# --------------------------------------------------------------------------- +# Pure-Python contract (no FalkorDB required) +# --------------------------------------------------------------------------- + + +@dataclass(frozen=True) +class IndexedFixture: + """Descriptor for an indexed fixture graph.""" + + project: str + branch: str + graph_name: str + path: Path + + +@pytest.fixture(scope="session") +def expected_contract() -> dict[str, Any]: + """Load ``fixtures/expected.yaml`` once per session.""" + with EXPECTED_PATH.open() as fh: + return yaml.safe_load(fh) + + +@pytest.fixture(scope="session") +def sample_project_path() -> Path: + """Filesystem path to the fixture project.""" + return SAMPLE_PROJECT + + +# --------------------------------------------------------------------------- +# Integration fixture — indexes into a real FalkorDB +# --------------------------------------------------------------------------- + + +def _falkordb_reachable() -> bool: + """Cheap probe so the integration fixture can self-skip in dev.""" + try: + import socket + + host = os.getenv("FALKORDB_HOST", "localhost") + port = int(os.getenv("FALKORDB_PORT", 6379)) + with socket.create_connection((host, port), timeout=1): + return True + except OSError: + return False + + +@pytest.fixture(scope="session") +def indexed_fixture(sample_project_path: Path) -> IndexedFixture: + """Index the sample project into a unique per-session graph. + + Each test session creates a new graph named + ``code:sample_project:test-`` so parallel CI shards never + contend on the same graph. The graph is intentionally **not** + cleaned up — short-lived CI runners discard the FalkorDB volume, + and keeping it around helps post-mortem debugging on developer + machines. + + Uses :class:`api.analyzers.SourceAnalyzer` directly (instead of + ``Project.from_local_repository``) so the fixture doesn't need to + be a git repository — analyzing a plain directory is exactly the + code path the ``index_repo`` MCP tool exercises for non-git + folders. + """ + + if not _falkordb_reachable(): + pytest.skip("FalkorDB not reachable on $FALKORDB_HOST:$FALKORDB_PORT") + + # Import locally so unit-only tests don't pay the import cost. + from api.analyzers.source_analyzer import SourceAnalyzer + from api.graph import Graph + + project_name = sample_project_path.name # "sample_project" + branch = f"test-{uuid.uuid4().hex[:8]}" + graph = Graph(project_name, branch=branch) + + analyzer = SourceAnalyzer() + analyzer.analyze_local_folder(str(sample_project_path), graph) + + return IndexedFixture( + project=project_name, + branch=branch, + graph_name=graph.name, + path=sample_project_path, + ) diff --git a/tests/mcp/fixtures/expected.yaml b/tests/mcp/fixtures/expected.yaml new file mode 100644 index 00000000..061258f1 --- /dev/null +++ b/tests/mcp/fixtures/expected.yaml @@ -0,0 +1,48 @@ +# MCP fixture assertion contract. +# +# Anything declared here is treated as load-bearing by tests/mcp/*. The +# precise numeric counts depend on which analyzers run in CI (some test +# environments skip the multilspy passes), so the per-label counts are +# expressed as minimums (`>=`) rather than equalities. Named-symbol +# assertions are exact. + +# The graph is named after the fixture directory (per Project.from_local_repository). +project_name: sample_project + +# Minimum counts produced by the Python tree-sitter analyzer alone. +# Java + C# add more when multilspy is available. +counts_min: + File: 4 # 4 python files (java/csharp may bump this) + Class: 3 # BaseRepo, UserRepo, OrderRepo + Function: 6 # entrypoint, service, db, BaseRepo.repo, UserRepo.repo, OrderRepo.repo + +# Named callers / callees (used by T5 — get_callers / get_callees). +calls: + service: + callers: ["entrypoint"] + # service() instantiates UserRepo + OrderRepo and calls .repo() on each; + # the analyzer encodes that as CALLS edges on the method. + callees_any_of: ["repo", "UserRepo", "OrderRepo"] + entrypoint: + callers: [] + callees_any_of: ["service"] + db: + # db() is called by both subclasses via super().repo() -> BaseRepo.repo(). + callers_any_of: ["repo"] + callees: [] + +# Known path between two named symbols (used by T7 — find_path). +paths: + - source: entrypoint + dest: db + min_paths: 1 + +# Prefix-search hits (used by T8 — search_code). +search_prefixes: + ent: + must_include: ["entrypoint"] + serv: + must_include: ["service"] + Repo: + # case-insensitive prefix match on Searchable label + must_include_any_of: ["BaseRepo", "UserRepo", "OrderRepo"] diff --git a/tests/mcp/fixtures/sample_project/README.md b/tests/mcp/fixtures/sample_project/README.md new file mode 100644 index 00000000..2c3f6147 --- /dev/null +++ b/tests/mcp/fixtures/sample_project/README.md @@ -0,0 +1,36 @@ +# Sample fixture project for the MCP test suite + +This directory is consumed by `tests/mcp/conftest.py::indexed_fixture`. Every +MCP tool ticket from T4 onward asserts against the assertions declared in +`expected.yaml`. + +## Canonical Python call graph + +``` +entrypoint() -> service() -> {UserRepo,OrderRepo}.repo() -> db() +``` + +Plus a small class hierarchy: + +``` +BaseRepo + ├── UserRepo + └── OrderRepo +``` + +## Why three languages? (deferred) + +The original T3 spec called for one Java + one C# file so multilspy's +second-pass code paths would be exercised. In practice both analyzers +demand a real Maven / .NET project layout at the **root** of the indexed +tree, which would make this fixture awkward to co-host with the Python +sample. The multilingual coverage is therefore deferred to a follow-up +ticket (likely T16, which already pulls in additional languages). + +T4-T8 only need Python, which this fixture covers in full. + +## Stability contract + +If you change this fixture, you must also update `expected.yaml`. Tests +read counts and named symbols directly from that file so the assertion +contract stays in lock-step. diff --git a/tests/mcp/fixtures/sample_project/python/__init__.py b/tests/mcp/fixtures/sample_project/python/__init__.py new file mode 100644 index 00000000..374aa568 --- /dev/null +++ b/tests/mcp/fixtures/sample_project/python/__init__.py @@ -0,0 +1 @@ +"""Marks ``sample_project/python`` as a package so IMPORTS edges resolve.""" diff --git a/tests/mcp/fixtures/sample_project/python/db.py b/tests/mcp/fixtures/sample_project/python/db.py new file mode 100644 index 00000000..37a382de --- /dev/null +++ b/tests/mcp/fixtures/sample_project/python/db.py @@ -0,0 +1,6 @@ +"""Bottom of the canonical call chain.""" + + +def db() -> str: + """Leaf function — entrypoint -> service -> repo -> db.""" + return "db" diff --git a/tests/mcp/fixtures/sample_project/python/entrypoint.py b/tests/mcp/fixtures/sample_project/python/entrypoint.py new file mode 100644 index 00000000..7edcce4a --- /dev/null +++ b/tests/mcp/fixtures/sample_project/python/entrypoint.py @@ -0,0 +1,17 @@ +"""Entrypoint for the MCP test fixture project. + +Call graph (must match ``expected.yaml``): + + entrypoint() -> service() -> repo() -> db() +""" + +from .service import service + + +def entrypoint() -> str: + """Top of the canonical call chain used by every MCP integration test.""" + return service() + + +if __name__ == "__main__": + entrypoint() diff --git a/tests/mcp/fixtures/sample_project/python/repo.py b/tests/mcp/fixtures/sample_project/python/repo.py new file mode 100644 index 00000000..c9295ce0 --- /dev/null +++ b/tests/mcp/fixtures/sample_project/python/repo.py @@ -0,0 +1,23 @@ +"""Repository layer for the MCP fixture project. + +Exercises a small class hierarchy: ``BaseRepo`` <- ``UserRepo`` / ``OrderRepo``. +""" + +from .db import db + + +class BaseRepo: + """Base class so the analyzer emits an INHERITS edge.""" + + def repo(self) -> str: + return db() + + +class UserRepo(BaseRepo): + def repo(self) -> str: + return "user:" + super().repo() + + +class OrderRepo(BaseRepo): + def repo(self) -> str: + return "order:" + super().repo() diff --git a/tests/mcp/fixtures/sample_project/python/service.py b/tests/mcp/fixtures/sample_project/python/service.py new file mode 100644 index 00000000..1afdcab7 --- /dev/null +++ b/tests/mcp/fixtures/sample_project/python/service.py @@ -0,0 +1,10 @@ +"""Service layer for the MCP fixture project.""" + +from .repo import UserRepo, OrderRepo + + +def service() -> str: + """Middle of the canonical call chain: entrypoint -> service -> repo.""" + users = UserRepo() + orders = OrderRepo() + return users.repo() + ":" + orders.repo() diff --git a/tests/mcp/test_fixture_contract.py b/tests/mcp/test_fixture_contract.py new file mode 100644 index 00000000..9e426c90 --- /dev/null +++ b/tests/mcp/test_fixture_contract.py @@ -0,0 +1,81 @@ +"""T3 — assertion contract sanity check. + +These tests validate the *fixture itself*: that the YAML contract parses, +that the sample-project tree on disk matches what the contract declares, +and that the integration fixture can actually index the project against +a live FalkorDB. Tool-specific assertions live in the per-tool ticket +test modules (T4, T5, T7, T8 ...). +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from tests.mcp.conftest import EXPECTED_PATH, SAMPLE_PROJECT + + +# --------------------------------------------------------------------------- +# Pure-Python contract checks (always run) +# --------------------------------------------------------------------------- + + +def test_expected_yaml_exists(): + assert EXPECTED_PATH.is_file(), "fixtures/expected.yaml must exist" + + +def test_expected_contract_shape(expected_contract): + """Required top-level keys are present and well-typed.""" + + assert expected_contract["project_name"] == "sample_project" + + counts = expected_contract["counts_min"] + for label in ("File", "Class", "Function"): + assert label in counts and isinstance(counts[label], int) and counts[label] >= 0 + + calls = expected_contract["calls"] + for sym in ("service", "entrypoint", "db"): + assert sym in calls, f"calls.{sym} missing from expected.yaml" + + paths = expected_contract["paths"] + assert isinstance(paths, list) and len(paths) >= 1 + for p in paths: + for k in ("source", "dest", "min_paths"): + assert k in p, f"path entry missing key: {k}" + + prefixes = expected_contract["search_prefixes"] + assert "ent" in prefixes + + +def test_sample_project_python_files_present(): + """The Python tree the contract references must exist on disk.""" + py = SAMPLE_PROJECT / "python" + for name in ("entrypoint.py", "service.py", "repo.py", "db.py", "__init__.py"): + assert (py / name).is_file(), f"missing fixture file: python/{name}" + + +# --------------------------------------------------------------------------- +# Integration check — requires FalkorDB; self-skips when unreachable +# --------------------------------------------------------------------------- + + +def test_indexed_fixture_loads_minimum_counts(indexed_fixture, expected_contract): + """The fixture indexes cleanly and meets the minimum count contract. + + Subsequent per-tool tickets (T4+) use ``indexed_fixture`` directly; + this test exists so the fixture itself is regression-tested in + isolation. + """ + + from api.graph import Graph + + g = Graph(indexed_fixture.project, branch=indexed_fixture.branch) + counts_min = expected_contract["counts_min"] + + for label, minimum in counts_min.items(): + rows = g.g.query(f"MATCH (n:{label}) RETURN count(n) AS c").result_set + actual = rows[0][0] if rows else 0 + assert actual >= minimum, ( + f"label {label}: expected >={minimum}, got {actual}" + ) diff --git a/uv.lock b/uv.lock index 0258bad8..ac98c2d8 100644 --- a/uv.lock +++ b/uv.lock @@ -383,6 +383,7 @@ test = [ dev = [ { name = "pytest" }, { name = "pytest-anyio" }, + { name = "pyyaml" }, ] [package.metadata] @@ -416,6 +417,7 @@ provides-extras = ["test"] dev = [ { name = "pytest", specifier = ">=9.0.2" }, { name = "pytest-anyio", specifier = ">=0.0.0" }, + { name = "pyyaml", specifier = ">=6.0.3" }, ] [[package]] From b7cbbb0da5512a9f3eae4205af283b9a78022e0f Mon Sep 17 00:00:00 2001 From: Dvir Dukhan <12258836+DvirDukhan@users.noreply.github.com> Date: Fri, 29 May 2026 01:01:22 +0300 Subject: [PATCH 2/3] =?UTF-8?q?test(mcp):=20address=20PR=20#677=20review?= =?UTF-8?q?=20=E2=80=94=20exact=20counts=20+=20falkordb-py=20ping?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - expected.yaml: counts_min -> counts (exact); File: 4 -> File: 5 (analyzer counts __init__.py); paths use exact paths_count. - test_fixture_contract.py: shape check + indexed-fixture test now assert equality, not >=. Drops analyzer-availability hedge — the fixture is Python-only and the deterministic tree-sitter analyzer always covers it. - conftest.py: replace raw-socket reachability probe with falkordb-py's FalkorDB(...).connection.ping(); same connection settings as the rest of the suite, surfaces auth/protocol errors rather than just "port is open". - conftest.py: pass ignore=[venv, __pycache__, .venv] to analyze_local_folder so a stray venv (jedi/multilspy used to auto-create one) can never break the exact-count contract. - fixtures/sample_project/.gitignore: prevent committing venv/. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/mcp/conftest.py | 27 +++++++++++++++----- tests/mcp/fixtures/expected.yaml | 22 ++++++++-------- tests/mcp/fixtures/sample_project/.gitignore | 6 +++++ tests/mcp/test_fixture_contract.py | 25 +++++++++--------- 4 files changed, 52 insertions(+), 28 deletions(-) create mode 100644 tests/mcp/fixtures/sample_project/.gitignore diff --git a/tests/mcp/conftest.py b/tests/mcp/conftest.py index 1b24abe1..3a0b8e24 100644 --- a/tests/mcp/conftest.py +++ b/tests/mcp/conftest.py @@ -64,15 +64,21 @@ def sample_project_path() -> Path: def _falkordb_reachable() -> bool: - """Cheap probe so the integration fixture can self-skip in dev.""" + """Cheap probe so the integration fixture can self-skip in dev. + + Uses the falkordb-py client (rather than a raw TCP socket) so the + check exercises the same connection settings the rest of the test + suite uses, and surfaces auth / protocol failures — not just + "something is listening on that port". + """ try: - import socket + from falkordb import FalkorDB host = os.getenv("FALKORDB_HOST", "localhost") port = int(os.getenv("FALKORDB_PORT", 6379)) - with socket.create_connection((host, port), timeout=1): - return True - except OSError: + db = FalkorDB(host=host, port=port, socket_timeout=1) + return bool(db.connection.ping()) + except Exception: return False @@ -106,7 +112,16 @@ def indexed_fixture(sample_project_path: Path) -> IndexedFixture: graph = Graph(project_name, branch=branch) analyzer = SourceAnalyzer() - analyzer.analyze_local_folder(str(sample_project_path), graph) + # Belt-and-suspenders: jedi/multilspy used to create venv/ inside the + # fixture during resolution, which polluted the tree with hundreds of + # site-packages files. Tree-sitter doesn't do this, but we still pass + # an explicit ignore list so a stray venv on a contributor's machine + # can never break the exact-count contract. + analyzer.analyze_local_folder( + str(sample_project_path), + graph, + ignore=["venv", "__pycache__", ".venv"], + ) return IndexedFixture( project=project_name, diff --git a/tests/mcp/fixtures/expected.yaml b/tests/mcp/fixtures/expected.yaml index 061258f1..97d1aa9f 100644 --- a/tests/mcp/fixtures/expected.yaml +++ b/tests/mcp/fixtures/expected.yaml @@ -1,18 +1,19 @@ # MCP fixture assertion contract. # -# Anything declared here is treated as load-bearing by tests/mcp/*. The -# precise numeric counts depend on which analyzers run in CI (some test -# environments skip the multilspy passes), so the per-label counts are -# expressed as minimums (`>=`) rather than equalities. Named-symbol -# assertions are exact. +# Anything declared here is treated as load-bearing by tests/mcp/*. +# Counts are EXACT (not minimums): the fixture is pinned to a Python-only +# tree the deterministic tree-sitter analyzer fully covers, so there is +# no analyzer-availability skew to hedge against. +# Named-symbol assertions are also exact. # The graph is named after the fixture directory (per Project.from_local_repository). project_name: sample_project -# Minimum counts produced by the Python tree-sitter analyzer alone. -# Java + C# add more when multilspy is available. -counts_min: - File: 4 # 4 python files (java/csharp may bump this) +# Exact counts produced by the Python tree-sitter analyzer over the +# fixture as committed. If you add/remove a fixture source file, update +# these numbers to match. +counts: + File: 5 # entrypoint.py, service.py, repo.py, db.py, __init__.py Class: 3 # BaseRepo, UserRepo, OrderRepo Function: 6 # entrypoint, service, db, BaseRepo.repo, UserRepo.repo, OrderRepo.repo @@ -32,10 +33,11 @@ calls: callees: [] # Known path between two named symbols (used by T7 — find_path). +# Exact path count per (source, dest) pair. paths: - source: entrypoint dest: db - min_paths: 1 + paths_count: 1 # Prefix-search hits (used by T8 — search_code). search_prefixes: diff --git a/tests/mcp/fixtures/sample_project/.gitignore b/tests/mcp/fixtures/sample_project/.gitignore new file mode 100644 index 00000000..1e8a7f63 --- /dev/null +++ b/tests/mcp/fixtures/sample_project/.gitignore @@ -0,0 +1,6 @@ +# Keep the fixture deterministic: jedi/multilspy historically auto-created +# a venv/ here during indexing, which polluted the tree with hundreds of +# stdlib + site-packages files and broke the exact-count contract. +venv/ +__pycache__/ +*.pyc diff --git a/tests/mcp/test_fixture_contract.py b/tests/mcp/test_fixture_contract.py index 9e426c90..1d239eb5 100644 --- a/tests/mcp/test_fixture_contract.py +++ b/tests/mcp/test_fixture_contract.py @@ -26,23 +26,24 @@ def test_expected_yaml_exists(): def test_expected_contract_shape(expected_contract): - """Required top-level keys are present and well-typed.""" + """Required top-level keys are present with the expected exact shape.""" assert expected_contract["project_name"] == "sample_project" - counts = expected_contract["counts_min"] - for label in ("File", "Class", "Function"): - assert label in counts and isinstance(counts[label], int) and counts[label] >= 0 + counts = expected_contract["counts"] + # Exact, not >=: the fixture is deterministic Python-only tree-sitter. + assert counts == {"File": 5, "Class": 3, "Function": 6} calls = expected_contract["calls"] for sym in ("service", "entrypoint", "db"): assert sym in calls, f"calls.{sym} missing from expected.yaml" paths = expected_contract["paths"] - assert isinstance(paths, list) and len(paths) >= 1 + assert isinstance(paths, list) and len(paths) == 1 for p in paths: - for k in ("source", "dest", "min_paths"): + for k in ("source", "dest", "paths_count"): assert k in p, f"path entry missing key: {k}" + assert isinstance(p["paths_count"], int) and p["paths_count"] >= 1 prefixes = expected_contract["search_prefixes"] assert "ent" in prefixes @@ -60,8 +61,8 @@ def test_sample_project_python_files_present(): # --------------------------------------------------------------------------- -def test_indexed_fixture_loads_minimum_counts(indexed_fixture, expected_contract): - """The fixture indexes cleanly and meets the minimum count contract. +def test_indexed_fixture_loads_exact_counts(indexed_fixture, expected_contract): + """The fixture indexes cleanly and matches the exact count contract. Subsequent per-tool tickets (T4+) use ``indexed_fixture`` directly; this test exists so the fixture itself is regression-tested in @@ -71,11 +72,11 @@ def test_indexed_fixture_loads_minimum_counts(indexed_fixture, expected_contract from api.graph import Graph g = Graph(indexed_fixture.project, branch=indexed_fixture.branch) - counts_min = expected_contract["counts_min"] + counts = expected_contract["counts"] - for label, minimum in counts_min.items(): + for label, expected in counts.items(): rows = g.g.query(f"MATCH (n:{label}) RETURN count(n) AS c").result_set actual = rows[0][0] if rows else 0 - assert actual >= minimum, ( - f"label {label}: expected >={minimum}, got {actual}" + assert actual == expected, ( + f"label {label}: expected exactly {expected}, got {actual}" ) From 42b794b7c98460f18cbc823a0cf517219a688fcd Mon Sep 17 00:00:00 2001 From: Dvir Dukhan <12258836+DvirDukhan@users.noreply.github.com> Date: Fri, 29 May 2026 01:18:38 +0300 Subject: [PATCH 3/3] test(mcp): address second round of PR #677 review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five comments from @DvirDukhan + bots: 1. (Copilot) indexed_fixture leaked test- graphs into FalkorDB on every run. Convert to a yield fixture that drops the graph on teardown; preserve post-mortem debugging via opt-in MCP_KEEP_TEST_GRAPHS=1 env var. 2. (Copilot) T3 acceptance #650 requires asserting at least one named caller/callee, named path, and prefix-search hit. The original PR only checked node counts; the calls/paths/search_prefixes sections of expected.yaml were never exercised. Add three integration tests: - test_indexed_fixture_callers_and_callees: exact sorted lists of CALLS in/out for each named symbol. - test_indexed_fixture_paths: exact path count for each (src,dst). - test_indexed_fixture_prefix_search: assert each must_include name resolves via api.auto_complete.prefix_search. Probing the live graph revealed two contract drifts that the missing tests had hidden: - entrypoint -> db has 2 paths, not 1 (one per subclass: UserRepo and OrderRepo both route through their .repo() override). - prefix 'Repo' matches lowercase 'repo' (Searchable.name case-insensitive startswith), not BaseRepo/UserRepo/OrderRepo. Swap to prefix 'Base' which actually resolves to 'BaseRepo'. Also tightened expected_contract_shape to require both callers and callees as exact lists (was: just 'sym in calls'). 3. (Copilot) fixtures/sample_project/python/repo.py docstring said the analyzer emits an INHERITS edge; the analyzer actually emits EXTENDS (api/analyzers/source_analyzer.py:169). Update the docstring to match so a future contributor writing T6 doesn't try to assert against a non-existent edge type. 4. (code-quality bot) unused 'Path' import in tests/mcp/test_fixture_contract.py — removed. 5. (code-quality bot) unused 'pytest' import in same file — removed. All 10 mcp tests pass; teardown verified empty (and verified the MCP_KEEP_TEST_GRAPHS=1 escape hatch leaves the graph in place). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/mcp/conftest.py | 26 ++++- tests/mcp/fixtures/expected.yaml | 26 +++-- .../fixtures/sample_project/python/repo.py | 2 +- tests/mcp/test_fixture_contract.py | 108 +++++++++++++++++- 4 files changed, 141 insertions(+), 21 deletions(-) diff --git a/tests/mcp/conftest.py b/tests/mcp/conftest.py index 3a0b8e24..cb381b3c 100644 --- a/tests/mcp/conftest.py +++ b/tests/mcp/conftest.py @@ -83,15 +83,18 @@ def _falkordb_reachable() -> bool: @pytest.fixture(scope="session") -def indexed_fixture(sample_project_path: Path) -> IndexedFixture: +def indexed_fixture(sample_project_path: Path): """Index the sample project into a unique per-session graph. Each test session creates a new graph named ``code:sample_project:test-`` so parallel CI shards never - contend on the same graph. The graph is intentionally **not** - cleaned up — short-lived CI runners discard the FalkorDB volume, - and keeping it around helps post-mortem debugging on developer - machines. + contend on the same graph. The graph is deleted on teardown so + long-lived FalkorDB deployments (developer machines, shared CI) + don't accumulate orphan ``test-*`` graphs across runs. + + Set ``MCP_KEEP_TEST_GRAPHS=1`` to skip teardown — useful for + post-mortem debugging when a test fails and you want to poke at + the graph by hand. Uses :class:`api.analyzers.SourceAnalyzer` directly (instead of ``Project.from_local_repository``) so the fixture doesn't need to @@ -123,9 +126,20 @@ def indexed_fixture(sample_project_path: Path) -> IndexedFixture: ignore=["venv", "__pycache__", ".venv"], ) - return IndexedFixture( + yield IndexedFixture( project=project_name, branch=branch, graph_name=graph.name, path=sample_project_path, ) + + # Teardown — drop the graph so we don't leak ``test-`` graphs + # across runs. Opt out with MCP_KEEP_TEST_GRAPHS=1 when debugging. + if os.getenv("MCP_KEEP_TEST_GRAPHS") == "1": + return + try: + graph.delete() + except Exception: + # Best-effort cleanup: never fail a test session because + # teardown couldn't reach FalkorDB. + pass diff --git a/tests/mcp/fixtures/expected.yaml b/tests/mcp/fixtures/expected.yaml index 97d1aa9f..7bd290d7 100644 --- a/tests/mcp/fixtures/expected.yaml +++ b/tests/mcp/fixtures/expected.yaml @@ -18,33 +18,39 @@ counts: Function: 6 # entrypoint, service, db, BaseRepo.repo, UserRepo.repo, OrderRepo.repo # Named callers / callees (used by T5 — get_callers / get_callees). +# Verified against the live graph: assertions are exact, not "at least". calls: service: callers: ["entrypoint"] # service() instantiates UserRepo + OrderRepo and calls .repo() on each; - # the analyzer encodes that as CALLS edges on the method. - callees_any_of: ["repo", "UserRepo", "OrderRepo"] + # the analyzer emits CALLS edges to both the class constructors and + # the repo method (one per subclass, so `repo` appears twice). + callees: ["OrderRepo", "UserRepo", "repo", "repo"] entrypoint: callers: [] - callees_any_of: ["service"] + callees: ["service"] db: - # db() is called by both subclasses via super().repo() -> BaseRepo.repo(). - callers_any_of: ["repo"] + # db() is called from BaseRepo.repo (the subclasses delegate via super()). + callers: ["repo"] callees: [] -# Known path between two named symbols (used by T7 — find_path). +# Known paths between two named symbols (used by T7 — find_path). # Exact path count per (source, dest) pair. +# +# entrypoint -> service -> { UserRepo.repo, OrderRepo.repo } -> db +# yields two distinct paths through the class hierarchy. paths: - source: entrypoint dest: db - paths_count: 1 + paths_count: 2 # Prefix-search hits (used by T8 — search_code). +# Verified against api.auto_complete.prefix_search. search_prefixes: ent: must_include: ["entrypoint"] serv: must_include: ["service"] - Repo: - # case-insensitive prefix match on Searchable label - must_include_any_of: ["BaseRepo", "UserRepo", "OrderRepo"] + Base: + # Case-insensitive prefix match against Searchable.name. + must_include: ["BaseRepo"] diff --git a/tests/mcp/fixtures/sample_project/python/repo.py b/tests/mcp/fixtures/sample_project/python/repo.py index c9295ce0..69e4b61f 100644 --- a/tests/mcp/fixtures/sample_project/python/repo.py +++ b/tests/mcp/fixtures/sample_project/python/repo.py @@ -7,7 +7,7 @@ class BaseRepo: - """Base class so the analyzer emits an INHERITS edge.""" + """Base class so the analyzer emits an EXTENDS edge.""" def repo(self) -> str: return db() diff --git a/tests/mcp/test_fixture_contract.py b/tests/mcp/test_fixture_contract.py index 1d239eb5..0a95da43 100644 --- a/tests/mcp/test_fixture_contract.py +++ b/tests/mcp/test_fixture_contract.py @@ -9,10 +9,6 @@ from __future__ import annotations -from pathlib import Path - -import pytest - from tests.mcp.conftest import EXPECTED_PATH, SAMPLE_PROJECT @@ -37,6 +33,14 @@ def test_expected_contract_shape(expected_contract): calls = expected_contract["calls"] for sym in ("service", "entrypoint", "db"): assert sym in calls, f"calls.{sym} missing from expected.yaml" + # Each entry must declare both directions as exact lists. + for direction in ("callers", "callees"): + assert direction in calls[sym], ( + f"calls.{sym}.{direction} missing" + ) + assert isinstance(calls[sym][direction], list), ( + f"calls.{sym}.{direction} must be a list" + ) paths = expected_contract["paths"] assert isinstance(paths, list) and len(paths) == 1 @@ -47,6 +51,11 @@ def test_expected_contract_shape(expected_contract): prefixes = expected_contract["search_prefixes"] assert "ent" in prefixes + for key, spec in prefixes.items(): + assert "must_include" in spec, ( + f"search_prefixes.{key}.must_include missing" + ) + assert isinstance(spec["must_include"], list) and spec["must_include"] def test_sample_project_python_files_present(): @@ -80,3 +89,94 @@ def test_indexed_fixture_loads_exact_counts(indexed_fixture, expected_contract): assert actual == expected, ( f"label {label}: expected exactly {expected}, got {actual}" ) + + +def test_indexed_fixture_callers_and_callees(indexed_fixture, expected_contract): + """T3 acceptance: assert at least one named caller and one named callee + from the contract resolve against the indexed graph. + + Without this the ``calls`` section of ``expected.yaml`` would never be + exercised, and contract drift would silently break the T5 + (get_callers / get_callees) downstream test. + """ + + from api.graph import Graph + + g = Graph(indexed_fixture.project, branch=indexed_fixture.branch) + calls = expected_contract["calls"] + + def names(query: str, **params) -> list[str]: + rows = g.g.query(query, params).result_set + return sorted(r[0] for r in rows) + + for sym, spec in calls.items(): + expected_callers = sorted(spec["callers"]) + expected_callees = sorted(spec["callees"]) + + actual_callers = names( + "MATCH (s)-[:CALLS]->(d:Searchable {name:$n}) RETURN s.name", + n=sym, + ) + actual_callees = names( + "MATCH (s:Searchable {name:$n})-[:CALLS]->(d) RETURN d.name", + n=sym, + ) + + assert actual_callers == expected_callers, ( + f"calls.{sym}.callers: expected {expected_callers}, " + f"got {actual_callers}" + ) + assert actual_callees == expected_callees, ( + f"calls.{sym}.callees: expected {expected_callees}, " + f"got {actual_callees}" + ) + + +def test_indexed_fixture_paths(indexed_fixture, expected_contract): + """T3 acceptance: assert at least one named path from the contract + resolves against the indexed graph (used by T7 — find_path).""" + + from api.graph import Graph + + g = Graph(indexed_fixture.project, branch=indexed_fixture.branch) + + def id_for(name: str) -> int: + rows = g.g.query( + "MATCH (n:Searchable {name:$n}) RETURN ID(n) LIMIT 1", + {"n": name}, + ).result_set + assert rows, f"symbol {name!r} not found in indexed fixture" + return rows[0][0] + + for entry in expected_contract["paths"]: + src = id_for(entry["source"]) + dst = id_for(entry["dest"]) + paths = g.find_paths(src, dst) + assert len(paths) == entry["paths_count"], ( + f"paths {entry['source']!r} -> {entry['dest']!r}: " + f"expected exactly {entry['paths_count']}, got {len(paths)}" + ) + + +def test_indexed_fixture_prefix_search(indexed_fixture, expected_contract): + """T3 acceptance: assert at least one prefix-search hit from the + contract resolves against the indexed graph (used by T8 — + search_code).""" + + from api.auto_complete import prefix_search + + for prefix, spec in expected_contract["search_prefixes"].items(): + results = prefix_search( + indexed_fixture.project, + prefix, + branch=indexed_fixture.branch, + ) + names = { + (r.get("properties") or {}).get("name") + for r in results + } + for expected_name in spec["must_include"]: + assert expected_name in names, ( + f"prefix {prefix!r}: expected {expected_name!r} in " + f"results, got {sorted(n for n in names if n)}" + )