diff --git a/node_modules b/node_modules new file mode 120000 index 00000000..4340d146 --- /dev/null +++ b/node_modules @@ -0,0 +1 @@ +../mcp-server-a11y-pmfix/node_modules \ No newline at end of file diff --git a/src/tools/accessibility.ts b/src/tools/accessibility.ts index 5fe52c60..b76f7e9d 100644 --- a/src/tools/accessibility.ts +++ b/src/tools/accessibility.ts @@ -27,7 +27,7 @@ import { compareIssues, loadMainIssues, parseAccessibilityReportFromCSV, - type ReviewCounts, + type IssueSummary as WebIssueSummary, } from "./accessiblity-utils/report-parser.js"; import { wrapUntrusted } from "../lib/untrusted-content.js"; import { queryAccessibilityRAG } from "./accessiblity-utils/accessibility-rag.js"; @@ -545,26 +545,35 @@ function summaryNote(summary: AppIssueSummary, total: number): string[] { ]; } -const REVIEW_TAGS_NOTE = - "needs_review marks an issue the engine could not decide, so a person confirms it: the value is its review status, and until it is confirmed it is not a confirmed failure. best_practice marks a best practice rather than a WCAG failure, and wcag names the WCAG criteria an issue checks. Issues with neither needs_review nor best_practice are confirmed WCAG failures."; +/** How many rules and pages a web report's summary names; the rest are counted. */ +const SUMMARY_RULES = 20; +const SUMMARY_PAGES = 10; /** - * How many of a web report's issues still need a person to confirm them, and how many are - * best practices: counted over the whole report, not just this page. + * A web report's issues at a glance, all of them rather than one page's: by severity, how + * many a person still has to confirm, how many are best practices, by rule and by page. */ -function reviewCountsNote( - counts: ReviewCounts | undefined, +function webSummaryNote( + summary: WebIssueSummary | undefined, total: number, - records: { needs_review?: string; best_practice?: true }[], ): string[] { - const review = counts?.needs_review ?? 0; - const practice = counts?.best_practice ?? 0; + if (!summary || total === 0) return []; const one = (n: number, singular: string, plural: string) => n === 1 ? singular : plural; - const parts = [ + const rank = (severity: string) => { + const i = SEVERITY_ORDER.indexOf(severity); + return i === -1 ? SEVERITY_ORDER.length : i; + }; + const severities = Object.entries(summary.by_severity) + .sort(([a], [b]) => rank(a) - rank(b)) + .map(([severity, n]) => `${n} ${severity}`) + .join(", "); + const review = summary.needs_review; + const practice = summary.best_practice; + const notes = [ ...(review > 0 ? [ - `${review} ${one(review, "needs review (a person has to confirm it)", "need review (a person has to confirm them)")}`, + `${review} ${one(review, "needs", "need")} review (a person has to confirm ${one(review, "it", "them")})`, ] : []), ...(practice > 0 @@ -573,17 +582,40 @@ function reviewCountsNote( ] : []), ]; - const tagged = - parts.length > 0 || - records.some((record) => record.needs_review || record.best_practice); + const listed = ( + rows: T[], + limit: number, + label: (row: T) => string, + rest: string, + ) => { + const more = rows.length - Math.min(rows.length, limit); + return `${rows.slice(0, limit).map(label).join(", ")}${more > 0 ? `, and ${more} more ${rest}` : ""}.`; + }; + const pages = summary.by_page ?? []; return [ - ...(parts.length > 0 - ? [`Of the ${total} issues, ${parts.join(" and ")}.`] - : []), - ...(tagged ? [REVIEW_TAGS_NOTE] : []), + `Summary of the ${total} issues: ${severities}${notes.map((note) => `; ${note}`).join("")}.`, + wrapUntrusted( + "accessibility issue counts", + [ + `By rule: ${listed(summary.by_rule, SUMMARY_RULES, (r) => `${r.issue_type} ${r.count}`, "rules")}`, + ...(pages.length > 0 + ? [ + `By page: ${listed(pages, SUMMARY_PAGES, (p) => `${p.url} ${p.count}`, "pages")}`, + ] + : []), + ].join("\n"), + ), ]; } +const WEB_ISSUES_NOTE = + "Each issue names its rule (issue_type): the rule's name, description, WCAG criteria and usual fix are under rules, and an issue has its own how_to_fix only when it differs. An issue with needs_review is one the engine could not decide, so a person confirms it: the value is its review status, and until it is confirmed it is not a confirmed failure. An issue with no needs_review, whose rule is not a best practice (best_practice under rules), is a confirmed WCAG failure."; + +/** The CSV link, with what it is for: an answer gives a page of issues at a time. */ +function fullReportLine(reportLink: string): string { + return `The full list of issues is a CSV, valid for about 12 hours: download it when every issue is needed at once (this answer gives them a page at a time), from the following link: ${reportLink}`; +} + /** How many new and resolved issues a comparison lists per page. */ const COMPARE_LIST_MAX = 50; @@ -1002,6 +1034,7 @@ async function fetchAccessibilityIssues( const { records, + rules, summary, page_length, total_issues, @@ -1021,10 +1054,12 @@ async function fetchAccessibilityIssues( ...processingNote(processing), ...(page ? pageNote(page) : []), `Retrieved ${page_length} accessibility issues (Total: ${total_issues})`, - ...reviewCountsNote(summary, total_issues, records), + // The whole report's counts, said on its first page. + ...(cursor ? [] : webSummaryNote(summary, total_issues)), ...excludedIssuesNote(duplicate_issues, unlinked_duplicates, hidden_issues), - `You can also download the full report (CSV) from the following link, valid for about 12 hours: ${reportLink}`, - `Issues: ${wrapUntrusted("accessibility scan results", JSON.stringify(records, null, 2))}`, + fullReportLine(reportLink), + WEB_ISSUES_NOTE, + `Issues: ${wrapUntrusted("accessibility scan results", JSON.stringify({ issues: records, rules }))}`, closingMessage(next_page, remaining, source, stillProcessing, pageArg), ]); } @@ -1432,28 +1467,29 @@ function createScanFailureResponse( function createScanSuccessResponse( name: string, - totalIssues: number, - pageLength: number, - records: any[], + report: Awaited>, scanId: string, scanRunId: string, reportUrl: string, - cursor: number | null, - duplicates: string[] = [], + dashboardLink: string | undefined, settings: ScanSettings = {}, applied?: ScanSettings, - dashboardLink?: string, - reviewNotes: string[] = [], ): CallToolResult { + const cursor = report.next_page; const messages = [ `Accessibility scan "${name}" completed. ${scanDashboardNote(dashboardLink, scanId)}`, `Scan ID: ${scanId} and Scan Run ID: ${scanRunId}`, scanSettingsNote(settings, applied), - `You can also download the full report from the following link: ${reportUrl}`, - `We found ${totalIssues} issues. Below are the details of the ${pageLength} most critical issues.`, - ...reviewNotes, - ...duplicates, - `Scan results: ${wrapUntrusted("accessibility scan results", JSON.stringify(records, null, 2))}`, + fullReportLine(reportUrl), + `We found ${report.total_issues} issues. Below are the details of the ${report.page_length} most critical issues.`, + ...webSummaryNote(report.summary, report.total_issues), + ...excludedIssuesNote( + report.duplicate_issues, + report.unlinked_duplicates, + report.hidden_issues, + ), + WEB_ISSUES_NOTE, + `Scan results: ${wrapUntrusted("accessibility scan results", JSON.stringify({ issues: report.records, rules: report.rules }))}`, ]; if (cursor !== null) { @@ -1528,31 +1564,15 @@ async function runAccessibilityScan( throw error; } - const { - records, - summary, - page_length, - total_issues, - next_page, - duplicate_issues, - unlinked_duplicates, - hidden_issues, - } = await parseAccessibilityReportFromCSV(reportLink); - return createScanSuccessResponse( name, - total_issues, - page_length, - records, + await parseAccessibilityReportFromCSV(reportLink), scanId, scanRunId, reportLink, - next_page, - excludedIssuesNote(duplicate_issues, unlinked_duplicates, hidden_issues), + dashboardLink, settings, applied, - dashboardLink, - reviewCountsNote(summary, total_issues, records), ); } diff --git a/src/tools/accessiblity-utils/app-report-fetcher.ts b/src/tools/accessiblity-utils/app-report-fetcher.ts index ce4941ee..96c1f6c8 100644 --- a/src/tools/accessiblity-utils/app-report-fetcher.ts +++ b/src/tools/accessiblity-utils/app-report-fetcher.ts @@ -2,7 +2,7 @@ import { apiClient } from "../../lib/apiClient.js"; import { SessionType } from "../../lib/constants.js"; import { sessionsListUrl } from "../automate-utils/list-session-ids.js"; import { getAutomationBaseUrl } from "../rca-agent-utils/constants.js"; -import { pageOf, sortBySeverity } from "./paginate.js"; +import { mostCommon, pageOf, sortBySeverity } from "./paginate.js"; /** * App Accessibility: the issues of a mobile app build run through App Automate with @@ -390,18 +390,6 @@ export function flattenAppSnapshots( return sortBySeverity(rows); } -/** The value most rows give, the first of them on a tie. */ -function mostCommon(values: string[]): string { - const counts = new Map(); - for (const v of values) counts.set(v, (counts.get(v) ?? 0) + 1); - let best = ""; - let bestCount = 0; - for (const [v, n] of counts) { - if (n > bestCount) [best, bestCount] = [v, n]; - } - return best; -} - /** * An issue's name, description, WCAG criteria and fix are its rule's, and its screenshot is * its screen's: two thirds of an issue's length, repeated on every one. Said once, under diff --git a/src/tools/accessiblity-utils/paginate.ts b/src/tools/accessiblity-utils/paginate.ts index 3d2657ae..d0b66e1f 100644 --- a/src/tools/accessiblity-utils/paginate.ts +++ b/src/tools/accessiblity-utils/paginate.ts @@ -11,20 +11,37 @@ const SEVERITY_ORDER: Record = { minor: 3, }; +/** A severity's place, most severe first; an unknown severity sorts last. */ +export function severityRank(severity: string): number { + return SEVERITY_ORDER[severity] ?? 99; +} + /** Most severe first; an unknown severity sorts last. */ export function sortBySeverity(rows: T[]): T[] { return rows.sort( - (a, b) => - (SEVERITY_ORDER[a.severity] ?? 99) - (SEVERITY_ORDER[b.severity] ?? 99), + (a, b) => severityRank(a.severity) - severityRank(b.severity), ); } +/** The value most rows give, the first of them on a tie. */ +export function mostCommon(values: string[]): string { + const counts = new Map(); + for (const v of values) counts.set(v, (counts.get(v) ?? 0) + 1); + let best = ""; + let bestCount = 0; + for (const [v, n] of counts) { + if (n > bestCount) [best, bestCount] = [v, n]; + } + return best; +} + /** - * How many characters of issue JSON one call hands out by default: about 2,500 tokens, so - * a page stays small next to a client's own limit on tool output while a large report - * still takes few calls. + * How many characters of issue JSON one call hands out by default: about 8,000 tokens, under + * Claude Code's 10,000-token warning and its 50,000-character limit on a tool's result + * (above which the result is saved to a file instead of shown), so a large report takes + * few calls and a mid-size one fits in one. */ -export const PAGE_CHARACTERS = 10_000; +export const PAGE_CHARACTERS = 30_000; export interface Page { records: T[]; @@ -37,7 +54,19 @@ export interface Page { export function pageOf( all: T[], - { maxCharacterLength = PAGE_CHARACTERS, nextPage = 0 } = {}, + { + maxCharacterLength = PAGE_CHARACTERS, + nextPage = 0, + extraLength, + }: { + maxCharacterLength?: number; + nextPage?: number; + /** + * What else a row brings onto its page, counted against the page's length but not + * the cursor: the text of a rule the page has not named yet. + */ + extraLength?: (record: T) => number; + } = {}, ): Page { // Walk to the starting offset let charCursor = 0; @@ -51,12 +80,17 @@ export function pageOf( // Collect up to maxCharacterLength; a row larger than that still makes a page of its // own, or the cursor would never move past it const page: T[] = []; + let extra = 0; for (let i = idx; i < all.length; i++) { const recStr = JSON.stringify(all[i]); - const fits = charCursor - nextPage + recStr.length <= maxCharacterLength; + const more = extraLength?.(all[i]) ?? 0; + const fits = + charCursor - nextPage + extra + more + recStr.length <= + maxCharacterLength; if (!fits && page.length > 0) break; page.push(all[i]); charCursor += recStr.length; + extra += more; } const remaining = all.length - idx - page.length; diff --git a/src/tools/accessiblity-utils/report-parser.ts b/src/tools/accessiblity-utils/report-parser.ts index 7e3c696a..25db2258 100644 --- a/src/tools/accessiblity-utils/report-parser.ts +++ b/src/tools/accessiblity-utils/report-parser.ts @@ -1,7 +1,13 @@ import { apiClient } from "../../lib/apiClient.js"; import { parse } from "csv-parse/sync"; import logger from "../../logger.js"; -import { PAGE_CHARACTERS, pageOf, sortBySeverity } from "./paginate.js"; +import { + mostCommon, + PAGE_CHARACTERS, + pageOf, + severityRank, + sortBySeverity, +} from "./paginate.js"; type SimplifiedAccessibilityIssue = { issue_type: string; @@ -25,14 +31,36 @@ type SimplifiedAccessibilityIssue = { * "unreviewed" until someone does. */ needs_review?: string; - /** A best practice, not a WCAG failure. */ - best_practice?: true; +}; + +/** An issue as a page hands it out: its rule's text is under the page's rules. */ +type IssueRow = Omit< + SimplifiedAccessibilityIssue, + "issue_description" | "how_to_fix" +> & { + /** Only when it differs from its rule's description. */ + issue_description?: string; + /** Only when it differs from its rule's usual fix. */ + how_to_fix?: string; +}; + +/** A rule's text, the same on each of its issues. */ +type RuleText = { + name?: string; + description: string; /** The WCAG success criteria it checks, as the report writes them. */ wcag?: string; + /** A best practice, not a WCAG failure. */ + best_practice?: true; + how_to_fix: string; }; -/** Of all the issues read, how many a person still has to confirm, and how many are best practices. */ -export type ReviewCounts = { +export type IssueSummary = { + by_severity: Record; + /** Largest first. */ + by_rule: { issue_type: string; count: number }[]; + /** The page each issue was found on, most issues first. */ + by_page: { url: string; count: number }[]; /** Needs-review issues no one has confirmed yet. */ needs_review: number; best_practice: number; @@ -65,9 +93,11 @@ type PaginationOptions = { }; type PaginatedResult = { - records: SimplifiedAccessibilityIssue[]; - /** Counted over all the issues read, not just this page's. */ - summary: ReviewCounts; + records: IssueRow[]; + /** The rules this page's issues name. */ + rules: Record; + /** All the issues read, not just this page's. */ + summary: IssueSummary; /** Character offset for the next page, or null if done */ page_length: number; /** Main issues only when the report is grouped, so it matches the run's issue count. */ @@ -89,7 +119,6 @@ const DUPLICATE_COLUMN = "Duplicate Issue of"; const NEEDS_REVIEW_COLUMN = "Needs review"; const REVIEW_STATUS_COLUMN = "Needs Review Status"; const BEST_PRACTICE_COLUMN = "Best practice"; -const GUIDELINE_COLUMN = "Guideline(s)"; const ISSUE_LINK_COLUMN = "Issue link"; const STATUS_COLUMN = "Issue status"; @@ -114,6 +143,20 @@ function samePage(url: string): string { } } +/** + * How much of an issue's HTML snippet a page hands out. A snippet can be an element's whole + * markup (thousands of characters for a container), which took most of a page by itself; + * its opening tag, which says which element it is, comes first. + */ +const SNIPPET_CHARACTERS = 400; + +function shortSnippet(snippet: string | undefined): string { + const text = snippet ?? ""; + return text.length > SNIPPET_CHARACTERS + ? `${text.slice(0, SNIPPET_CHARACTERS)}… (${text.length - SNIPPET_CHARACTERS} more characters in the CSV)` + : text; +} + const isYes = (value: string | undefined) => (value ?? "").trim().toLowerCase() === "yes"; @@ -125,7 +168,7 @@ function recordOfRow( issue_type: row["Issue type"], component: row["Component"], issue_description: row["Issue description"], - HTML_snippet: row["HTML snippet"], + HTML_snippet: shortSnippet(row["HTML snippet"]), how_to_fix: row["How to fix this issue"], severity: (row["Severity"] || "unknown").trim(), affected_page: row["Affected page"], @@ -137,22 +180,138 @@ function recordOfRow( ).toLowerCase(), } : {}), - ...(isYes(row[BEST_PRACTICE_COLUMN]) - ? { best_practice: true as const } - : {}), - ...(row[GUIDELINE_COLUMN] ? { wcag: row[GUIDELINE_COLUMN] } : {}), ...(hasSourceTest ? { source_test: row[SOURCE_TEST_COLUMN] } : {}), }; } -/** How many of the issues still need a person to confirm them, and how many are best practices. */ -function reviewCountsOf(issues: SimplifiedAccessibilityIssue[]): ReviewCounts { +/** + * Each rule's name, description, WCAG criteria and usual fix, from the report's rows: the + * same on every issue of the rule, so said once under a page's rules. The usual fix is the + * one most of its issues give; some rules (colour contrast) write the element's own values + * into it. + */ +function rulesOf( + rows: Record[], + issues: SimplifiedAccessibilityIssue[], +): Record { + const fixes = new Map(); + for (const issue of issues) { + const list = fixes.get(issue.issue_type) ?? []; + list.push(issue.how_to_fix ?? ""); + fixes.set(issue.issue_type, list); + } + const rules: Record = {}; + for (const row of rows) { + const type = row["Issue type"]; + if (!type || rules[type]) continue; + rules[type] = { + ...(row["Issue name"] ? { name: row["Issue name"] } : {}), + description: row["Issue description"] ?? "", + ...(row["Guideline(s)"] ? { wcag: row["Guideline(s)"] } : {}), + ...(isYes(row[BEST_PRACTICE_COLUMN]) + ? { best_practice: true as const } + : {}), + how_to_fix: mostCommon( + fixes.get(type) ?? [row["How to fix this issue"] ?? ""], + ), + }; + } + return rules; +} + +/** An issue without its rule's text, keeping its own description or fix where they differ. */ +function withoutRuleText( + issue: SimplifiedAccessibilityIssue, + rule: RuleText | undefined, +): IssueRow { + const { issue_description, how_to_fix, ...own } = issue; + return { + ...own, + ...(issue_description !== rule?.description ? { issue_description } : {}), + ...(how_to_fix !== rule?.how_to_fix ? { how_to_fix } : {}), + }; +} + +/** + * The whole report's issues at a glance: by severity, rule and page, and how many to + * confirm. Pages are told apart as the report writes them, give or take host case, trailing + * slash and hash, and each is named by the first address the report gives it. + */ +function summaryOf( + issues: SimplifiedAccessibilityIssue[], + rules: Record, +): IssueSummary { + const bySeverity: Record = {}; + const byRule = new Map(); + const byPage = new Map(); + for (const issue of issues) { + bySeverity[issue.severity] = (bySeverity[issue.severity] ?? 0) + 1; + byRule.set(issue.issue_type, (byRule.get(issue.issue_type) ?? 0) + 1); + const url = issue.affected_page ?? ""; + const page = byPage.get(samePage(url)) ?? { url, count: 0 }; + page.count++; + byPage.set(samePage(url), page); + } return { + by_severity: bySeverity, + by_rule: [...byRule] + .map(([issue_type, count]) => ({ issue_type, count })) + .sort((a, b) => b.count - a.count), + by_page: [...byPage.values()].sort((a, b) => b.count - a.count), // A needs-review issue a person has confirmed is a confirmed failure. needs_review: issues.filter( (issue) => issue.needs_review && issue.needs_review !== "confirmed", ).length, - best_practice: issues.filter((issue) => issue.best_practice).length, + best_practice: issues.filter( + (issue) => rules[issue.issue_type]?.best_practice, + ).length, + }; +} + +/** + * One page of issues, each without its rule's text, with the rules that page names and the + * counts of all of them. Most severe first, and within a severity rule by rule, the most + * common rule first: a page then names fewer rules, and a rule's text, counted against the + * page's length the first time the page names it, is said once for all its issues there. + */ +function pageWithRules( + all: SimplifiedAccessibilityIssue[], + rows: Record[], + options: { maxCharacterLength: number; nextPage: number }, +) { + const rules = rulesOf(rows, all); + const summary = summaryOf(all, rules); + const ruleRank = new Map( + summary.by_rule.map(({ issue_type }, i) => [issue_type, i]), + ); + const ordered = all + .map((issue, i) => ({ issue, i })) + .sort( + (a, b) => + severityRank(a.issue.severity) - severityRank(b.issue.severity) || + (ruleRank.get(a.issue.issue_type) ?? 0) - + (ruleRank.get(b.issue.issue_type) ?? 0) || + a.i - b.i, + ) + .map(({ issue }) => withoutRuleText(issue, rules[issue.issue_type])); + const named = new Set(); + const page = pageOf(ordered, { + ...options, + extraLength: (row) => { + if (named.has(row.issue_type)) return 0; + named.add(row.issue_type); + return JSON.stringify(rules[row.issue_type] ?? {}).length; + }, + }); + return { + ...page, + rules: Object.fromEntries( + [...new Set(page.records.map((r) => r.issue_type))].map((type) => [ + type, + rules[type], + ]), + ), + summary, }; } @@ -420,13 +579,9 @@ export async function parseAccessibilityReportFromCSV( if (pageUrl) { const { all, page } = pageIssues(parsed, columns, pageUrl); - const slice = pageOf(sortBySeverity(all), { - maxCharacterLength, - nextPage, - }); + const slice = pageWithRules(all, parsed, { maxCharacterLength, nextPage }); return { ...slice, - summary: reviewCountsOf(all), total_issues: all.length, duplicate_issues: null, unlinked_duplicates: 0, @@ -490,12 +645,11 @@ export async function parseAccessibilityReportFromCSV( else unlinkedDuplicates++; } - // 3) Most severe first, in pages by JSON length - const page = pageOf(sortBySeverity(all), { maxCharacterLength, nextPage }); + // 3) Most severe first, in pages by JSON length, each rule's text once per page + const page = pageWithRules(all, parsed, { maxCharacterLength, nextPage }); return { ...page, - summary: reviewCountsOf(all), total_issues: all.length, duplicate_issues: grouped ? duplicates : null, unlinked_duplicates: unlinkedDuplicates, diff --git a/tests/tools/accessibilityIssueRules.test.ts b/tests/tools/accessibilityIssueRules.test.ts index 26c1ec10..c99e4652 100644 --- a/tests/tools/accessibilityIssueRules.test.ts +++ b/tests/tools/accessibilityIssueRules.test.ts @@ -1,8 +1,8 @@ import { beforeEach, describe, expect, it, Mock, vi } from "vitest"; -// A web report's issues say which ones a person still has to confirm ("needs review"), which -// are best practices rather than WCAG failures, and the WCAG criteria each checks. Without -// them every issue read as a confirmed WCAG failure. +// A web report's issues say which ones a person still has to confirm ("needs review"), and a +// page names each rule's text once, under rules, rather than on every issue: the rule's +// description and usual fix were more than half of an issue's length, so a page held 10-13. vi.mock("../../src/lib/apiClient.js", () => ({ apiClient: { get: vi.fn(), post: vi.fn() }, @@ -97,7 +97,7 @@ function toCsv(rows: Record[]): string { .join("\n"); } -describe("a web report's needs-review and best-practice issues", () => { +describe("a web report's issues, rules and counts", () => { beforeEach(() => { get.mockReset(); get.mockResolvedValue({ ok: true, statusText: "OK", data: toCsv(ROWS) }); @@ -113,91 +113,182 @@ describe("a web report's needs-review and best-practice issues", () => { } }); - it("marks a best-practice issue, which is not a WCAG failure, and gives each issue its WCAG criteria", async () => { - const { records } = await parseAccessibilityReportFromCSV("https://csv"); + it("does not count a needs-review issue a person has confirmed as still needing review", async () => { + get.mockResolvedValue({ + ok: true, + statusText: "OK", + data: toCsv([ + ...ROWS, + row({ "Issue type": "label", Severity: "serious", "Issue description": "l", "How to fix this issue": "f", "Needs review": "Yes", "Needs Review Status": "Confirmed" }), + ]), + }); - const region = records.find((r) => r.issue_type === "region"); - expect(region).toMatchObject({ best_practice: true, wcag: "Best Practice" }); - for (const r of records.filter((r) => r.issue_type !== "region")) { - expect(r).not.toHaveProperty("best_practice"); - } - expect(records.find((r) => r.issue_type === "image-alt")?.wcag).toBe("SC 1.1.1"); + const { records, summary } = await parseAccessibilityReportFromCSV("https://csv"); + + expect(records.find((r) => r.issue_type === "label")?.needs_review).toBe("confirmed"); + expect(summary.needs_review).toBe(1); }); - it("adds no WCAG criteria when the report gives none", async () => { + it("reads a review status written with spaces around it, and an empty one as unreviewed", async () => { get.mockResolvedValue({ ok: true, statusText: "OK", - data: toCsv([row({ "Issue type": "label", Severity: "serious", "Issue description": "l", "How to fix this issue": "f", "Guideline(s)": "" })]), + data: toCsv([ + row({ "Issue type": "label", Severity: "serious", "Issue description": "l", "How to fix this issue": "f", "Needs review": "Yes", "Needs Review Status": " Confirmed " }), + row({ "Issue type": "region", Severity: "moderate", "Issue description": "r", "How to fix this issue": "f", "Needs review": "Yes", "Needs Review Status": " " }), + ]), }); - const { records } = await parseAccessibilityReportFromCSV("https://csv"); + const { records, summary } = await parseAccessibilityReportFromCSV("https://csv"); - expect(records[0]).not.toHaveProperty("wcag"); + expect(records.find((r) => r.issue_type === "label")?.needs_review).toBe("confirmed"); + expect(records.find((r) => r.issue_type === "region")?.needs_review).toBe("unreviewed"); + expect(summary.needs_review).toBe(1); }); - it("keeps each issue's own description and fix", async () => { - const { records } = await parseAccessibilityReportFromCSV("https://csv"); + it("names each rule's text once, under rules, and keeps an issue's own fix only when it differs", async () => { + const { records, rules } = await parseAccessibilityReportFromCSV("https://csv"); + expect(rules["color-contrast"]).toEqual({ + name: "Elements must meet minimum color contrast ratio thresholds", + description: "Ensure the contrast between foreground and background colors meets WCAG thresholds", + wcag: "SC 1.4.3", + how_to_fix: "Raise the contrast to 4.5:1 (now 2.1)", + }); const contrastIssues = records.filter((r) => r.issue_type === "color-contrast"); expect(contrastIssues).toHaveLength(3); + for (const r of records) expect(r).not.toHaveProperty("issue_description"); + expect(contrastIssues.map((r) => r.how_to_fix ?? null).sort()).toEqual([ + "Raise the contrast to 4.5:1 (now 3.0)", + null, + null, + ]); + }); + + it("keeps a rule's WCAG criteria and best-practice mark on the rule, not on each issue", async () => { + const { records } = await parseAccessibilityReportFromCSV("https://csv"); + for (const r of records) { - expect(r.issue_description).toBeTruthy(); - expect(r.how_to_fix).toBeTruthy(); + expect(r).not.toHaveProperty("wcag"); + expect(r).not.toHaveProperty("best_practice"); } }); - it("counts the needs-review and best-practice issues of the whole report, not just the page", async () => { - const { records, summary, next_page } = await parseAccessibilityReportFromCSV("https://csv", { - maxCharacterLength: 1, + it("counts issues by page, the page with most first", async () => { + get.mockResolvedValue({ + ok: true, + statusText: "OK", + data: toCsv([ + row({ "Issue type": "label", Severity: "serious", "Issue description": "l", "How to fix this issue": "f", "Affected page": "https://site.example/a" }), + row({ "Issue type": "label", Severity: "serious", "Issue description": "l", "How to fix this issue": "f", "Affected page": "https://site.example/b" }), + row({ "Issue type": "region", Severity: "moderate", "Issue description": "r", "How to fix this issue": "f", "Affected page": "https://site.example/b" }), + ]), }); + const { summary } = await parseAccessibilityReportFromCSV("https://csv"); + + expect(summary.by_page).toEqual([ + { url: "https://site.example/b", count: 2 }, + { url: "https://site.example/a", count: 1 }, + ]); + }); + + it("marks a best-practice rule, which is not a WCAG failure", async () => { + const { rules } = await parseAccessibilityReportFromCSV("https://csv"); + + expect(rules.region).toMatchObject({ best_practice: true, wcag: "Best Practice" }); + expect(rules["color-contrast"]).not.toHaveProperty("best_practice"); + }); + + it("counts the whole report, while a page names only its own issues' rules", async () => { + const { records, rules, summary, next_page } = await parseAccessibilityReportFromCSV( + "https://csv", + { maxCharacterLength: 1 }, + ); + expect(records).toHaveLength(1); expect(next_page).not.toBeNull(); - expect(summary).toEqual({ needs_review: 1, best_practice: 1 }); + expect(Object.keys(rules)).toEqual([records[0].issue_type]); + expect(summary).toEqual({ + by_severity: { critical: 1, serious: 3, moderate: 1 }, + by_rule: [ + { issue_type: "color-contrast", count: 3 }, + { issue_type: "image-alt", count: 1 }, + { issue_type: "region", count: 1 }, + ], + by_page: [{ url: "https://site.example/", count: 5 }], + needs_review: 1, + best_practice: 1, + }); }); - it("does not count a needs-review issue a person has confirmed as still needing review", async () => { + it("keeps a page within its length, its rules' text counted once", async () => { + for (const maxCharacterLength of [900, 1500, 2500]) { + const { records, rules } = await parseAccessibilityReportFromCSV("https://csv", { + maxCharacterLength, + }); + if (records.length < 2) continue; + const length = + records.reduce((sum, r) => sum + JSON.stringify(r).length, 0) + + Object.values(rules).reduce((sum, r) => sum + JSON.stringify(r).length, 0); + expect(length).toBeLessThanOrEqual(maxCharacterLength); + } + }); + + it("lists a severity's issues rule by rule, the most common rule first", async () => { get.mockResolvedValue({ ok: true, statusText: "OK", data: toCsv([ - ...ROWS, - row({ "Issue type": "label", Severity: "serious", "Issue description": "l", "How to fix this issue": "f", "Needs review": "Yes", "Needs Review Status": "Confirmed" }), + row({ "Issue type": "label", Severity: "serious", "Issue description": "l", "How to fix this issue": "f" }), + contrast("c"), + row({ "Issue type": "label", Severity: "serious", "Issue description": "l", "How to fix this issue": "f" }), + contrast("c"), + row({ "Issue type": "image-alt", Severity: "critical", "Issue description": "a", "How to fix this issue": "f" }), + contrast("c"), ]), }); - const { records, summary } = await parseAccessibilityReportFromCSV("https://csv"); + const { records } = await parseAccessibilityReportFromCSV("https://csv"); - expect(records.find((r) => r.issue_type === "label")?.needs_review).toBe("confirmed"); - expect(summary.needs_review).toBe(1); + expect(records.map((r) => r.issue_type)).toEqual([ + "image-alt", + "color-contrast", + "color-contrast", + "color-contrast", + "label", + "label", + ]); }); - it("reads a review status written with spaces around it, and an empty one as unreviewed", async () => { + it("shortens a very long HTML snippet, saying how much the CSV has", async () => { + const long = `
${"offer".repeat(200)}
`; get.mockResolvedValue({ ok: true, statusText: "OK", data: toCsv([ - row({ "Issue type": "label", Severity: "serious", "Issue description": "l", "How to fix this issue": "f", "Needs review": "Yes", "Needs Review Status": " Confirmed " }), - row({ "Issue type": "region", Severity: "moderate", "Issue description": "r", "How to fix this issue": "f", "Needs review": "Yes", "Needs Review Status": " " }), + row({ "Issue type": "region", Severity: "moderate", "Issue description": "r", "How to fix this issue": "f", "HTML snippet": long }), + row({ "Issue type": "label", Severity: "serious", "Issue description": "l", "How to fix this issue": "f", "HTML snippet": "" }), ]), }); - const { records, summary } = await parseAccessibilityReportFromCSV("https://csv"); + const { records } = await parseAccessibilityReportFromCSV("https://csv"); - expect(records.find((r) => r.issue_type === "label")?.needs_review).toBe("confirmed"); - expect(records.find((r) => r.issue_type === "region")?.needs_review).toBe("unreviewed"); - expect(summary.needs_review).toBe(1); + const region = records.find((r) => r.issue_type === "region")!; + expect(region.HTML_snippet.startsWith('
offer')).toBe(true); + expect(region.HTML_snippet.length).toBeLessThan(500); + expect(region.HTML_snippet).toMatch(new RegExp(`… \\(${long.length - 400} more characters in the CSV\\)$`)); + expect(records.find((r) => r.issue_type === "label")!.HTML_snippet).toBe(""); }); it("does the same when the report is read for one page", async () => { - const { records, summary } = await parseAccessibilityReportFromCSV("https://csv", { + const { records, rules } = await parseAccessibilityReportFromCSV("https://csv", { pageUrl: "https://site.example/", }); expect(records).toHaveLength(5); expect(records.find((r) => r.issue_type === "image-alt")?.needs_review).toBe("unreviewed"); - expect(records.find((r) => r.issue_type === "region")?.best_practice).toBe(true); - expect(summary).toEqual({ needs_review: 1, best_practice: 1 }); + expect(Object.keys(rules).sort()).toEqual(["color-contrast", "image-alt", "region"]); + for (const r of records) expect(r).not.toHaveProperty("issue_description"); }); }); diff --git a/tests/tools/accessibilityPageSize.test.ts b/tests/tools/accessibilityPageSize.test.ts index 596340d8..8ccc348f 100644 --- a/tests/tools/accessibilityPageSize.test.ts +++ b/tests/tools/accessibilityPageSize.test.ts @@ -1,7 +1,8 @@ import { describe, expect, it, Mock, vi } from "vitest"; -// How much of a report one call hands out by default: about 10,000 characters of issue -// JSON, whatever the report, so a large report takes half as many calls as at 5,000. +// How much of a report one call hands out by default: about 30,000 characters of issue +// JSON (about 8k tokens), whatever the report: under Claude Code's 10k-token warning and its +// 50,000-character limit, at a third of the calls 10,000 took. vi.mock("../../src/lib/apiClient.js", () => ({ apiClient: { get: vi.fn() }, @@ -14,10 +15,10 @@ import { apiClient } from "../../src/lib/apiClient.js"; import { pageOf } from "../../src/tools/accessiblity-utils/paginate.js"; import { parseAccessibilityReportFromCSV } from "../../src/tools/accessiblity-utils/report-parser.js"; -const row = (id: number) => ({ id, text: "x".repeat(3000) }); +const row = (id: number) => ({ id, text: "x".repeat(9000) }); describe("the default page size", () => { - it("fills a page up to 10,000 characters", () => { + it("fills a page up to 30,000 characters", () => { const page = pageOf([row(1), row(2), row(3), row(4)]); expect(page.records.map((r) => r.id)).toEqual([1, 2, 3]); @@ -25,9 +26,11 @@ describe("the default page size", () => { }); it("is the same for a CSV report", async () => { + // Each issue's rule text, its fix here, counts against the page the first time the page + // names the rule. (A long HTML snippet is cut short, so it cannot fill a page.) const header = '"Issue type","Severity","Component","Issue description","Affected page","HTML snippet","How to fix this issue"'; const line = (n: number) => - `"rule-${n}","serious","div","d","https://shop.example/","${"x".repeat(3000)}","fix"`; + `"rule-${n}","serious","div","d","https://shop.example/","
","${"x".repeat(9000)}"`; (apiClient.get as Mock).mockResolvedValue({ ok: true, statusText: "OK", @@ -43,11 +46,11 @@ describe("the default page size", () => { describe("a record longer than a page", () => { it("is handed out on a page of its own, so the cursor moves past it", () => { - const first = pageOf([row(1), { id: 2, text: "x".repeat(12_000) }, row(3)]); - const second = pageOf([row(1), { id: 2, text: "x".repeat(12_000) }, row(3)], { + const first = pageOf([row(1), { id: 2, text: "x".repeat(40_000) }, row(3)]); + const second = pageOf([row(1), { id: 2, text: "x".repeat(40_000) }, row(3)], { nextPage: first.next_page!, }); - const third = pageOf([row(1), { id: 2, text: "x".repeat(12_000) }, row(3)], { + const third = pageOf([row(1), { id: 2, text: "x".repeat(40_000) }, row(3)], { nextPage: second.next_page!, }); diff --git a/tests/tools/accessibilityReportPages.test.ts b/tests/tools/accessibilityReportPages.test.ts index e3034cf4..b7e8824d 100644 --- a/tests/tools/accessibilityReportPages.test.ts +++ b/tests/tools/accessibilityReportPages.test.ts @@ -132,8 +132,10 @@ describe("parseAccessibilityReportFromCSV for one page", () => { describe("parseAccessibilityReportFromCSV paging", () => { beforeEach(() => vi.clearAllMocks()); - it("hands out about 10,000 characters of issues per call by default", async () => { - serve([1, 2, 3, 4].map((n) => row({ "Issue type": `rule-${n}`, Component: "div", "Affected page": HOME, "Issue link": link(`p${n}`), "HTML snippet": "x".repeat(3000) }))); + // Rule text (here, each rule's fix) counts against the page the first time the page names + // the rule; a long HTML snippet is cut short, so it cannot fill a page. + it("hands out about 30,000 characters of issues per call by default", async () => { + serve([1, 2, 3, 4].map((n) => row({ "Issue type": `rule-${n}`, Component: "div", "Affected page": HOME, "Issue link": link(`p${n}`), "How to fix this issue": "x".repeat(9000) }))); const page = await parseAccessibilityReportFromCSV("https://csv"); @@ -144,7 +146,7 @@ describe("parseAccessibilityReportFromCSV paging", () => { it("still returns an issue larger than a page, so paging never stalls", async () => { serve([ row({ "Issue type": "small", Component: "a", "Affected page": HOME, "Issue link": link("s") }), - row({ "Issue type": "huge", Component: "div", "Affected page": HOME, "Issue link": link("h"), "HTML snippet": "x".repeat(8000) }), + row({ "Issue type": "huge", Component: "div", "Affected page": HOME, "Issue link": link("h"), "How to fix this issue": "x".repeat(40_000) }), row({ "Issue type": "after", Component: "b", "Affected page": HOME, "Issue link": link("t") }), ]); diff --git a/tests/tools/accessibilityScanOutput.test.ts b/tests/tools/accessibilityScanOutput.test.ts index c73932d7..0a8084c1 100644 --- a/tests/tools/accessibilityScanOutput.test.ts +++ b/tests/tools/accessibilityScanOutput.test.ts @@ -262,7 +262,7 @@ describe("fetchAccessibilityIssues sources", () => { `Use fetchAccessibilityIssues with ${nextPageArgs} and cursor: 140`, ); expect(body).toContain( - "download the full report (CSV) from the following link", + "The full list of issues is a CSV, valid for about 12 hours", ); expect(body).toContain("https://csv\n"); }, @@ -823,21 +823,37 @@ describe("fetchAccessibilityIssues and App Accessibility manual reports", () => }); }); -// A web report's answer says how many issues still need a person to confirm them and how many -// are best practices, over the whole report; and a scan's answer links the scan's own page. +// A web report's answer counts the whole report, says which issues a person still has to +// confirm, and names each rule's text once; and a scan's answer links the scan's own page. describe("what a web report's answer says about its issues", () => { let handlers: Record any>; - const report = (needs_review: number, best_practice: number, total_issues = 5) => ({ - records: [{ issue_type: "image-alt", severity: "critical", needs_review: "unreviewed", wcag: "SC 1.1.1" }], - summary: { needs_review, best_practice }, + const REPORT = { + records: [{ issue_type: "image-alt", severity: "critical", needs_review: "unreviewed" }], + rules: { + "image-alt": { name: "Images must have alternative text", description: "d", wcag: "SC 1.1.1", how_to_fix: "Add alt" }, + }, + summary: { + by_severity: { critical: 1, serious: 3, moderate: 1 }, + by_rule: [ + { issue_type: "color-contrast", count: 3 }, + { issue_type: "image-alt", count: 1 }, + { issue_type: "region", count: 1 }, + ], + by_page: [ + { url: "https://shop.example/", count: 3 }, + { url: "https://shop.example/cart", count: 2 }, + ], + needs_review: 1, + best_practice: 1, + }, page_length: 1, - total_issues, + total_issues: 5, next_page: 90, - remaining: total_issues - 1, + remaining: 4, duplicate_issues: null, unlinked_duplicates: 0, hidden_issues: null, - }); + }; beforeEach(() => { vi.clearAllMocks(); @@ -845,56 +861,78 @@ describe("what a web report's answer says about its issues", () => { m.waitUntilComplete.mockResolvedValue("completed"); m.getReportLink.mockResolvedValue("https://csv"); m.reportStatus.mockResolvedValue(undefined); - m.parse.mockResolvedValue(report(1, 1)); + m.parse.mockResolvedValue(REPORT); handlers = register(); }); - const fetchBody = async () => - text(await handlers.fetchAccessibilityIssues({ scanId: "scan-42", scanRunId: "run-7" }, CONTEXT)); - - it("says how many issues need review and how many are best practices, and what the tags mean", async () => { - const body = await fetchBody(); + it("counts the whole report, says what needs review means, and gives the rules once", async () => { + const body = text( + await handlers.fetchAccessibilityIssues({ scanId: "scan-42", scanRunId: "run-7" }, CONTEXT), + ); expect(body).toContain( - "Of the 5 issues, 1 needs review (a person has to confirm it) and 1 is a best practice, not a WCAG failure.", + "Summary of the 5 issues: 1 critical, 3 serious, 1 moderate; 1 needs review (a person has to confirm it); 1 is a best practice, not a WCAG failure.", ); - expect(body).toContain("needs_review marks an issue the engine could not decide"); + expect(body).toMatch(/By rule: color-contrast 3, image-alt 1, region 1\./); + expect(body).toMatch(/By page: https:\/\/shop\.example\/ 3, https:\/\/shop\.example\/cart 2\./); + expect(body).toContain("needs_review is one the engine could not decide, so a person confirms it"); // A best-practice issue has no needs_review either, so the note must not call it a failure. - expect(body).toContain("Issues with neither needs_review nor best_practice are confirmed WCAG failures."); + expect(body).toContain("An issue with no needs_review, whose rule is not a best practice (best_practice under rules), is a confirmed WCAG failure."); expect(body).not.toContain("Issues without needs_review are confirmed failures."); - expect(body).toContain('"needs_review": "unreviewed"'); - expect(body).toContain('"wcag": "SC 1.1.1"'); + expect(body).toContain('"rules"'); + expect(body).toContain('"needs_review":"unreviewed"'); }); - it("words the counts for several issues", async () => { - m.parse.mockResolvedValue(report(161, 191, 527)); + it("words the counts for several issues, and leaves out a count that is zero", async () => { + m.parse.mockResolvedValue({ ...REPORT, total_issues: 527, summary: { ...REPORT.summary, needs_review: 161, best_practice: 0 } }); - expect(await fetchBody()).toContain( - "Of the 527 issues, 161 need review (a person has to confirm them) and 191 are best practices, not WCAG failures.", + const body = text( + await handlers.fetchAccessibilityIssues({ scanId: "scan-42", scanRunId: "run-7" }, CONTEXT), ); + + expect(body).toContain("Summary of the 527 issues: 1 critical, 3 serious, 1 moderate; 161 need review (a person has to confirm them)."); + expect(body).not.toMatch(/is a best practice, not|are best practices, not/); }); - it("leaves out a count that is zero, and says nothing when both are", async () => { - m.parse.mockResolvedValue(report(0, 2)); - const some = await fetchBody(); - expect(some).toContain("Of the 5 issues, 2 are best practices, not WCAG failures."); - expect(some).not.toMatch(/need(s)? review \(/); + it("names the 10 pages with most issues, and counts the rest", async () => { + const by_page = Array.from({ length: 12 }, (_, i) => ({ url: `https://shop.example/p${i}`, count: 12 - i })); + m.parse.mockResolvedValue({ ...REPORT, summary: { ...REPORT.summary, by_page } }); - m.parse.mockResolvedValue({ ...report(0, 0), records: [{ issue_type: "label", severity: "serious" }] }); - const none = await fetchBody(); - expect(none).not.toContain("Of the 5 issues"); - expect(none).not.toContain("needs_review marks"); + const body = text( + await handlers.fetchAccessibilityIssues({ scanId: "scan-42", scanRunId: "run-7" }, CONTEXT), + ); + + expect(body).toContain("https://shop.example/p9 3, and 2 more pages."); + expect(body).not.toContain("https://shop.example/p10 "); }); - it("gives the same counts in a scan's answer", async () => { + it("gives the same overview in a scan's answer", async () => { const body = text( await handlers.startAccessibilityScan({ name: "home", pageURL: "https://site.example" }, CONTEXT), ); - expect(body).toContain( - "Of the 5 issues, 1 needs review (a person has to confirm it) and 1 is a best practice, not a WCAG failure.", + expect(body).toContain("Summary of the 5 issues: 1 critical, 3 serious, 1 moderate;"); + expect(body).toMatch(/By page: https:\/\/shop\.example\/ 3/); + }); + + it("gives the whole report's counts on its first page only", async () => { + const body = text( + await handlers.fetchAccessibilityIssues({ scanId: "scan-42", scanRunId: "run-7", cursor: 90 }, CONTEXT), ); - expect(body.match(/following link: (\S+)/)?.[1]).toBe("https://csv"); + + expect(body).not.toContain("Summary of the 5 issues"); + expect(body).toContain("Retrieved 1 accessibility issues (Total: 5)"); + }); + + it("says the CSV holds every issue, for an agent that needs them all at once", async () => { + for (const result of [ + await handlers.fetchAccessibilityIssues({ scanId: "scan-42", scanRunId: "run-7" }, CONTEXT), + await handlers.startAccessibilityScan({ name: "home", pageURL: "https://site.example" }, CONTEXT), + ]) { + const body = text(result); + expect(body).toMatch(/full list of issues is a CSV.*every issue is needed at once/s); + expect(body.match(/following link: (\S+)/)?.[1]).toBe("https://csv"); + } }); it("links the scan's own dashboard page, from the scans list, not a link made from its name", async () => {