Skip to content

Commit 1f79635

Browse files
fix(graph): correct TESTED_BY edge direction in tests_for queries
The parser stores TESTED_BY edges as source=production, target=test, but query_graph(pattern="tests_for"), get_transitive_tests, and load_flow_adjacency all traversed them in the wrong direction. The naming-convention fallback (test_<name>/Test<name>) silently masked the bug for tests following standard naming. Changes: - tools/query.py: flip tests_for branch to use get_edges_by_source + e.target_qualified - graph.py::get_transitive_tests: flip three SQL queries to select target_qualified where source_qualified = ? - graph.py::load_flow_adjacency: track production node (src) in has_tested_by so criticality scoring reflects real coverage Regression tests use unconventional test names so the naming-convention fallback cannot mask future regressions. Fixes #515
1 parent 0c9a5ff commit 1f79635

4 files changed

Lines changed: 155 additions & 24 deletions

File tree

code_review_graph/graph.py

Lines changed: 25 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -371,7 +371,10 @@ def get_transitive_tests(
371371
) -> list[dict]:
372372
"""Find tests covering a node, including indirect (transitive) coverage.
373373
374-
1. Direct: TESTED_BY edges targeting this node (+ bare-name fallback).
374+
TESTED_BY edges are stored as source=production, target=test by
375+
the parser, so look them up by source_qualified. See: #515
376+
377+
1. Direct: TESTED_BY edges originating at this node (+ bare-name fallback).
375378
2. Indirect: follow outgoing CALLS edges up to *max_depth* hops,
376379
then collect TESTED_BY edges on each callee.
377380
@@ -415,31 +418,31 @@ def _node_dict(qn: str, indirect: bool) -> dict | None:
415418
"indirect": indirect,
416419
}
417420

418-
# Direct TESTED_BY
421+
# Direct TESTED_BY (source=production, target=test). See: #515
419422
for qn in input_qns:
420423
for row in conn.execute(
421-
"SELECT source_qualified FROM edges "
422-
"WHERE target_qualified = ? AND kind = 'TESTED_BY'",
424+
"SELECT target_qualified FROM edges "
425+
"WHERE source_qualified = ? AND kind = 'TESTED_BY'",
423426
(qn,),
424427
).fetchall():
425-
src = row["source_qualified"]
426-
if src not in seen:
427-
seen.add(src)
428-
d = _node_dict(src, indirect=False)
428+
tgt = row["target_qualified"]
429+
if tgt not in seen:
430+
seen.add(tgt)
431+
d = _node_dict(tgt, indirect=False)
429432
if d:
430433
results.append(d)
431434

432435
# Bare-name fallback for direct
433436
bare = qualified_name.rsplit("::", 1)[-1] if "::" in qualified_name else qualified_name
434437
for row in conn.execute(
435-
"SELECT source_qualified FROM edges "
436-
"WHERE target_qualified = ? AND kind = 'TESTED_BY'",
438+
"SELECT target_qualified FROM edges "
439+
"WHERE source_qualified = ? AND kind = 'TESTED_BY'",
437440
(bare,),
438441
).fetchall():
439-
src = row["source_qualified"]
440-
if src not in seen:
441-
seen.add(src)
442-
d = _node_dict(src, indirect=False)
442+
tgt = row["target_qualified"]
443+
if tgt not in seen:
444+
seen.add(tgt)
445+
d = _node_dict(tgt, indirect=False)
443446
if d:
444447
results.append(d)
445448

@@ -458,14 +461,14 @@ def _node_dict(qn: str, indirect: bool) -> dict | None:
458461
next_frontier = set(list(next_frontier)[:max_frontier])
459462
for callee in next_frontier:
460463
for row in conn.execute(
461-
"SELECT source_qualified FROM edges "
462-
"WHERE target_qualified = ? AND kind = 'TESTED_BY'",
464+
"SELECT target_qualified FROM edges "
465+
"WHERE source_qualified = ? AND kind = 'TESTED_BY'",
463466
(callee,),
464467
).fetchall():
465-
src = row["source_qualified"]
466-
if src not in seen:
467-
seen.add(src)
468-
d = _node_dict(src, indirect=True)
468+
tgt = row["target_qualified"]
469+
if tgt not in seen:
470+
seen.add(tgt)
471+
d = _node_dict(tgt, indirect=True)
469472
if d:
470473
results.append(d)
471474
frontier = next_frontier
@@ -1263,8 +1266,8 @@ def load_flow_adjacency(self) -> "FlowAdjacency":
12631266
kind, src, tgt = row["kind"], row["source_qualified"], row["target_qualified"]
12641267
if kind == "CALLS":
12651268
calls_out.setdefault(src, []).append(tgt)
1266-
else: # TESTED_BY
1267-
has_tested_by.add(tgt)
1269+
else: # TESTED_BY: source is the production node being tested. See: #515
1270+
has_tested_by.add(src)
12681271

12691272
return FlowAdjacency(
12701273
calls_out=calls_out,

code_review_graph/tools/query.py

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -293,9 +293,11 @@ def query_graph(
293293
results.append(node_to_dict(child))
294294

295295
elif pattern == "tests_for":
296-
for e in store.get_edges_by_target(qn):
296+
# TESTED_BY edges are stored as source=production, target=test
297+
# by the parser, so look them up by source. See: #515
298+
for e in store.get_edges_by_source(qn):
297299
if e.kind == "TESTED_BY":
298-
test = store.get_node(e.source_qualified)
300+
test = store.get_node(e.target_qualified)
299301
if test:
300302
results.append(node_to_dict(test))
301303
# Also search by naming convention

tests/test_graph.py

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -226,6 +226,66 @@ def test_metadata(self):
226226
assert self.store.get_metadata("test_key") == "test_value"
227227
assert self.store.get_metadata("nonexistent") is None
228228

229+
def test_get_transitive_tests_follows_direct_tested_by_edge(self):
230+
"""Regression test for #515: get_transitive_tests must follow
231+
TESTED_BY edges by source_qualified (production) since the parser
232+
stores source=production, target=test. The test function uses an
233+
unconventional name so the bare-name fallback cannot mask the bug.
234+
"""
235+
self.store.upsert_node(self._make_file_node("/src/calc.py"))
236+
self.store.upsert_node(self._make_func_node("add", "/src/calc.py"))
237+
self.store.upsert_node(self._make_file_node("/tests/check.py"))
238+
self.store.upsert_node(self._make_func_node(
239+
"verify_addition", "/tests/check.py", is_test=True,
240+
))
241+
self.store.upsert_edge(EdgeInfo(
242+
kind="TESTED_BY",
243+
source="/src/calc.py::add",
244+
target="/tests/check.py::verify_addition",
245+
file_path="/tests/check.py", line=1,
246+
))
247+
self.store.commit()
248+
249+
results = self.store.get_transitive_tests("/src/calc.py::add")
250+
qns = {r["qualified_name"] for r in results}
251+
assert "/tests/check.py::verify_addition" in qns
252+
assert all(not r["indirect"] for r in results)
253+
254+
def test_get_transitive_tests_follows_calls_then_tested_by(self):
255+
"""Transitive coverage: caller -> CALLS -> callee -> TESTED_BY -> test.
256+
Uses an unconventional test name so the bare-name fallback cannot
257+
match. See: #515.
258+
"""
259+
self.store.upsert_node(self._make_file_node("/src/svc.py"))
260+
self.store.upsert_node(self._make_func_node("orchestrate", "/src/svc.py"))
261+
self.store.upsert_node(self._make_func_node("compute", "/src/svc.py"))
262+
self.store.upsert_node(self._make_file_node("/tests/check.py"))
263+
self.store.upsert_node(self._make_func_node(
264+
"verify_compute", "/tests/check.py", is_test=True,
265+
))
266+
self.store.upsert_edge(EdgeInfo(
267+
kind="CALLS", source="/src/svc.py::orchestrate",
268+
target="/src/svc.py::compute", file_path="/src/svc.py", line=2,
269+
))
270+
self.store.upsert_edge(EdgeInfo(
271+
kind="TESTED_BY",
272+
source="/src/svc.py::compute",
273+
target="/tests/check.py::verify_compute",
274+
file_path="/tests/check.py", line=1,
275+
))
276+
self.store.commit()
277+
278+
results = self.store.get_transitive_tests(
279+
"/src/svc.py::orchestrate", max_depth=2,
280+
)
281+
qns = {r["qualified_name"] for r in results}
282+
assert "/tests/check.py::verify_compute" in qns
283+
match = next(
284+
r for r in results
285+
if r["qualified_name"] == "/tests/check.py::verify_compute"
286+
)
287+
assert match["indirect"] is True
288+
229289
def test_get_all_community_ids_logs_when_column_missing(self, caplog):
230290
conn = sqlite3.connect(":memory:")
231291
conn.row_factory = sqlite3.Row

tests/test_tools.py

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -366,6 +366,72 @@ def test_validate_repo_root_error_mentions_svn_marker(self, tmp_path):
366366
_validate_repo_root(tmp_path)
367367

368368

369+
class TestQueryGraphTestsFor:
370+
"""Regression tests for #515: query_graph(pattern='tests_for')
371+
must follow direct TESTED_BY edges (source=production, target=test)
372+
rather than relying on the naming-convention fallback.
373+
"""
374+
375+
def setup_method(self, tmp_path_factory=None):
376+
# tmp_path_factory is unavailable in setup_method; use a TemporaryDirectory.
377+
import tempfile as _tempfile
378+
self._tmpdir = _tempfile.TemporaryDirectory()
379+
self.repo_root = Path(self._tmpdir.name)
380+
# _validate_repo_root requires .git or .code-review-graph.
381+
(self.repo_root / ".code-review-graph").mkdir()
382+
# find_project_root / get_db_path look here for the DB.
383+
from code_review_graph.incremental import get_db_path
384+
self.db_path = get_db_path(self.repo_root)
385+
self.store = GraphStore(str(self.db_path))
386+
self._seed_graph()
387+
388+
def teardown_method(self):
389+
self.store.close()
390+
self._tmpdir.cleanup()
391+
392+
def _seed_graph(self):
393+
# Production function with an unconventional name so the
394+
# naming-convention fallback (test_<name> / Test<name>) cannot match.
395+
self.store.upsert_node(NodeInfo(
396+
kind="File", name="/src/calc.py", file_path="/src/calc.py",
397+
line_start=1, line_end=20, language="python",
398+
))
399+
self.store.upsert_node(NodeInfo(
400+
kind="Function", name="combine", file_path="/src/calc.py",
401+
line_start=1, line_end=5, language="python",
402+
))
403+
self.store.upsert_node(NodeInfo(
404+
kind="File", name="/tests/spec.py", file_path="/tests/spec.py",
405+
line_start=1, line_end=20, language="python",
406+
))
407+
self.store.upsert_node(NodeInfo(
408+
kind="Test", name="verify_combine_behaviour",
409+
file_path="/tests/spec.py",
410+
line_start=1, line_end=5, language="python", is_test=True,
411+
))
412+
# Parser-canonical direction: source=production, target=test.
413+
self.store.upsert_edge(EdgeInfo(
414+
kind="TESTED_BY",
415+
source="/src/calc.py::combine",
416+
target="/tests/spec.py::verify_combine_behaviour",
417+
file_path="/tests/spec.py", line=1,
418+
))
419+
self.store.commit()
420+
# Release the writer connection so query_graph can open its own.
421+
self.store.close()
422+
423+
def test_query_graph_tests_for_finds_direct_edge(self):
424+
from code_review_graph.tools import query_graph
425+
result = query_graph(
426+
pattern="tests_for",
427+
target="/src/calc.py::combine",
428+
repo_root=str(self.repo_root),
429+
)
430+
assert result["status"] == "ok"
431+
qns = {r["qualified_name"] for r in result["results"]}
432+
assert "/tests/spec.py::verify_combine_behaviour" in qns
433+
434+
369435
class TestGetDocsSection:
370436
"""Tests for the get_docs_section tool."""
371437

0 commit comments

Comments
 (0)