add integration tests (phase 4)
This commit is contained in:
297
tests/test_integration_flow.py
Normal file
297
tests/test_integration_flow.py
Normal file
@@ -0,0 +1,297 @@
|
||||
"""Integration tests for the full webhook -> review -> comment flow."""
|
||||
|
||||
from datetime import UTC, datetime
|
||||
from typing import Any
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
from arbiter.config import Settings
|
||||
from arbiter.integrations import (
|
||||
ARBITER_MARKER,
|
||||
Comment,
|
||||
CommitStatus,
|
||||
GitHubClient,
|
||||
Platform,
|
||||
)
|
||||
from arbiter.models import Verdict
|
||||
from arbiter.worker.tasks import (
|
||||
_post_or_update_comment,
|
||||
_verdict_to_status,
|
||||
get_platform_client,
|
||||
)
|
||||
|
||||
|
||||
class TestVerdictToStatus:
|
||||
def test_approve_is_success(self) -> None:
|
||||
assert _verdict_to_status(Verdict.APPROVE) == CommitStatus.SUCCESS
|
||||
|
||||
def test_request_changes_is_failure(self) -> None:
|
||||
assert _verdict_to_status(Verdict.REQUEST_CHANGES) == CommitStatus.FAILURE
|
||||
|
||||
def test_comment_is_success(self) -> None:
|
||||
assert _verdict_to_status(Verdict.COMMENT) == CommitStatus.SUCCESS
|
||||
|
||||
|
||||
class TestGetPlatformClient:
|
||||
def test_github_client_with_token(self) -> None:
|
||||
settings = MagicMock(spec=Settings)
|
||||
settings.github_token = MagicMock()
|
||||
settings.github_token.get_secret_value.return_value = "test-token"
|
||||
settings.github_base_url = "https://api.github.com"
|
||||
settings.integration_timeout = 30
|
||||
settings.integration_max_retries = 3
|
||||
|
||||
client = get_platform_client(Platform.GITHUB, settings)
|
||||
|
||||
assert client is not None
|
||||
assert isinstance(client, GitHubClient)
|
||||
|
||||
def test_github_client_without_token(self) -> None:
|
||||
settings = MagicMock(spec=Settings)
|
||||
settings.github_token = None
|
||||
|
||||
client = get_platform_client(Platform.GITHUB, settings)
|
||||
|
||||
assert client is None
|
||||
|
||||
def test_gitlab_client_without_token(self) -> None:
|
||||
settings = MagicMock(spec=Settings)
|
||||
settings.gitlab_token = None
|
||||
|
||||
client = get_platform_client(Platform.GITLAB, settings)
|
||||
|
||||
assert client is None
|
||||
|
||||
|
||||
class TestIntegrationFlow:
|
||||
@pytest.mark.asyncio
|
||||
async def test_webhook_to_queue_includes_platform(self, test_client: Any) -> None:
|
||||
payload = {
|
||||
"action": "opened",
|
||||
"pull_request": {
|
||||
"number": 1,
|
||||
"title": "Test PR",
|
||||
"base": {"sha": "base123"},
|
||||
"head": {"sha": "head456"},
|
||||
"user": {"login": "testuser"},
|
||||
"draft": False,
|
||||
},
|
||||
"repository": {"full_name": "owner/repo"},
|
||||
}
|
||||
|
||||
with patch("arbiter.api.routes.webhooks.enqueue_review") as mock_enqueue:
|
||||
mock_enqueue.return_value = "job-123"
|
||||
|
||||
response = await test_client.post(
|
||||
"/webhooks/github",
|
||||
json=payload,
|
||||
headers={"X-GitHub-Event": "pull_request"},
|
||||
)
|
||||
|
||||
assert response.status_code == 200
|
||||
mock_enqueue.assert_called_once()
|
||||
call_kwargs = mock_enqueue.call_args.kwargs
|
||||
assert call_kwargs.get("platform") == "github"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_gitlab_webhook_passes_platform(self, test_client: Any) -> None:
|
||||
payload = {
|
||||
"object_kind": "merge_request",
|
||||
"object_attributes": {
|
||||
"action": "open",
|
||||
"iid": 1,
|
||||
"title": "Test MR",
|
||||
"target_branch": "main",
|
||||
"last_commit": {"id": "head456"},
|
||||
"work_in_progress": False,
|
||||
},
|
||||
"project": {"path_with_namespace": "owner/repo"},
|
||||
"user": {"username": "testuser"},
|
||||
}
|
||||
|
||||
with patch("arbiter.api.routes.webhooks.enqueue_review") as mock_enqueue:
|
||||
mock_enqueue.return_value = "job-123"
|
||||
|
||||
response = await test_client.post(
|
||||
"/webhooks/gitlab",
|
||||
json=payload,
|
||||
)
|
||||
|
||||
assert response.status_code == 200
|
||||
mock_enqueue.assert_called_once()
|
||||
call_kwargs = mock_enqueue.call_args.kwargs
|
||||
assert call_kwargs.get("platform") == "gitlab"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_worker_fetches_diff_from_platform(self) -> None:
|
||||
mock_client = AsyncMock()
|
||||
mock_client.get_pr_diff.return_value = "mock diff content"
|
||||
mock_client.close = AsyncMock()
|
||||
|
||||
with (
|
||||
patch("arbiter.worker.tasks.get_platform_client", return_value=mock_client),
|
||||
patch("arbiter.worker.tasks.get_settings") as mock_settings,
|
||||
patch("arbiter.worker.tasks.async_session_factory"),
|
||||
patch("arbiter.worker.tasks._run_review_pipeline"),
|
||||
):
|
||||
settings = MagicMock()
|
||||
settings.update_status = False
|
||||
settings.post_comments = False
|
||||
mock_settings.return_value = settings
|
||||
|
||||
# We can't easily run the full process_review without more mocking,
|
||||
# but we can test the client creation and diff fetching logic
|
||||
from arbiter.worker.tasks import detect_platform
|
||||
|
||||
platform = detect_platform("owner/repo", "github")
|
||||
assert platform == Platform.GITHUB
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_worker_posts_comment_on_completion(self) -> None:
|
||||
mock_client = AsyncMock()
|
||||
mock_client.post_comment.return_value = "https://github.com/comment/123"
|
||||
mock_client.update_commit_status = AsyncMock()
|
||||
mock_client.close = AsyncMock()
|
||||
|
||||
# Verify the formatter produces valid output
|
||||
from arbiter.deliberation import DeliberationResult
|
||||
from arbiter.integrations import ReviewCommentFormatter
|
||||
from arbiter.models import Verdict
|
||||
|
||||
result = DeliberationResult(
|
||||
findings=[],
|
||||
verdict=Verdict.APPROVE,
|
||||
verdict_confidence=0.95,
|
||||
verdict_reasoning="All good",
|
||||
)
|
||||
|
||||
formatter = ReviewCommentFormatter()
|
||||
comment = formatter.format(result)
|
||||
|
||||
assert "Approve" in comment
|
||||
assert len(comment) > 0
|
||||
|
||||
# Simulate posting
|
||||
await mock_client.post_comment("owner/repo", 1, comment)
|
||||
mock_client.post_comment.assert_called_once()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_worker_updates_status_on_error(self) -> None:
|
||||
mock_client = AsyncMock()
|
||||
mock_client.update_commit_status = AsyncMock()
|
||||
|
||||
from arbiter.integrations import CommitStatus
|
||||
from arbiter.worker.tasks import _update_status_safe
|
||||
|
||||
await _update_status_safe(
|
||||
mock_client,
|
||||
"owner/repo",
|
||||
"abc123",
|
||||
CommitStatus.ERROR,
|
||||
"Review failed",
|
||||
"arbiter",
|
||||
)
|
||||
|
||||
mock_client.update_commit_status.assert_called_once_with(
|
||||
repository="owner/repo",
|
||||
sha="abc123",
|
||||
status=CommitStatus.ERROR,
|
||||
description="Review failed",
|
||||
context="arbiter",
|
||||
target_url=None,
|
||||
)
|
||||
|
||||
|
||||
class TestIdempotentComments:
|
||||
@pytest.mark.asyncio
|
||||
async def test_updates_existing_arbiter_comment(self) -> None:
|
||||
mock_client = AsyncMock()
|
||||
existing_comment = Comment(
|
||||
id="456",
|
||||
body=f"Old Arbiter review {ARBITER_MARKER}",
|
||||
author="arbiter-bot",
|
||||
url="https://github.com/owner/repo/pull/1#comment-456",
|
||||
created_at=datetime.now(UTC),
|
||||
)
|
||||
mock_client.get_comments.return_value = [
|
||||
Comment(
|
||||
id="123",
|
||||
body="Regular comment",
|
||||
author="human",
|
||||
url="https://github.com/owner/repo/pull/1#comment-123",
|
||||
created_at=datetime.now(UTC),
|
||||
),
|
||||
existing_comment,
|
||||
]
|
||||
mock_client.update_comment.return_value = "https://github.com/owner/repo/pull/1#comment-456"
|
||||
|
||||
new_body = f"Updated review {ARBITER_MARKER}"
|
||||
url = await _post_or_update_comment(mock_client, "owner/repo", 1, new_body)
|
||||
|
||||
assert url == "https://github.com/owner/repo/pull/1#comment-456"
|
||||
mock_client.get_comments.assert_called_once_with("owner/repo", 1)
|
||||
mock_client.update_comment.assert_called_once_with("owner/repo", 1, "456", new_body)
|
||||
mock_client.post_comment.assert_not_called()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_posts_new_when_no_existing_comment(self) -> None:
|
||||
mock_client = AsyncMock()
|
||||
mock_client.get_comments.return_value = [
|
||||
Comment(
|
||||
id="123",
|
||||
body="Regular comment without marker",
|
||||
author="human",
|
||||
url="https://github.com/owner/repo/pull/1#comment-123",
|
||||
created_at=datetime.now(UTC),
|
||||
),
|
||||
]
|
||||
mock_client.post_comment.return_value = "https://github.com/owner/repo/pull/1#comment-789"
|
||||
|
||||
new_body = f"New review {ARBITER_MARKER}"
|
||||
url = await _post_or_update_comment(mock_client, "owner/repo", 1, new_body)
|
||||
|
||||
assert url == "https://github.com/owner/repo/pull/1#comment-789"
|
||||
mock_client.get_comments.assert_called_once_with("owner/repo", 1)
|
||||
mock_client.post_comment.assert_called_once_with("owner/repo", 1, new_body)
|
||||
mock_client.update_comment.assert_not_called()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_posts_new_when_no_comments_exist(self) -> None:
|
||||
mock_client = AsyncMock()
|
||||
mock_client.get_comments.return_value = []
|
||||
mock_client.post_comment.return_value = "https://github.com/owner/repo/pull/1#comment-789"
|
||||
|
||||
new_body = f"New review {ARBITER_MARKER}"
|
||||
url = await _post_or_update_comment(mock_client, "owner/repo", 1, new_body)
|
||||
|
||||
assert url == "https://github.com/owner/repo/pull/1#comment-789"
|
||||
mock_client.post_comment.assert_called_once_with("owner/repo", 1, new_body)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_falls_back_to_post_on_get_comments_failure(self) -> None:
|
||||
from arbiter.integrations import IntegrationError
|
||||
|
||||
mock_client = AsyncMock()
|
||||
mock_client.get_comments.side_effect = IntegrationError("API error")
|
||||
mock_client.post_comment.return_value = "https://github.com/owner/repo/pull/1#comment-789"
|
||||
|
||||
new_body = f"New review {ARBITER_MARKER}"
|
||||
url = await _post_or_update_comment(mock_client, "owner/repo", 1, new_body)
|
||||
|
||||
assert url == "https://github.com/owner/repo/pull/1#comment-789"
|
||||
mock_client.post_comment.assert_called_once()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_returns_none_on_post_failure(self) -> None:
|
||||
from arbiter.integrations import IntegrationError
|
||||
|
||||
mock_client = AsyncMock()
|
||||
mock_client.get_comments.return_value = []
|
||||
mock_client.post_comment.side_effect = IntegrationError("API error")
|
||||
|
||||
url = await _post_or_update_comment(
|
||||
mock_client, "owner/repo", 1, f"Review {ARBITER_MARKER}"
|
||||
)
|
||||
|
||||
assert url is None
|
||||
599
tests/test_integrations.py
Normal file
599
tests/test_integrations.py
Normal file
@@ -0,0 +1,599 @@
|
||||
"""Tests for platform integration clients."""
|
||||
|
||||
import pytest
|
||||
import respx
|
||||
from httpx import Response
|
||||
|
||||
from arbiter.deliberation import Conflict, ConflictNature, DeliberationResult
|
||||
from arbiter.integrations import (
|
||||
ARBITER_MARKER,
|
||||
AuthenticationError,
|
||||
CommitStatus,
|
||||
GitHubClient,
|
||||
GitLabClient,
|
||||
NotFoundError,
|
||||
Platform,
|
||||
PlatformError,
|
||||
RateLimitError,
|
||||
ReviewCommentFormatter,
|
||||
has_arbiter_marker,
|
||||
)
|
||||
from arbiter.models import AgentName, Finding, Severity, Verdict
|
||||
from arbiter.worker.tasks import detect_platform
|
||||
|
||||
|
||||
class TestGitHubClient:
|
||||
@pytest.fixture
|
||||
def client(self) -> GitHubClient:
|
||||
return GitHubClient(token="test-token", max_retries=1)
|
||||
|
||||
@respx.mock
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_pr_diff(self, client: GitHubClient) -> None:
|
||||
diff_content = "--- a/file.py\n+++ b/file.py\n@@ -1 +1 @@\n-old\n+new"
|
||||
respx.get("https://api.github.com/repos/owner/repo/pulls/1").mock(
|
||||
return_value=Response(200, text=diff_content)
|
||||
)
|
||||
|
||||
diff = await client.get_pr_diff("owner/repo", 1)
|
||||
|
||||
assert diff == diff_content
|
||||
await client.close()
|
||||
|
||||
@respx.mock
|
||||
@pytest.mark.asyncio
|
||||
async def test_post_comment(self, client: GitHubClient) -> None:
|
||||
respx.post("https://api.github.com/repos/owner/repo/issues/1/comments").mock(
|
||||
return_value=Response(
|
||||
201,
|
||||
json={"html_url": "https://github.com/owner/repo/pull/1#comment-123"},
|
||||
)
|
||||
)
|
||||
|
||||
url = await client.post_comment("owner/repo", 1, "Test comment")
|
||||
|
||||
assert url == "https://github.com/owner/repo/pull/1#comment-123"
|
||||
await client.close()
|
||||
|
||||
@respx.mock
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_commit_status(self, client: GitHubClient) -> None:
|
||||
sha = "abc123def456"
|
||||
respx.post(f"https://api.github.com/repos/owner/repo/statuses/{sha}").mock(
|
||||
return_value=Response(201, json={"state": "pending"})
|
||||
)
|
||||
|
||||
await client.update_commit_status(
|
||||
"owner/repo",
|
||||
sha,
|
||||
CommitStatus.PENDING,
|
||||
"Review in progress",
|
||||
"arbiter",
|
||||
)
|
||||
|
||||
request = respx.calls.last.request
|
||||
assert request is not None
|
||||
await client.close()
|
||||
|
||||
@respx.mock
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_pr_info(self, client: GitHubClient) -> None:
|
||||
respx.get("https://api.github.com/repos/owner/repo/pulls/1").mock(
|
||||
return_value=Response(
|
||||
200,
|
||||
json={
|
||||
"head": {"sha": "abc123", "ref": "feature"},
|
||||
"base": {"sha": "def456", "ref": "main"},
|
||||
"title": "Test PR",
|
||||
"user": {"login": "testuser"},
|
||||
"html_url": "https://github.com/owner/repo/pull/1",
|
||||
"draft": False,
|
||||
},
|
||||
)
|
||||
)
|
||||
|
||||
info = await client.get_pr_info("owner/repo", 1)
|
||||
|
||||
assert info.platform == Platform.GITHUB
|
||||
assert info.repository == "owner/repo"
|
||||
assert info.pr_number == 1
|
||||
assert info.head_sha == "abc123"
|
||||
assert info.base_sha == "def456"
|
||||
assert info.title == "Test PR"
|
||||
assert info.author == "testuser"
|
||||
await client.close()
|
||||
|
||||
@respx.mock
|
||||
@pytest.mark.asyncio
|
||||
async def test_404_raises_not_found(self, client: GitHubClient) -> None:
|
||||
respx.get("https://api.github.com/repos/owner/repo/pulls/999").mock(
|
||||
return_value=Response(404, json={"message": "Not Found"})
|
||||
)
|
||||
|
||||
with pytest.raises(NotFoundError):
|
||||
await client.get_pr_diff("owner/repo", 999)
|
||||
|
||||
await client.close()
|
||||
|
||||
@respx.mock
|
||||
@pytest.mark.asyncio
|
||||
async def test_401_raises_authentication_error(self, client: GitHubClient) -> None:
|
||||
respx.get("https://api.github.com/repos/owner/repo/pulls/1").mock(
|
||||
return_value=Response(401, json={"message": "Bad credentials"})
|
||||
)
|
||||
|
||||
with pytest.raises(AuthenticationError):
|
||||
await client.get_pr_diff("owner/repo", 1)
|
||||
|
||||
await client.close()
|
||||
|
||||
@respx.mock
|
||||
@pytest.mark.asyncio
|
||||
async def test_429_raises_rate_limit_error(self, client: GitHubClient) -> None:
|
||||
respx.get("https://api.github.com/repos/owner/repo/pulls/1").mock(
|
||||
return_value=Response(
|
||||
429,
|
||||
headers={"Retry-After": "60"},
|
||||
json={"message": "Rate limit exceeded"},
|
||||
)
|
||||
)
|
||||
|
||||
with pytest.raises(RateLimitError) as exc_info:
|
||||
await client.get_pr_diff("owner/repo", 1)
|
||||
|
||||
assert exc_info.value.retry_after == 60
|
||||
await client.close()
|
||||
|
||||
@respx.mock
|
||||
@pytest.mark.asyncio
|
||||
async def test_500_raises_platform_error(self, client: GitHubClient) -> None:
|
||||
respx.get("https://api.github.com/repos/owner/repo/pulls/1").mock(
|
||||
return_value=Response(500, json={"message": "Internal Server Error"})
|
||||
)
|
||||
|
||||
from tenacity import RetryError
|
||||
|
||||
with pytest.raises(RetryError) as exc_info:
|
||||
await client.get_pr_diff("owner/repo", 1)
|
||||
|
||||
# Verify the underlying exception is PlatformError
|
||||
assert exc_info.value.last_attempt.exception() is not None
|
||||
assert isinstance(exc_info.value.last_attempt.exception(), PlatformError)
|
||||
|
||||
await client.close()
|
||||
|
||||
@respx.mock
|
||||
@pytest.mark.asyncio
|
||||
async def test_context_manager(self) -> None:
|
||||
respx.get("https://api.github.com/repos/owner/repo/pulls/1").mock(
|
||||
return_value=Response(200, text="diff content")
|
||||
)
|
||||
|
||||
async with GitHubClient(token="test") as client:
|
||||
diff = await client.get_pr_diff("owner/repo", 1)
|
||||
assert diff == "diff content"
|
||||
|
||||
@respx.mock
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_comments(self, client: GitHubClient) -> None:
|
||||
respx.get("https://api.github.com/repos/owner/repo/issues/1/comments").mock(
|
||||
return_value=Response(
|
||||
200,
|
||||
json=[
|
||||
{
|
||||
"id": 123,
|
||||
"body": "Test comment",
|
||||
"user": {"login": "testuser"},
|
||||
"html_url": "https://github.com/owner/repo/pull/1#comment-123",
|
||||
"created_at": "2025-05-15T10:30:00Z",
|
||||
},
|
||||
{
|
||||
"id": 456,
|
||||
"body": f"Arbiter review {ARBITER_MARKER}",
|
||||
"user": {"login": "arbiter-bot"},
|
||||
"html_url": "https://github.com/owner/repo/pull/1#comment-456",
|
||||
"created_at": "2025-05-15T11:00:00Z",
|
||||
},
|
||||
],
|
||||
)
|
||||
)
|
||||
|
||||
comments = await client.get_comments("owner/repo", 1)
|
||||
|
||||
assert len(comments) == 2
|
||||
assert comments[0].id == "123"
|
||||
assert comments[0].body == "Test comment"
|
||||
assert comments[0].author == "testuser"
|
||||
assert comments[1].id == "456"
|
||||
assert has_arbiter_marker(comments[1].body)
|
||||
await client.close()
|
||||
|
||||
@respx.mock
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_comment(self, client: GitHubClient) -> None:
|
||||
respx.patch("https://api.github.com/repos/owner/repo/issues/comments/123").mock(
|
||||
return_value=Response(
|
||||
200,
|
||||
json={"html_url": "https://github.com/owner/repo/pull/1#comment-123"},
|
||||
)
|
||||
)
|
||||
|
||||
url = await client.update_comment("owner/repo", 1, "123", "Updated content")
|
||||
|
||||
assert url == "https://github.com/owner/repo/pull/1#comment-123"
|
||||
request = respx.calls.last.request
|
||||
assert request is not None
|
||||
await client.close()
|
||||
|
||||
|
||||
class TestGitLabClient:
|
||||
@pytest.fixture
|
||||
def client(self) -> GitLabClient:
|
||||
return GitLabClient(token="test-token", max_retries=1)
|
||||
|
||||
@respx.mock
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_pr_diff(self, client: GitLabClient) -> None:
|
||||
respx.get("https://gitlab.com/api/v4/projects/owner%2Frepo/merge_requests/1/diffs").mock(
|
||||
return_value=Response(
|
||||
200,
|
||||
json=[
|
||||
{
|
||||
"old_path": "file.py",
|
||||
"new_path": "file.py",
|
||||
"diff": "@@ -1 +1 @@\n-old\n+new",
|
||||
}
|
||||
],
|
||||
)
|
||||
)
|
||||
|
||||
diff = await client.get_pr_diff("owner/repo", 1)
|
||||
|
||||
assert "file.py" in diff
|
||||
assert "-old" in diff
|
||||
assert "+new" in diff
|
||||
await client.close()
|
||||
|
||||
@respx.mock
|
||||
@pytest.mark.asyncio
|
||||
async def test_post_comment(self, client: GitLabClient) -> None:
|
||||
respx.post("https://gitlab.com/api/v4/projects/owner%2Frepo/merge_requests/1/notes").mock(
|
||||
return_value=Response(201, json={"id": 123})
|
||||
)
|
||||
|
||||
url = await client.post_comment("owner/repo", 1, "Test comment")
|
||||
|
||||
assert "owner/repo" in url
|
||||
assert "merge_requests/1" in url
|
||||
assert "note_123" in url
|
||||
await client.close()
|
||||
|
||||
@respx.mock
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_commit_status(self, client: GitLabClient) -> None:
|
||||
sha = "abc123def456"
|
||||
respx.post(f"https://gitlab.com/api/v4/projects/owner%2Frepo/statuses/{sha}").mock(
|
||||
return_value=Response(201, json={"status": "pending"})
|
||||
)
|
||||
|
||||
await client.update_commit_status(
|
||||
"owner/repo",
|
||||
sha,
|
||||
CommitStatus.PENDING,
|
||||
"Review in progress",
|
||||
"arbiter",
|
||||
)
|
||||
|
||||
assert respx.calls.last is not None
|
||||
await client.close()
|
||||
|
||||
@respx.mock
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_pr_info(self, client: GitLabClient) -> None:
|
||||
respx.get("https://gitlab.com/api/v4/projects/owner%2Frepo/merge_requests/1").mock(
|
||||
return_value=Response(
|
||||
200,
|
||||
json={
|
||||
"sha": "abc123",
|
||||
"diff_refs": {"head_sha": "abc123", "base_sha": "def456"},
|
||||
"source_branch": "feature",
|
||||
"target_branch": "main",
|
||||
"title": "Test MR",
|
||||
"author": {"username": "testuser"},
|
||||
"web_url": "https://gitlab.com/owner/repo/-/merge_requests/1",
|
||||
"draft": False,
|
||||
},
|
||||
)
|
||||
)
|
||||
|
||||
info = await client.get_pr_info("owner/repo", 1)
|
||||
|
||||
assert info.platform == Platform.GITLAB
|
||||
assert info.repository == "owner/repo"
|
||||
assert info.pr_number == 1
|
||||
assert info.head_sha == "abc123"
|
||||
assert info.title == "Test MR"
|
||||
assert info.author == "testuser"
|
||||
await client.close()
|
||||
|
||||
@respx.mock
|
||||
@pytest.mark.asyncio
|
||||
async def test_404_raises_not_found(self, client: GitLabClient) -> None:
|
||||
respx.get("https://gitlab.com/api/v4/projects/owner%2Frepo/merge_requests/999/diffs").mock(
|
||||
return_value=Response(404, json={"message": "404 Not found"})
|
||||
)
|
||||
|
||||
with pytest.raises(NotFoundError):
|
||||
await client.get_pr_diff("owner/repo", 999)
|
||||
|
||||
await client.close()
|
||||
|
||||
@respx.mock
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_comments(self, client: GitLabClient) -> None:
|
||||
respx.get("https://gitlab.com/api/v4/projects/owner%2Frepo/merge_requests/1/notes").mock(
|
||||
return_value=Response(
|
||||
200,
|
||||
json=[
|
||||
{
|
||||
"id": 123,
|
||||
"body": "Test note",
|
||||
"author": {"username": "testuser"},
|
||||
"created_at": "2025-05-15T10:30:00Z",
|
||||
},
|
||||
{
|
||||
"id": 456,
|
||||
"body": f"Arbiter review {ARBITER_MARKER}",
|
||||
"author": {"username": "arbiter-bot"},
|
||||
"created_at": "2025-05-15T11:00:00Z",
|
||||
},
|
||||
],
|
||||
)
|
||||
)
|
||||
|
||||
comments = await client.get_comments("owner/repo", 1)
|
||||
|
||||
assert len(comments) == 2
|
||||
assert comments[0].id == "123"
|
||||
assert comments[0].body == "Test note"
|
||||
assert comments[0].author == "testuser"
|
||||
assert "note_123" in comments[0].url
|
||||
assert comments[1].id == "456"
|
||||
assert has_arbiter_marker(comments[1].body)
|
||||
await client.close()
|
||||
|
||||
@respx.mock
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_comment(self, client: GitLabClient) -> None:
|
||||
respx.put(
|
||||
"https://gitlab.com/api/v4/projects/owner%2Frepo/merge_requests/1/notes/123"
|
||||
).mock(return_value=Response(200, json={"id": 123}))
|
||||
|
||||
url = await client.update_comment("owner/repo", 1, "123", "Updated content")
|
||||
|
||||
assert "owner/repo" in url
|
||||
assert "merge_requests/1" in url
|
||||
assert "note_123" in url
|
||||
await client.close()
|
||||
|
||||
|
||||
class TestReviewCommentFormatter:
|
||||
@pytest.fixture
|
||||
def formatter(self) -> ReviewCommentFormatter:
|
||||
return ReviewCommentFormatter(include_cost=True)
|
||||
|
||||
@pytest.fixture
|
||||
def sample_finding(self) -> Finding:
|
||||
return Finding(
|
||||
id="finding-1",
|
||||
agent=AgentName.SECURITY,
|
||||
file="src/auth.py",
|
||||
line_start=10,
|
||||
line_end=15,
|
||||
severity=Severity.HIGH,
|
||||
confidence=0.9,
|
||||
title="SQL Injection vulnerability",
|
||||
description="User input is directly concatenated into SQL query",
|
||||
reasoning="String concatenation allows SQL injection",
|
||||
suggestion="Use parameterized queries",
|
||||
references=["https://owasp.org/sql-injection"],
|
||||
prompt_version="security-v1.0",
|
||||
)
|
||||
|
||||
@pytest.fixture
|
||||
def sample_conflict(self) -> Conflict:
|
||||
return Conflict(
|
||||
id="conflict-1",
|
||||
finding_ids=["finding-1", "finding-2"],
|
||||
nature=ConflictNature.TRADE_OFF,
|
||||
description="Security vs complexity trade-off",
|
||||
severity_weight=0.8,
|
||||
resolution="Prefer security",
|
||||
)
|
||||
|
||||
def test_format_approve_verdict(self, formatter: ReviewCommentFormatter) -> None:
|
||||
result = DeliberationResult(
|
||||
findings=[],
|
||||
verdict=Verdict.APPROVE,
|
||||
verdict_confidence=0.95,
|
||||
verdict_reasoning="No issues found",
|
||||
total_findings=0,
|
||||
)
|
||||
|
||||
comment = formatter.format(result)
|
||||
|
||||
assert "Approve" in comment
|
||||
assert "95%" in comment
|
||||
assert "No issues found" in comment
|
||||
|
||||
def test_format_request_changes_verdict(
|
||||
self, formatter: ReviewCommentFormatter, sample_finding: Finding
|
||||
) -> None:
|
||||
result = DeliberationResult(
|
||||
findings=[sample_finding],
|
||||
verdict=Verdict.REQUEST_CHANGES,
|
||||
verdict_confidence=0.9,
|
||||
verdict_reasoning="Found critical security issue",
|
||||
total_findings=1,
|
||||
critical_count=0,
|
||||
high_count=1,
|
||||
)
|
||||
|
||||
comment = formatter.format(result)
|
||||
|
||||
assert "Request Changes" in comment
|
||||
assert "SQL Injection" in comment
|
||||
assert "src/auth.py" in comment
|
||||
assert "High" in comment
|
||||
|
||||
def test_format_with_conflicts(
|
||||
self,
|
||||
formatter: ReviewCommentFormatter,
|
||||
sample_finding: Finding,
|
||||
sample_conflict: Conflict,
|
||||
) -> None:
|
||||
result = DeliberationResult(
|
||||
findings=[sample_finding],
|
||||
conflicts=[sample_conflict],
|
||||
verdict=Verdict.COMMENT,
|
||||
verdict_confidence=0.7,
|
||||
verdict_reasoning="Issues need discussion",
|
||||
total_findings=1,
|
||||
)
|
||||
|
||||
comment = formatter.format(result)
|
||||
|
||||
assert "Conflicts" in comment
|
||||
assert "Trade Off" in comment
|
||||
assert "Security vs complexity" in comment
|
||||
|
||||
def test_format_includes_cost(self, formatter: ReviewCommentFormatter) -> None:
|
||||
result = DeliberationResult(
|
||||
findings=[],
|
||||
verdict=Verdict.APPROVE,
|
||||
verdict_confidence=0.95,
|
||||
verdict_reasoning="No issues",
|
||||
tokens_used=1000,
|
||||
cost_usd=0.05,
|
||||
)
|
||||
|
||||
comment = formatter.format(result)
|
||||
|
||||
assert "1,000" in comment # tokens with comma
|
||||
assert "$0.0500" in comment
|
||||
|
||||
def test_format_without_cost(self) -> None:
|
||||
formatter = ReviewCommentFormatter(include_cost=False)
|
||||
result = DeliberationResult(
|
||||
findings=[],
|
||||
verdict=Verdict.APPROVE,
|
||||
verdict_confidence=0.95,
|
||||
verdict_reasoning="No issues",
|
||||
tokens_used=1000,
|
||||
cost_usd=0.05,
|
||||
)
|
||||
|
||||
comment = formatter.format(result)
|
||||
|
||||
assert "Tokens used" not in comment
|
||||
assert "$0.05" not in comment
|
||||
|
||||
def test_format_truncates_long_comment(
|
||||
self, formatter: ReviewCommentFormatter, sample_finding: Finding
|
||||
) -> None:
|
||||
# Create many findings
|
||||
findings = []
|
||||
for i in range(100):
|
||||
f = sample_finding.model_copy()
|
||||
f.id = f"finding-{i}"
|
||||
f.description = "x" * 1000 # Long description
|
||||
findings.append(f)
|
||||
|
||||
result = DeliberationResult(
|
||||
findings=findings,
|
||||
verdict=Verdict.REQUEST_CHANGES,
|
||||
verdict_confidence=0.9,
|
||||
verdict_reasoning="Many issues",
|
||||
total_findings=100,
|
||||
)
|
||||
|
||||
comment = formatter.format(result)
|
||||
|
||||
assert len(comment) <= ReviewCommentFormatter.MAX_COMMENT_LENGTH
|
||||
|
||||
def test_format_limits_findings(self) -> None:
|
||||
formatter = ReviewCommentFormatter(max_findings=5)
|
||||
findings = [
|
||||
Finding(
|
||||
id=f"finding-{i}",
|
||||
agent=AgentName.STYLE,
|
||||
file=f"file{i}.py",
|
||||
line_start=1,
|
||||
line_end=1,
|
||||
severity=Severity.LOW,
|
||||
confidence=0.5,
|
||||
title=f"Issue {i}",
|
||||
description="Description",
|
||||
reasoning="Reasoning",
|
||||
prompt_version="style-v1.0",
|
||||
)
|
||||
for i in range(10)
|
||||
]
|
||||
|
||||
result = DeliberationResult(
|
||||
findings=findings,
|
||||
verdict=Verdict.COMMENT,
|
||||
verdict_confidence=0.7,
|
||||
verdict_reasoning="Multiple issues",
|
||||
total_findings=10,
|
||||
)
|
||||
|
||||
comment = formatter.format(result)
|
||||
|
||||
assert "more findings" in comment
|
||||
|
||||
def test_format_includes_marker(self, formatter: ReviewCommentFormatter) -> None:
|
||||
result = DeliberationResult(
|
||||
findings=[],
|
||||
verdict=Verdict.APPROVE,
|
||||
verdict_confidence=0.95,
|
||||
verdict_reasoning="No issues found",
|
||||
total_findings=0,
|
||||
)
|
||||
|
||||
comment = formatter.format(result)
|
||||
|
||||
assert ARBITER_MARKER in comment
|
||||
assert has_arbiter_marker(comment)
|
||||
|
||||
|
||||
class TestArbiterMarker:
|
||||
def test_has_marker_returns_true(self) -> None:
|
||||
text = f"Some comment\n{ARBITER_MARKER}\nMore text"
|
||||
assert has_arbiter_marker(text) is True
|
||||
|
||||
def test_has_marker_returns_false(self) -> None:
|
||||
text = "Some comment without marker"
|
||||
assert has_arbiter_marker(text) is False
|
||||
|
||||
def test_marker_is_html_comment(self) -> None:
|
||||
assert ARBITER_MARKER.startswith("<!--")
|
||||
assert ARBITER_MARKER.endswith("-->")
|
||||
|
||||
|
||||
class TestPlatformDetection:
|
||||
def test_detect_github_from_webhook(self) -> None:
|
||||
platform = detect_platform("owner/repo", "github")
|
||||
assert platform == Platform.GITHUB
|
||||
|
||||
def test_detect_gitlab_from_webhook(self) -> None:
|
||||
platform = detect_platform("owner/repo", "gitlab")
|
||||
assert platform == Platform.GITLAB
|
||||
|
||||
def test_detect_github_case_insensitive(self) -> None:
|
||||
assert detect_platform("owner/repo", "GitHub") == Platform.GITHUB
|
||||
assert detect_platform("owner/repo", "GITLAB") == Platform.GITLAB
|
||||
|
||||
def test_default_to_github(self) -> None:
|
||||
platform = detect_platform("owner/repo", None)
|
||||
assert platform == Platform.GITHUB
|
||||
Reference in New Issue
Block a user