open-swe/tests/test_reviewer_groups.py
Adam Moussa 62d9945df4
refactor: consolidate reviewer modules into agent/review/
Part of the domain-reorg adoption (build plan step C2): fork content,
upstream layout. Nine 1:1 module moves (reviewer_diff/eval_store/
findings/groups/publish/reconcile/trace_context + review_style_
collector/guidance) into agent/review/, with internal relative
imports re-wired to the new package depth. agent/review/__init__.py
mirrors upstream's thin re-export shim (one of the 21 verified "A"
structural adds).

Rewrote the 38 grep hits across importer files (agent/{analyzer,
ci_autofix,reviewer,webapp}.py, agent/dashboard/*, agent/middleware/
settle_review_check.py, agent/tools/*, agent/utils/github_feedback.py,
agent/webhooks/github.py, evals/reviewer/*, and the reviewer test
suite) to point at agent.review.*; 4 of the 38 hits were name
collisions (list_reviewer_findings, reviewer_outcomes,
_reviewer_thread_id, reviewer_thread_id — not the moved modules) and
were left untouched. tests/test_github_checks.py's module-alias
import (`from agent import reviewer_publish`) follows upstream's own
`from agent.review import publish as reviewer_publish` pattern so
downstream `reviewer_publish.*` call sites needed no changes.
agent/reviewer.py and agent/webapp.py stay in place per the hard
rule (fork content, import-only rewire) and are not part of this
package.

Gates: ruff check + ruff format --check, pytest --co -q (1637
collected), full unit suite (1637 passed), and the reviewer/findings
suite in isolation (pytest -k "review or finding", 421 passed).
2026-07-17 13:52:03 -04:00

171 lines
5.3 KiB
Python

from __future__ import annotations
from typing import Any
from unittest.mock import AsyncMock, MagicMock, patch
import pytest
from agent.review.groups import (
_build_prompt,
_DiffGroupingResult,
_DiffGroupModel,
diff_signature,
generate_diff_groups,
maybe_generate_and_store_diff_groups,
)
_DIFF = """diff --git a/foo.py b/foo.py
--- a/foo.py
+++ b/foo.py
@@ -1,2 +1,3 @@
a
+b
c
diff --git a/bar.py b/bar.py
--- a/bar.py
+++ b/bar.py
@@ -1 +1,2 @@
x
+y
"""
class _FakeStructured:
def __init__(self, result: Any, *, raises: bool = False) -> None:
self._result = result
self._raises = raises
async def ainvoke(self, _prompt: str) -> Any:
if self._raises:
raise RuntimeError("boom")
return self._result
class _FakeModel:
def __init__(self, result: Any, *, raises: bool = False) -> None:
self._structured = _FakeStructured(result, raises=raises)
def with_structured_output(self, _schema: Any) -> _FakeStructured:
return self._structured
@pytest.mark.asyncio
async def test_generate_diff_groups_partitions() -> None:
result = _DiffGroupingResult(
groups=[
_DiffGroupModel(title="Foo change", summary="Edits foo", files=["foo.py"]),
_DiffGroupModel(title="Bar change", summary="Edits bar", files=["bar.py"]),
]
)
groups = await generate_diff_groups(diff_text=_DIFF, model=_FakeModel(result))
assert groups == [
{"title": "Foo change", "summary": "Edits foo", "files": ["foo.py"]},
{"title": "Bar change", "summary": "Edits bar", "files": ["bar.py"]},
]
@pytest.mark.asyncio
async def test_generate_diff_groups_dedupes_and_drops_unknown() -> None:
result = _DiffGroupingResult(
groups=[
_DiffGroupModel(
title="First",
summary="",
files=["foo.py", "foo.py", "does-not-exist.py"],
),
_DiffGroupModel(title="Second", summary="", files=["foo.py", "bar.py"]),
_DiffGroupModel(title="", summary="ignored", files=["bar.py"]),
]
)
groups = await generate_diff_groups(diff_text=_DIFF, model=_FakeModel(result))
# foo.py only in the first group, bar.py only in the second; the untitled
# group and the unknown path are dropped.
assert groups == [
{"title": "First", "summary": "", "files": ["foo.py"]},
{"title": "Second", "summary": "", "files": ["bar.py"]},
]
@pytest.mark.asyncio
async def test_generate_diff_groups_empty_diff_returns_empty() -> None:
assert await generate_diff_groups(diff_text="", model=_FakeModel(None)) == []
@pytest.mark.asyncio
async def test_generate_diff_groups_llm_failure_returns_none() -> None:
groups = await generate_diff_groups(diff_text=_DIFF, model=_FakeModel(None, raises=True))
assert groups is None
def test_build_prompt_includes_files_and_diffs() -> None:
prompt = _build_prompt(_DIFF, ["foo.py", "bar.py"])
assert "### foo.py" in prompt
assert "### bar.py" in prompt
# Hunks are rendered as plain diff fences (no line-range annotations).
assert "lines 1-3:" not in prompt
assert "```diff" in prompt
# The verbatim file list is included so every file can be assigned.
assert "- foo.py" in prompt
assert "- bar.py" in prompt
def test_diff_signature_is_stable_and_content_sensitive() -> None:
assert diff_signature(_DIFF) == diff_signature(_DIFF)
assert diff_signature(_DIFF) != diff_signature(_DIFF + "\n+extra")
@pytest.mark.asyncio
async def test_maybe_generate_skips_when_signature_unchanged() -> None:
existing = {
"diff_groups": {
"signature": diff_signature(_DIFF),
"groups": [{"title": "t", "summary": "", "files": ["foo.py"]}],
}
}
with (
patch(
"agent.review.groups.get_thread_metadata",
new_callable=AsyncMock,
return_value=existing,
),
patch("agent.review.groups.generate_diff_groups", new_callable=AsyncMock) as gen,
patch(
"agent.review.groups.set_reviewer_thread_metadata",
new_callable=AsyncMock,
) as set_meta,
):
await maybe_generate_and_store_diff_groups(
thread_id="t1", head_sha="h", diff_text=_DIFF, model=MagicMock()
)
gen.assert_not_awaited()
set_meta.assert_not_awaited()
@pytest.mark.asyncio
async def test_maybe_generate_stores_when_changed() -> None:
groups = [{"title": "t", "summary": "s", "files": ["foo.py"]}]
with (
patch(
"agent.review.groups.get_thread_metadata",
new_callable=AsyncMock,
return_value={},
),
patch(
"agent.review.groups.generate_diff_groups",
new_callable=AsyncMock,
return_value=groups,
) as gen,
patch(
"agent.review.groups.set_reviewer_thread_metadata",
new_callable=AsyncMock,
) as set_meta,
):
await maybe_generate_and_store_diff_groups(
thread_id="t1", head_sha="head123", diff_text=_DIFF, model=MagicMock()
)
gen.assert_awaited_once()
set_meta.assert_awaited_once()
payload = set_meta.call_args.kwargs["extra"]["diff_groups"]
assert payload["head_sha"] == "head123"
assert payload["signature"] == diff_signature(_DIFF)
assert payload["groups"] == groups