From fe03b05ad4c146c7463ae875d367740dca96ddb0 Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Fri, 31 Jul 2026 14:46:14 -0400 Subject: [PATCH] test(e2e): isolate reviewer verdict coverage --- tests/e2e/fakes.py | 2 + tests/e2e/harness.py | 20 ++++++- tests/e2e/tests/dashboard.spec.ts | 53 +++++++++++++------ tests/e2e/tests/reviewer_verification.spec.ts | 39 ++++++++------ ui/src/routes/review.tsx | 1 + 5 files changed, 83 insertions(+), 32 deletions(-) diff --git a/tests/e2e/fakes.py b/tests/e2e/fakes.py index e7fc1efb..e08aac2a 100644 --- a/tests/e2e/fakes.py +++ b/tests/e2e/fakes.py @@ -71,6 +71,7 @@ def slack_messages(channel: str) -> list[dict[str, Any]]: PULLS: list[dict[str, Any]] = [] CHECK_RUNS: list[dict[str, Any]] = [] REVIEW_DISPATCHES: list[dict[str, Any]] = [] +EMAIL_MAPPING_LOOKUPS: list[str] = [] _pr_seq = [0] _check_seq = [0] @@ -214,6 +215,7 @@ def reset() -> None: PULLS.clear() CHECK_RUNS.clear() REVIEW_DISPATCHES.clear() + EMAIL_MAPPING_LOOKUPS.clear() _pr_seq[0] = 0 _check_seq[0] = 0 seed_bare_remote() diff --git a/tests/e2e/harness.py b/tests/e2e/harness.py index 27a05c5e..24e1bd6e 100644 --- a/tests/e2e/harness.py +++ b/tests/e2e/harness.py @@ -62,7 +62,7 @@ _SLACK_USERS: dict[str, dict[str, str]] = { from agent.api.app import app # noqa: E402 from agent.dashboard import routes as dashboard_routes # noqa: E402 from agent.dashboard.oauth import COOKIE_NAME, issue_session # noqa: E402 -from agent.utils import github_checks # noqa: E402 +from agent.utils import github_checks, github_org_membership # noqa: E402 from agent.utils.thread_ids import generate_thread_id_from_slack_thread # noqa: E402 from agent.webhooks import common as webhook_common # noqa: E402 @@ -74,6 +74,7 @@ CURRENT_THREAD: dict[str, str | None] = {"channel": DEMO_CHANNEL, "thread_ts": N fakes.seed_bare_remote() _real_dispatch_agent_run = webhook_common.dispatch_agent_run +_real_email_for_login = webhook_common.email_for_login async def _fake_installation_token(*_args: object, **_kwargs: object) -> str: @@ -103,6 +104,15 @@ async def _fake_started_comment(*_args: object, **_kwargs: object) -> None: return None +async def _fake_active_org_member(username: str, org: str) -> bool: + return bool(username and org) + + +async def _record_email_mapping_lookup(login: str) -> str | None: + fakes.EMAIL_MAPPING_LOOKUPS.append(login) + return await _real_email_for_login(login) + + async def _record_review_dispatch( thread_id: str, prompt: str, @@ -151,6 +161,9 @@ webhook_common._reviewer_token_for_repo = _fake_reviewer_token webhook_common.react_to_github_comment = _fake_reaction webhook_common.post_review_started_comment = _fake_started_comment webhook_common.dispatch_agent_run = _record_review_dispatch +webhook_common.email_for_login = _record_email_mapping_lookup +github_org_membership.is_user_active_org_member = _fake_active_org_member +webhook_common.is_user_active_org_member = _fake_active_org_member dashboard_routes._fetch_user_installations_and_repos = _fake_installations_and_repos github_checks._GITHUB_API_BASE = e2e_env.FAKE_GITHUB_API @@ -175,6 +188,11 @@ async def control_review_dispatches() -> JSONResponse: return JSONResponse(fakes.REVIEW_DISPATCHES) +@app.get("/control/email-mapping-lookups") +async def control_email_mapping_lookups() -> JSONResponse: + return JSONResponse(fakes.EMAIL_MAPPING_LOOKUPS) + + @app.get("/control/check-runs") async def control_check_runs() -> JSONResponse: return JSONResponse(fakes.CHECK_RUNS) diff --git a/tests/e2e/tests/dashboard.spec.ts b/tests/e2e/tests/dashboard.spec.ts index e38e0a10..23d0f1e4 100644 --- a/tests/e2e/tests/dashboard.spec.ts +++ b/tests/e2e/tests/dashboard.spec.ts @@ -56,33 +56,54 @@ async function expectTranscriptVisible(page: Page) { }).toPass({ timeout: 60000 }); } +async function resetAutoVerdictPolicies( + page: Page, + baseURL: string | undefined, +) { + await loginAs(page, SAME_USER); + const headers = { origin: baseURL ?? "" }; + const currentResponse = await page.request.get( + "/dashboard/api/team-settings", + ); + expect(currentResponse.ok()).toBeTruthy(); + const current = await currentResponse.json(); + const teamResponse = await page.request.put("/dashboard/api/team-settings", { + data: { ...current, auto_verdict: false }, + headers, + }); + expect(teamResponse.ok(), await teamResponse.text()).toBeTruthy(); + const repoResponse = await page.request.put( + "/dashboard/api/auto-verdict-repos", + { + data: { full_name: "fakeorg/demo", enabled: false }, + headers, + }, + ); + expect(repoResponse.ok()).toBeTruthy(); +} + test.describe("Review verdict settings (real dashboard UI and API)", () => { + test.beforeEach(async ({ page, baseURL }) => { + await resetAutoVerdictPolicies(page, baseURL); + }); + + test.afterEach(async ({ page, baseURL }) => { + await resetAutoVerdictPolicies(page, baseURL); + }); + test("admin controls persist team and per-repository auto-verdict policy", async ({ page, baseURL, }) => { - await loginAs(page, SAME_USER); const mutationHeaders = { origin: baseURL ?? "" }; - const resetTeam = await page.request.put("/dashboard/api/team-settings", { - data: { auto_verdict: false }, - headers: mutationHeaders, - }); - expect(resetTeam.ok(), await resetTeam.text()).toBeTruthy(); - const resetRepo = await page.request.put( - "/dashboard/api/auto-verdict-repos", - { - data: { full_name: "fakeorg/demo", enabled: false }, - headers: mutationHeaders, - }, - ); - expect(resetRepo.ok()).toBeTruthy(); - await page.goto("/review"); await expect( page.getByRole("heading", { name: "Open SWE Review" }), ).toBeVisible(); - const teamToggle = page.getByRole("switch").first(); + const teamToggle = page.getByRole("switch", { + name: "Allow automatic verdicts team-wide", + }); await expect(teamToggle).not.toBeChecked(); const teamSaved = page.waitForResponse( (response) => diff --git a/tests/e2e/tests/reviewer_verification.spec.ts b/tests/e2e/tests/reviewer_verification.spec.ts index 839dec98..f32ad6f1 100644 --- a/tests/e2e/tests/reviewer_verification.spec.ts +++ b/tests/e2e/tests/reviewer_verification.spec.ts @@ -72,11 +72,11 @@ test.describe("Reviewer verification contracts", () => { repo: { owner: "fakeorg", name: "demo" }, pr_number: pr.number, }); - expect(dispatches[0].configurable).not.toHaveProperty("email"); - expect(dispatches[0].prompt).toContain( - "focus on authorization boundaries", - ); - + const emailMappingLookups = await ( + await page.request.get("/control/email-mapping-lookups") + ).json(); + expect(emailMappingLookups).toEqual([]); + expect(dispatches[0].prompt).toContain("focus on authorization boundaries"); }); test("review outcomes settle checks as success, failure, or neutral", async ({ @@ -102,6 +102,16 @@ test.describe("Reviewer verification contracts", () => { conclusion: "failure", title: "Changes requested", }, + { + outcome: { + verdict_submitted: false, + verdict_authorization: "consistent", + verdict_ignored_reason: "approve_with_open_findings", + blocking_finding_count: 2, + }, + conclusion: "failure", + title: "Found 2 blocking issues", + }, { outcome: { verdict_submitted: false, @@ -130,16 +140,15 @@ test.describe("Reviewer verification contracts", () => { }); } - const checks = await ( - await page.request.get("/control/check-runs") - ).json(); - expect(checks.map((check: { conclusion: string }) => check.conclusion)).toEqual([ - "success", - "failure", - "neutral", - ]); + const checks = await (await page.request.get("/control/check-runs")).json(); expect( - checks.map((check: { status: string }) => check.status), - ).toEqual(["completed", "completed", "completed"]); + checks.map((check: { conclusion: string }) => check.conclusion), + ).toEqual(["success", "failure", "failure", "neutral"]); + expect(checks.map((check: { status: string }) => check.status)).toEqual([ + "completed", + "completed", + "completed", + "completed", + ]); }); }); diff --git a/ui/src/routes/review.tsx b/ui/src/routes/review.tsx index 8b36acbc..6a672810 100644 --- a/ui/src/routes/review.tsx +++ b/ui/src/routes/review.tsx @@ -147,6 +147,7 @@ function ReviewPage() { description="Allow trusted, non-fork pull requests to receive reviewer verdicts by default. Repository-specific opt-ins remain available when this is off." control={ persist({ auto_verdict: v })} disabled={!canEdit}