mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
test(task): DB-backed coverage for the PR-review lifecycle methods
Real-Postgres round-trips for the external/internal PR-review TaskService helpers that the existing mock tests can't prove actually persist: - ingest_external_pr — create-once, head-SHA dedup (unchanged head skips), and re-review on a new head; internal_pr source wording; - pr_review_claim / complete_review — the planless, branchless pending -> in_progress -> completed lifecycle, with re-claim / re-complete no-ops and the "complete requires in_progress" guard; - create_supersede_umbrella / find_supersede_umbrella — created on the same repo (not parented), idempotent lookup, non-review rejection, and the pr=5-vs-pr=50 exact-marker disambiguation; - list_external_pr_reviews — source isolation, the data-layer half of the dispatcher contract (regular tasks never leak into the review queue). Writes use flush (not commit) so the rollback-per-test fixture keeps each case isolated from the others against the shared session-scoped test DB.
This commit is contained in:
Renn F
@@ -0,0 +1,425 @@
|
||||
"""PR-review TaskService methods against a real Postgres DB.
|
||||
|
||||
The dedup/claim/complete/supersede helpers in TaskService run real SQL and
|
||||
real lifecycle transitions, so the mock-based tests in
|
||||
``test_external_pr_ingest.py`` can't prove they actually persist. These seed a
|
||||
project + reviewer agent in the test DB and exercise the full round-trip:
|
||||
|
||||
- ``ingest_external_pr`` creates one review task and de-dupes per head SHA;
|
||||
- ``pr_review_claim`` / ``complete_review`` drive the planless, branchless
|
||||
pending -> in_progress -> completed lifecycle the reviewer uses;
|
||||
- ``create_supersede_umbrella`` / ``find_supersede_umbrella`` create and
|
||||
idempotently locate the takeover coordination task;
|
||||
- ``list_external_pr_reviews`` isolates PR-review work from regular tasks —
|
||||
the same source filter the orchestrator's dispatchers key on.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from typing import TYPE_CHECKING
|
||||
from uuid import UUID, uuid4
|
||||
|
||||
import pytest
|
||||
from roboco.db.tables import AgentTable, ProjectTable
|
||||
from roboco.models.base import (
|
||||
AgentRole,
|
||||
AgentStatus,
|
||||
Complexity,
|
||||
TaskNature,
|
||||
TaskStatus,
|
||||
TaskType,
|
||||
Team,
|
||||
)
|
||||
from roboco.services.task import TaskCreateRequest, TaskService
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from sqlalchemy.ext.asyncio import AsyncSession
|
||||
|
||||
# PR numbers / counts used in the round-trips, named so comparisons aren't
|
||||
# "magic values" (ruff PLR2004) and the intent reads at the assertion site.
|
||||
EXTERNAL_PR = 170
|
||||
INTERNAL_PR = 42
|
||||
REVIEWS_AFTER_REREVIEW = 2
|
||||
|
||||
|
||||
async def _seed(session: AsyncSession) -> tuple[UUID, UUID, UUID]:
|
||||
"""Seed a system agent + pr_reviewer agent + project.
|
||||
|
||||
Returns ``(project_id, system_agent_id, reviewer_agent_id)`` — the FK
|
||||
targets every PR-review method needs (created_by, audit attribution,
|
||||
project the PR belongs to).
|
||||
"""
|
||||
system_agent = AgentTable(
|
||||
id=uuid4(),
|
||||
name="System",
|
||||
slug=f"system-{uuid4().hex[:8]}",
|
||||
role=AgentRole.SYSTEM,
|
||||
team=None,
|
||||
status=AgentStatus.ACTIVE,
|
||||
model_config={},
|
||||
system_prompt="system",
|
||||
capabilities=[],
|
||||
permissions={},
|
||||
metrics={},
|
||||
)
|
||||
session.add(system_agent)
|
||||
await session.flush()
|
||||
|
||||
project = ProjectTable(
|
||||
id=uuid4(),
|
||||
name="PR Review Test Project",
|
||||
slug=f"prreview-{uuid4().hex[:8]}",
|
||||
git_url="https://github.com/example/prreview.git",
|
||||
default_branch="main",
|
||||
protected_branches=["main"],
|
||||
assigned_cell=Team.BACKEND,
|
||||
created_by=system_agent.id,
|
||||
is_active=True,
|
||||
)
|
||||
session.add(project)
|
||||
await session.flush()
|
||||
|
||||
reviewer = AgentTable(
|
||||
id=uuid4(),
|
||||
name="PR Reviewer",
|
||||
slug=f"pr-reviewer-{uuid4().hex[:8]}",
|
||||
role=AgentRole.PR_REVIEWER,
|
||||
team=Team.SYSTEM,
|
||||
status=AgentStatus.ACTIVE,
|
||||
model_config={},
|
||||
system_prompt="reviewer",
|
||||
capabilities=["review"],
|
||||
permissions={},
|
||||
metrics={},
|
||||
)
|
||||
session.add(reviewer)
|
||||
await session.flush()
|
||||
|
||||
return (
|
||||
UUID(str(project.id)),
|
||||
UUID(str(system_agent.id)),
|
||||
UUID(str(reviewer.id)),
|
||||
)
|
||||
|
||||
|
||||
def _pr(number: int, head_sha: str, *, title: str = "Add feature") -> dict[str, object]:
|
||||
return {
|
||||
"number": number,
|
||||
"url": f"https://github.com/example/prreview/pull/{number}",
|
||||
"title": title,
|
||||
"head_sha": head_sha,
|
||||
}
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# ingest_external_pr — create-once + head-SHA dedup, persisted.
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_ingest_creates_review_task(db_session: AsyncSession) -> None:
|
||||
project_id, system_id, _ = await _seed(db_session)
|
||||
svc = TaskService(db_session)
|
||||
|
||||
task = await svc.ingest_external_pr(
|
||||
project_id=project_id,
|
||||
pr=_pr(EXTERNAL_PR, "abc123"),
|
||||
created_by=system_id,
|
||||
team=Team.SYSTEM,
|
||||
)
|
||||
await db_session.flush()
|
||||
|
||||
assert task is not None
|
||||
assert task.source == "external_pr"
|
||||
assert task.pr_number == EXTERNAL_PR
|
||||
assert task.task_type == TaskType.CODE
|
||||
assert task.confirmed_by_human is False
|
||||
assert task.status == TaskStatus.PENDING
|
||||
assert task.quick_context == "external_pr_head=abc123"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_ingest_dedups_same_head(db_session: AsyncSession) -> None:
|
||||
project_id, system_id, _ = await _seed(db_session)
|
||||
svc = TaskService(db_session)
|
||||
|
||||
first = await svc.ingest_external_pr(
|
||||
project_id=project_id,
|
||||
pr=_pr(EXTERNAL_PR, "abc123"),
|
||||
created_by=system_id,
|
||||
team=Team.SYSTEM,
|
||||
)
|
||||
await db_session.flush()
|
||||
second = await svc.ingest_external_pr(
|
||||
project_id=project_id,
|
||||
pr=_pr(EXTERNAL_PR, "abc123"),
|
||||
created_by=system_id,
|
||||
team=Team.SYSTEM,
|
||||
)
|
||||
await db_session.flush()
|
||||
|
||||
assert first is not None
|
||||
assert second is None # unchanged head -> skip
|
||||
assert (
|
||||
await svc.external_review_task_exists(project_id, EXTERNAL_PR, "abc123") is True
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_ingest_new_head_rereviews(db_session: AsyncSession) -> None:
|
||||
project_id, system_id, _ = await _seed(db_session)
|
||||
svc = TaskService(db_session)
|
||||
|
||||
await svc.ingest_external_pr(
|
||||
project_id=project_id,
|
||||
pr=_pr(EXTERNAL_PR, "abc123"),
|
||||
created_by=system_id,
|
||||
team=Team.SYSTEM,
|
||||
)
|
||||
await db_session.flush()
|
||||
# PR got new commits -> new head SHA -> a fresh review is opened.
|
||||
rereview = await svc.ingest_external_pr(
|
||||
project_id=project_id,
|
||||
pr=_pr(EXTERNAL_PR, "def456"),
|
||||
created_by=system_id,
|
||||
team=Team.SYSTEM,
|
||||
)
|
||||
await db_session.flush()
|
||||
|
||||
assert rereview is not None
|
||||
assert rereview.quick_context == "external_pr_head=def456"
|
||||
reviews = await svc.list_external_pr_reviews()
|
||||
matching = [t for t in reviews if t.pr_number == EXTERNAL_PR]
|
||||
assert len(matching) == REVIEWS_AFTER_REREVIEW
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_ingest_internal_pr_source(db_session: AsyncSession) -> None:
|
||||
project_id, system_id, _ = await _seed(db_session)
|
||||
svc = TaskService(db_session)
|
||||
|
||||
task = await svc.ingest_external_pr(
|
||||
project_id=project_id,
|
||||
pr=_pr(INTERNAL_PR, "aaa111"),
|
||||
created_by=system_id,
|
||||
team=Team.SYSTEM,
|
||||
source="internal_pr",
|
||||
)
|
||||
await db_session.flush()
|
||||
|
||||
assert task is not None
|
||||
assert task.source == "internal_pr"
|
||||
assert "internal PR #42" in task.title
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# pr_review_claim / complete_review — planless, branchless lifecycle.
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_pr_review_claim_and_complete(db_session: AsyncSession) -> None:
|
||||
project_id, system_id, reviewer_id = await _seed(db_session)
|
||||
svc = TaskService(db_session)
|
||||
|
||||
review = await svc.ingest_external_pr(
|
||||
project_id=project_id,
|
||||
pr=_pr(EXTERNAL_PR, "abc123"),
|
||||
created_by=system_id,
|
||||
team=Team.SYSTEM,
|
||||
)
|
||||
await db_session.flush()
|
||||
assert review is not None
|
||||
task_id = UUID(str(review.id))
|
||||
|
||||
# Claim: pending -> in_progress, no plan, no branch.
|
||||
claimed = await svc.pr_review_claim(reviewer_id, task_id)
|
||||
await db_session.flush()
|
||||
assert claimed is not None
|
||||
assert claimed.status == TaskStatus.IN_PROGRESS
|
||||
assert UUID(str(claimed.claimed_by)) == reviewer_id
|
||||
assert claimed.branch_name in (None, "")
|
||||
|
||||
# Re-claiming a non-pending task is a no-op.
|
||||
assert await svc.pr_review_claim(reviewer_id, task_id) is None
|
||||
|
||||
# Complete: in_progress -> completed, claim cleared, notes recorded.
|
||||
done = await svc.complete_review(reviewer_id, task_id, notes="Posted review.")
|
||||
await db_session.flush()
|
||||
assert done is not None
|
||||
assert done.status == TaskStatus.COMPLETED
|
||||
assert done.qa_notes == "Posted review."
|
||||
assert done.claimed_by is None
|
||||
|
||||
# Re-completing a completed task is a no-op.
|
||||
assert await svc.complete_review(reviewer_id, task_id) is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_complete_review_requires_in_progress(db_session: AsyncSession) -> None:
|
||||
project_id, system_id, reviewer_id = await _seed(db_session)
|
||||
svc = TaskService(db_session)
|
||||
|
||||
review = await svc.ingest_external_pr(
|
||||
project_id=project_id,
|
||||
pr=_pr(EXTERNAL_PR, "abc123"),
|
||||
created_by=system_id,
|
||||
team=Team.SYSTEM,
|
||||
)
|
||||
await db_session.flush()
|
||||
assert review is not None
|
||||
|
||||
# Still PENDING (never claimed) -> complete is rejected.
|
||||
assert await svc.complete_review(reviewer_id, UUID(str(review.id))) is None
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# create_supersede_umbrella / find_supersede_umbrella — idempotent takeover.
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_and_find_supersede_umbrella(db_session: AsyncSession) -> None:
|
||||
project_id, system_id, _ = await _seed(db_session)
|
||||
svc = TaskService(db_session)
|
||||
|
||||
review = await svc.ingest_external_pr(
|
||||
project_id=project_id,
|
||||
pr=_pr(EXTERNAL_PR, "abc123"),
|
||||
created_by=system_id,
|
||||
team=Team.SYSTEM,
|
||||
)
|
||||
await db_session.flush()
|
||||
assert review is not None
|
||||
review_id = UUID(str(review.id))
|
||||
|
||||
umbrella = await svc.create_supersede_umbrella(
|
||||
review_task_id=review_id,
|
||||
branch_name="feature/system/supersede-170",
|
||||
created_by=system_id,
|
||||
)
|
||||
await db_session.flush()
|
||||
assert umbrella is not None
|
||||
assert umbrella.source == "external_pr_supersede"
|
||||
assert umbrella.team == Team.MAIN_PM
|
||||
assert umbrella.branch_name == "feature/system/supersede-170"
|
||||
assert umbrella.confirmed_by_human is True
|
||||
|
||||
found = await svc.find_supersede_umbrella(project_id, EXTERNAL_PR)
|
||||
assert found is not None
|
||||
assert UUID(str(found.id)) == UUID(str(umbrella.id))
|
||||
# No umbrella for an unrelated PR.
|
||||
assert await svc.find_supersede_umbrella(project_id, 999) is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_supersede_rejects_non_review(db_session: AsyncSession) -> None:
|
||||
project_id, system_id, _ = await _seed(db_session)
|
||||
svc = TaskService(db_session)
|
||||
|
||||
# A regular (source='manual') task is not a PR review -> no umbrella.
|
||||
regular = await svc.create(
|
||||
TaskCreateRequest(
|
||||
title="Regular work",
|
||||
description="not a review",
|
||||
acceptance_criteria=["done"],
|
||||
team=Team.BACKEND,
|
||||
created_by=system_id,
|
||||
task_type=TaskType.CODE,
|
||||
nature=TaskNature.TECHNICAL,
|
||||
estimated_complexity=Complexity.LOW,
|
||||
project_id=project_id,
|
||||
)
|
||||
)
|
||||
await db_session.flush()
|
||||
|
||||
assert (
|
||||
await svc.create_supersede_umbrella(
|
||||
review_task_id=UUID(str(regular.id)),
|
||||
branch_name="feature/system/x",
|
||||
created_by=system_id,
|
||||
)
|
||||
is None
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_find_supersede_umbrella_no_prefix_false_match(
|
||||
db_session: AsyncSession,
|
||||
) -> None:
|
||||
"""``pr=5`` must not match the umbrella for ``pr=50`` (exact marker match)."""
|
||||
project_id, system_id, _ = await _seed(db_session)
|
||||
svc = TaskService(db_session)
|
||||
|
||||
for number, sha in ((5, "sha5"), (50, "sha50")):
|
||||
review = await svc.ingest_external_pr(
|
||||
project_id=project_id,
|
||||
pr=_pr(number, sha),
|
||||
created_by=system_id,
|
||||
team=Team.SYSTEM,
|
||||
)
|
||||
await db_session.flush()
|
||||
assert review is not None
|
||||
await svc.create_supersede_umbrella(
|
||||
review_task_id=UUID(str(review.id)),
|
||||
branch_name=f"feature/system/supersede-{number}",
|
||||
created_by=system_id,
|
||||
)
|
||||
await db_session.flush()
|
||||
|
||||
five = await svc.find_supersede_umbrella(project_id, 5)
|
||||
fifty = await svc.find_supersede_umbrella(project_id, 50)
|
||||
assert five is not None
|
||||
assert fifty is not None
|
||||
assert UUID(str(five.id)) != UUID(str(fifty.id))
|
||||
assert "pr=5 review=" in (five.quick_context or "")
|
||||
assert "pr=50 review=" in (fifty.quick_context or "")
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Source isolation — what the dispatchers key on.
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_review_listing_isolates_pr_sources(db_session: AsyncSession) -> None:
|
||||
"""list_external_pr_reviews returns only PR-review-sourced tasks.
|
||||
|
||||
The orchestrator's dev/PM dispatchers skip ``PR_REVIEW_SOURCES`` and only
|
||||
the pr_reviewer claims them; this pins the data-layer half of that contract
|
||||
— a regular task in the same project never leaks into the review queue.
|
||||
"""
|
||||
project_id, system_id, _ = await _seed(db_session)
|
||||
svc = TaskService(db_session)
|
||||
|
||||
await svc.create(
|
||||
TaskCreateRequest(
|
||||
title="Regular backend work",
|
||||
description="real delivery task",
|
||||
acceptance_criteria=["ships"],
|
||||
team=Team.BACKEND,
|
||||
created_by=system_id,
|
||||
task_type=TaskType.CODE,
|
||||
nature=TaskNature.TECHNICAL,
|
||||
estimated_complexity=Complexity.LOW,
|
||||
project_id=project_id,
|
||||
)
|
||||
)
|
||||
await svc.ingest_external_pr(
|
||||
project_id=project_id,
|
||||
pr=_pr(EXTERNAL_PR, "abc"),
|
||||
created_by=system_id,
|
||||
team=Team.SYSTEM,
|
||||
)
|
||||
await svc.ingest_external_pr(
|
||||
project_id=project_id,
|
||||
pr=_pr(INTERNAL_PR, "def"),
|
||||
created_by=system_id,
|
||||
team=Team.SYSTEM,
|
||||
source="internal_pr",
|
||||
)
|
||||
await db_session.flush()
|
||||
|
||||
reviews = await svc.list_external_pr_reviews()
|
||||
assert {t.source for t in reviews} == {"external_pr", "internal_pr"}
|
||||
assert {t.pr_number for t in reviews} == {EXTERNAL_PR, INTERNAL_PR}
|
||||
Reference in New Issue
Block a user