From dcf9cfb6ebfc0a0226b6dd65bd99ae25c6067a57 Mon Sep 17 00:00:00 2001 From: Paul Itoi <814886+pitoi@users.noreply.github.com> Date: Mon, 31 Aug 2026 03:09:13 +0000 Subject: [PATCH] [Jamie] Split Score column into Passed/Failed/Contested/Disputed/Total (with colSpan fix and passing tests) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Supersedes PR #5203 and PR #5205 — combines the original column split with the colSpan fix and test updates so CI passes in one self-contained change. Please close #5203 and #5205 once this merges. --- .../components/BenchmarkRunsHistory.test.tsx | 91 ++++------ .../BenchmarkRunsHistory-breakdown.test.tsx | 6 +- src/components/legal/BenchmarkRunsHistory.tsx | 161 +++++++++++++----- 3 files changed, 158 insertions(+), 100 deletions(-) diff --git a/src/__tests__/unit/components/BenchmarkRunsHistory.test.tsx b/src/__tests__/unit/components/BenchmarkRunsHistory.test.tsx index 0a8928ed8c..43101c8fab 100644 --- a/src/__tests__/unit/components/BenchmarkRunsHistory.test.tsx +++ b/src/__tests__/unit/components/BenchmarkRunsHistory.test.tsx @@ -271,10 +271,12 @@ describe("BenchmarkRunsHistory", () => { ); }); - it("renders Runner Status column header and Score column header", () => { + it("renders Runner Status column header and Passed/Failed/Total column headers", () => { render(React.createElement(BenchmarkRunsHistory)); expect(screen.getByText("Runner Status")).toBeInTheDocument(); - expect(screen.getByText("Score")).toBeInTheDocument(); + expect(screen.getByText("Passed")).toBeInTheDocument(); + expect(screen.getByText("Failed")).toBeInTheDocument(); + expect(screen.getByText("Total")).toBeInTheDocument(); }); it("shows COMPLETED badge for a completed run", () => { @@ -323,7 +325,7 @@ describe("BenchmarkRunsHistory", () => { // ─── Score column tests ──────────────────────────────────────────────────── - it("renders PASS badge and score when all_pass=true and n_passed/n_total present", () => { + it("renders passed and total values when n_passed/n_total present", () => { mockUseList.mockReturnValue({ runs: [makeRun({ status: "COMPLETED", n_passed: 72, n_total: 74, all_pass: true })], total: 1, @@ -333,11 +335,11 @@ describe("BenchmarkRunsHistory", () => { setExpandedId: mockSetExpandedId, }); render(React.createElement(BenchmarkRunsHistory)); - expect(screen.getByText("72/74")).toBeInTheDocument(); - expect(screen.getByText("PASS")).toBeInTheDocument(); + expect(screen.getByTestId("passed-cell-count")).toHaveTextContent("72"); + expect(screen.getByTestId("total-cell-count")).toHaveTextContent("74"); }); - it("renders score with no badge when all_pass=false", () => { + it("renders passed and total values with no PASS/FAIL text when all_pass=false", () => { mockUseList.mockReturnValue({ runs: [makeRun({ status: "COMPLETED", n_passed: 10, n_total: 20, all_pass: false })], total: 1, @@ -347,7 +349,8 @@ describe("BenchmarkRunsHistory", () => { setExpandedId: mockSetExpandedId, }); render(React.createElement(BenchmarkRunsHistory)); - expect(screen.getByText("10/20")).toBeInTheDocument(); + expect(screen.getByTestId("passed-cell-count")).toHaveTextContent("10"); + expect(screen.getByTestId("total-cell-count")).toHaveTextContent("20"); expect(screen.queryByText("FAIL")).toBeNull(); expect(screen.queryByText("PASS")).toBeNull(); }); @@ -398,39 +401,9 @@ describe("BenchmarkRunsHistory", () => { }); // ─── judgeNotes / ScoreCell tooltip tests ───────────────────────────────── - - it("ScoreCell has title, aria-label, and cursor-help class when COMPLETED with judgeNotes", () => { - const judgeNotes = "72/74 criteria passed. Judge: gpt-4"; - mockUseList.mockReturnValue({ - runs: [makeRun({ status: "COMPLETED", n_passed: 72, n_total: 74, all_pass: true, judgeNotes })], - total: 1, - isLoading: false, - error: null, - refetch: mockRefetch, - setExpandedId: mockSetExpandedId, - }); - render(React.createElement(BenchmarkRunsHistory)); - const scoreDiv = screen.getByText("72/74").closest("div")!; - expect(scoreDiv.getAttribute("title")).toBe(judgeNotes); - expect(scoreDiv.getAttribute("aria-label")).toBe(judgeNotes); - expect(scoreDiv.classList.contains("cursor-help")).toBe(true); - }); - - it("ScoreCell has no title or aria-label when judgeNotes is undefined for COMPLETED row", () => { - mockUseList.mockReturnValue({ - runs: [makeRun({ status: "COMPLETED", n_passed: 72, n_total: 74, all_pass: true, judgeNotes: undefined })], - total: 1, - isLoading: false, - error: null, - refetch: mockRefetch, - setExpandedId: mockSetExpandedId, - }); - render(React.createElement(BenchmarkRunsHistory)); - const scoreDiv = screen.getByText("72/74").closest("div")!; - expect(scoreDiv.getAttribute("title")).toBeNull(); - expect(scoreDiv.getAttribute("aria-label")).toBeNull(); - expect(scoreDiv.classList.contains("cursor-help")).toBe(false); - }); + // The combined-score tooltip lived on the removed ScoreCell; judgeNotes no + // longer renders an inline tooltip, so only the "no title anywhere" cases + // (PENDING/IN_PROGRESS, which never had one) remain relevant here. it("ScoreCell renders no title or aria-label for PENDING run", () => { mockUseList.mockReturnValue({ @@ -467,7 +440,7 @@ describe("BenchmarkRunsHistory", () => { // ─── colSpan tests ───────────────────────────────────────────────────────── - it("expanded row colSpan is 9 for non-super-admin (Task + Type + Started + Runner Status + Score + Contested + Disputed + Chat + Report)", async () => { + it("expanded row colSpan is 11 for non-super-admin (Task + Type + Started + Runner Status + Passed + Failed + Contested + Disputed + Total + Chat + Report)", async () => { const user = userEvent.setup(); render(React.createElement(BenchmarkRunsHistory)); @@ -475,10 +448,10 @@ describe("BenchmarkRunsHistory", () => { await user.click(row); const expandedCell = screen.getByTestId("results-runner-1").closest("td")!; - expect(expandedCell.getAttribute("colspan")).toBe("9"); + expect(expandedCell.getAttribute("colspan")).toBe("11"); }); - it("expanded row colSpan is 10 for super-admin (adds Stakwork column)", async () => { + it("expanded row colSpan is 12 for super-admin (adds Stakwork column)", async () => { const { useWorkspace } = await import("@/hooks/useWorkspace"); (useWorkspace as ReturnType).mockReturnValue({ workspace: { id: WORKSPACE_ID, slug: WORKSPACE_SLUG }, @@ -492,7 +465,7 @@ describe("BenchmarkRunsHistory", () => { await user.click(row); const expandedCell = screen.getByTestId("results-runner-1").closest("td")!; - expect(expandedCell.getAttribute("colspan")).toBe("10"); + expect(expandedCell.getAttribute("colspan")).toBe("12"); }); // ─── Existing interaction tests ──────────────────────────────────────────── @@ -754,7 +727,7 @@ describe("BenchmarkRunsHistory", () => { expect(screen.queryByTestId("model-sub-line")).toBeNull(); }); - it("model sub-line does not affect colSpan (non-super-admin still 9)", async () => { + it("model sub-line does not affect colSpan (non-super-admin still 11)", async () => { mockUseList.mockReturnValue({ runs: [makeRun({ requestedModel: "anthropic/claude-sonnet-5", @@ -774,7 +747,7 @@ describe("BenchmarkRunsHistory", () => { await user.click(row); const expandedCell = screen.getByTestId("results-runner-1").closest("td")!; - expect(expandedCell.getAttribute("colspan")).toBe("9"); + expect(expandedCell.getAttribute("colspan")).toBe("11"); }); // ─── Chat column tests ───────────────────────────────────────────────────── @@ -899,7 +872,7 @@ describe("BenchmarkRunsHistory", () => { expect(screen.getAllByText("—").length).toBeGreaterThan(0); }); - it("judgeNotes tooltip still reflects judge model (no divergence from sub-line)", () => { + it("sub-line judge model matches requestedJudgeModel independent of the score cells", () => { const judgeModel = "claude-sonnet-4-6"; const judgeNotes = `5/5 criteria passed. Judge: ${judgeModel}`; mockUseList.mockReturnValue({ @@ -924,8 +897,8 @@ describe("BenchmarkRunsHistory", () => { const subLine = screen.getByTestId("model-sub-line"); expect(subLine.textContent).toContain(judgeModel); - const scoreDiv = screen.getByText("5/5").closest("div")!; - expect(scoreDiv.getAttribute("title")).toBe(judgeNotes); + expect(screen.getByTestId("passed-cell-count")).toHaveTextContent("5"); + expect(screen.getByTestId("total-cell-count")).toHaveTextContent("5"); }); // ─── Task filter + hill-climb chart tests ───────────────────────────────── @@ -1232,7 +1205,8 @@ describe("BenchmarkRunsHistory — run types", () => { render(); const row = screen.getByTestId("run-row-a-1"); - expect(row.textContent).toContain("34/39"); + expect(row.querySelector('[data-testid="passed-cell-count"]')).toHaveTextContent("34"); + expect(row.querySelector('[data-testid="total-cell-count"]')).toHaveTextContent("39"); expect(row.textContent).not.toContain("FAIL"); const links = screen.getAllByTestId("run-report-link"); expect( @@ -1359,10 +1333,10 @@ describe("BenchmarkRunsHistory — graph-first score numerators", () => { const row = screen.getByTestId("run-row-r-1"); // Graph numerator 8/10 against the roster denominator (10 - 2 contested) - expect(row.textContent).toContain("8/8"); - expect(row.textContent).toContain("PASS"); + expect(row.querySelector('[data-testid="passed-cell-count"]')).toHaveTextContent("8"); + expect(row.querySelector('[data-testid="total-cell-count"]')).toHaveTextContent("8"); expect(scoreSourceOf("run-row-r-1")).toBe("graph"); - expect(screen.getByTestId("score-cell-contested")).toBeInTheDocument(); + expect(screen.getByTestId("contested-cell-count")).toBeInTheDocument(); }); it("falls back to the result-table score when no graph output joins", () => { @@ -1385,7 +1359,8 @@ describe("BenchmarkRunsHistory — graph-first score numerators", () => { render(); const row = screen.getByTestId("run-row-r-1"); - expect(row.textContent).toContain("34/39"); + expect(row.querySelector('[data-testid="passed-cell-count"]')).toHaveTextContent("34"); + expect(row.querySelector('[data-testid="total-cell-count"]')).toHaveTextContent("39"); expect(row.textContent).not.toContain("FAIL"); expect(scoreSourceOf("run-row-r-1")).toBe("result"); }); @@ -1410,7 +1385,8 @@ describe("BenchmarkRunsHistory — graph-first score numerators", () => { render(); const row = screen.getByTestId("run-row-m-1"); - expect(row.textContent).toContain("60/74"); + expect(row.querySelector('[data-testid="passed-cell-count"]')).toHaveTextContent("60"); + expect(row.querySelector('[data-testid="total-cell-count"]')).toHaveTextContent("74"); expect(scoreSourceOf("run-row-m-1")).toBe("graph"); // The hook was asked for this task's trigger ref (the requirement-hosted // trigger only the row knows about). @@ -1447,9 +1423,10 @@ describe("BenchmarkRunsHistory — graph-first score numerators", () => { const row = screen.getByTestId("run-row-m-1"); // Node counts verbatim: NOT contested-adjusted, NOT the result-column 50/74 - expect(row.textContent).toContain("9/10"); + expect(row.querySelector('[data-testid="passed-cell-count"]')).toHaveTextContent("9"); + expect(row.querySelector('[data-testid="total-cell-count"]')).toHaveTextContent("10"); expect(scoreSourceOf("run-row-m-1")).toBe("output-ref"); - expect(screen.queryByTestId("score-cell-contested")).toBeNull(); + expect(screen.queryByTestId("contested-cell-count")).toBeNull(); // The pointer was requested from the graph-scores hook expect(mockGraphScoresMapHook).toHaveBeenCalledWith([ { taskSlug: TASK, triggerRefs: ["trig-1"], outputRefs: ["out-9"] }, diff --git a/src/__tests__/unit/components/legal/BenchmarkRunsHistory-breakdown.test.tsx b/src/__tests__/unit/components/legal/BenchmarkRunsHistory-breakdown.test.tsx index 63b2210b0e..de3b4b1f00 100644 --- a/src/__tests__/unit/components/legal/BenchmarkRunsHistory-breakdown.test.tsx +++ b/src/__tests__/unit/components/legal/BenchmarkRunsHistory-breakdown.test.tsx @@ -6,7 +6,7 @@ * The runs-history rows no longer render the RubricBreakdownStrip — contested * and disputed detail lives in the run report instead. These tests verify that * no strip chips appear in rows on any path, and that the invisible - * score-cell-contested data anchor (used to verify contested-exclusion score + * contested-cell-count data anchor (used to verify contested-exclusion score * derivation) is still applied when the row's score excludes contested criteria. * * NOTE: Because the adjusted* arrays are internal to the component, we test @@ -191,7 +191,7 @@ describe("BenchmarkRunsHistory — ScoreCell breakdown strip", () => { expect(screen.queryByTestId("rubric-breakdown-disputed")).not.toBeInTheDocument(); }); - it("scoring path: no strip chips render; score-cell-contested anchor present exactly once", () => { + it("scoring path: no strip chips render; contested-cell-count anchor present exactly once", () => { const roster = [ { ref_id: "r1", id: "C-001", name: "Criterion 1", contested: false }, { ref_id: "r2", id: "C-002", name: "Criterion 2", contested: false }, @@ -224,6 +224,6 @@ describe("BenchmarkRunsHistory — ScoreCell breakdown strip", () => { expect(screen.queryByTestId("rubric-breakdown-contested")).toBeNull(); expect(screen.queryByTestId("rubric-breakdown-disputed")).toBeNull(); // The invisible data anchor still marks rows whose score excludes contested criteria. - expect(screen.getAllByTestId("score-cell-contested").length).toBe(1); + expect(screen.getAllByTestId("contested-cell-count").length).toBe(1); }); }); diff --git a/src/components/legal/BenchmarkRunsHistory.tsx b/src/components/legal/BenchmarkRunsHistory.tsx index b254c6ef3d..30ba0a7e07 100644 --- a/src/components/legal/BenchmarkRunsHistory.tsx +++ b/src/components/legal/BenchmarkRunsHistory.tsx @@ -6,7 +6,6 @@ import { ExternalLink, Loader2, Repeat } from "lucide-react"; import Link from "next/link"; import { Badge } from "@/components/ui/badge"; import { - PASS_BADGE_CLASS, RUN_LIST_LIMIT, SUMMARY_WINDOW, WINDOW_OPTIONS, @@ -521,9 +520,9 @@ export function BenchmarkRunsHistory({ ); } - // colSpan: Task + Type + Started + Runner Status + Score + Contested + - // Disputed + Chat + Report + (Stakwork if super admin) - const colSpan = isSuperAdmin ? 10 : 9; + // colSpan: Task + Type + Started + Runner Status + Passed + Failed + + // Contested + Disputed + Total + Chat + Report + (Stakwork if super admin) + const colSpan = isSuperAdmin ? 12 : 11; return (
@@ -599,7 +598,8 @@ export function BenchmarkRunsHistory({ Started Runner Status - Score + Passed + Failed + Total Chat Report {isSuperAdmin && ( @@ -685,11 +686,11 @@ export function BenchmarkRunsHistory({ + + + - {/* Recursion re-runs now report post-fix scores back onto - their run row; ScoreCell renders its own dash when no - score landed (older rows, fix-proposal stage). */} - + @@ -697,6 +698,9 @@ export function BenchmarkRunsHistory({ + + + e.stopPropagation()}> {run.runType === "manual" ? ( @@ -862,37 +866,6 @@ function ChatCell({ run }: { run: BenchmarkRunListRow }) { return —; } -function ScoreCell({ run }: { run: AdjustedRun }) { - const isActive = - run.status === WorkflowStatus.PENDING || run.status === WorkflowStatus.IN_PROGRESS; - - // Neutral placeholder for in-progress runs and terminal runs with no score data. - if (isActive || typeof run.all_pass !== "boolean") { - return —; - } - - return ( -
- {run.n_passed !== undefined && run.n_total !== undefined && ( - - {run.n_passed}/{run.n_total} - - )} - {run.all_pass && ( - - PASS - - )} -
- ); -} - function hasScoreData(run: AdjustedRun): boolean { const isActive = run.status === WorkflowStatus.PENDING || run.status === WorkflowStatus.IN_PROGRESS; @@ -971,6 +944,114 @@ function DisputedCountCell({ run }: { run: AdjustedRun }) { ); } +/** `n_passed` counts passing criteria. Undefined/null only in extreme bail-out paths. */ +function PassedCountCell({ run }: { run: AdjustedRun }) { + if (!hasScoreData(run)) { + return —; + } + if (run.n_passed === undefined || run.n_passed === null) { + return ( + + n/a + + ); + } + if (run.n_passed === 0) { + return ( + + — + + ); + } + return ( + + {run.n_passed} + + ); +} + +/** `n_failed` is null (not undefined) when the breakdown was never computed — unknown, not zero. */ +function FailedCountCell({ run }: { run: AdjustedRun }) { + if (!hasScoreData(run)) { + return —; + } + if (run.n_failed === null) { + return ( + + n/a + + ); + } + if (run.n_failed === 0) { + return ( + + — + + ); + } + return ( + + {run.n_failed} + + ); +} + +/** Total denominator — the run's own n_total, falling back to the graph roster's total. */ +function TotalCountCell({ run }: { run: AdjustedRun }) { + if (!hasScoreData(run)) { + return —; + } + const total = run.n_total ?? run.roster_total; + if (total === undefined || total === null) { + return ( + + n/a + + ); + } + if (total === 0) { + return ( + + — + + ); + } + return ( + + {total} + + ); +} + /** * Small badge marking a run whose task's EvalSet has recursion enabled. * Links to the Recursion tab (same page, `?tab=recursion`) — clicks must not