diff --git a/.github/actions/file/README.md b/.github/actions/file/README.md index 7680cce..fd956e4 100644 --- a/.github/actions/file/README.md +++ b/.github/actions/file/README.md @@ -11,6 +11,7 @@ Files GitHub issues to track potential accessibility gaps. **Required** Path to a JSON file containing the list of potential accessibility gaps. The path can be absolute or relative to the working directory (which defaults to `GITHUB_WORKSPACE`). For example: `findings.json`. The file should contain a JSON array of finding objects. For example: + ```json [] ``` @@ -28,15 +29,31 @@ The file should contain a JSON array of finding objects. For example: **Optional** Path to a JSON file containing cached filings from previous runs. The path can be absolute or relative to the working directory (which defaults to `GITHUB_WORKSPACE`). Without this, duplicate issues may be filed. For example: `cached-filings.json`. The file should contain a JSON array of filing objects. For example: + ```json [ { "findings": [], - "issue": {"id":1,"nodeId":"SXNzdWU6MQ==","url":"https://github.com/github/docs/issues/123","title":"Accessibility issue: 1"} + "issue": { + "id": 1, + "nodeId": "SXNzdWU6MQ==", + "url": "https://github.com/github/docs/issues/123", + "title": "Accessibility issue: 1" + } } ] ``` +#### `group_by` + +**Optional** How to consolidate findings into issues. One of: + +- `finding` (default): one issue per individual violation — current behavior, unchanged. +- `rule`: one issue per rule (`ruleId`/`scannerType`), aggregating every occurrence across all scanned URLs. +- `rule+url`: one issue per rule per scanned URL. + +When grouping, each additional occurrence is appended to the single "umbrella" issue body as a checklist item under an **Occurrences** section rather than spawning a new issue. This is the preferred mechanism for consolidating issues over `open_grouped_issues`. + ### Outputs #### `filings_file` @@ -44,11 +61,17 @@ The file should contain a JSON array of filing objects. For example: Absolute path to a JSON file containing the list of issues filed (and their associated finding(s)). The action writes this file to a temporary directory and returns the absolute path. For example: `$RUNNER_TEMP/filings-.json`. The file will contain a JSON array of filing objects. For example: + ```json [ { "findings": [], - "issue": {"id":1,"nodeId":"SXNzdWU6MQ==","url":"https://github.com/github/docs/issues/123","title":"Accessibility issue: 1"} + "issue": { + "id": 1, + "nodeId": "SXNzdWU6MQ==", + "url": "https://github.com/github/docs/issues/123", + "title": "Accessibility issue: 1" + } } ] ``` diff --git a/.github/actions/file/action.yml b/.github/actions/file/action.yml index 6d22925..a0f1a80 100644 --- a/.github/actions/file/action.yml +++ b/.github/actions/file/action.yml @@ -1,21 +1,21 @@ -name: "File" -description: "Files GitHub issues to track potential accessibility gaps." +name: 'File' +description: 'Files GitHub issues to track potential accessibility gaps.' inputs: findings_file: - description: "Path to a JSON file containing the list of potential accessibility gaps" + description: 'Path to a JSON file containing the list of potential accessibility gaps' required: true repository: - description: "Repository (with owner) to file issues in" + description: 'Repository (with owner) to file issues in' required: true token: description: "Token with fine-grained permission 'issues: write'" required: true base_url: - description: "Optional base URL to pass into Octokit for the GitHub API (for example, `https://YOUR_HOSTNAME/api/v3` for GitHub Enterprise Server)" + description: 'Optional base URL to pass into Octokit for the GitHub API (for example, `https://YOUR_HOSTNAME/api/v3` for GitHub Enterprise Server)' required: false cached_filings_file: - description: "Path to a JSON file containing cached filings from previous runs. Without this, duplicate issues may be filed." + description: 'Path to a JSON file containing cached filings from previous runs. Without this, duplicate issues may be filed.' required: false screenshot_repository: description: "Repository (with owner) where screenshots are stored on the gh-cache branch. Defaults to the 'repository' input if not set. Required if issues are open in a different repo to construct proper screenshot URLs." @@ -23,28 +23,32 @@ inputs: open_grouped_issues: description: "In the 'file' step, also open grouped issues which link to all issues with the same root cause" required: false - default: "false" + default: 'false' + group_by: + description: "How to group findings into issues: 'finding' (one issue per violation, default), 'rule' (one issue per rule), or 'rule+url' (one issue per rule per scanned URL)." + required: false + default: 'finding' dry_run: - description: "When true, log the issues that would be filed without opening, closing, or reopening any issues." + description: 'When true, log the issues that would be filed without opening, closing, or reopening any issues.' required: false - default: "false" + default: 'false' file_best_practice_issues: - description: "File issues for best-practice findings (accessibility recommendations that are not hard WCAG failures). Disabling only suppresses new issues; existing ones are left untouched." + description: 'File issues for best-practice findings (accessibility recommendations that are not hard WCAG failures). Disabling only suppresses new issues; existing ones are left untouched.' required: false - default: "true" + default: 'true' file_experimental_issues: - description: "File issues for experimental findings (checks that are not yet stable). Disabling only suppresses new issues; existing ones are left untouched." + description: 'File issues for experimental findings (checks that are not yet stable). Disabling only suppresses new issues; existing ones are left untouched.' required: false - default: "true" + default: 'true' outputs: filings_file: - description: "Path to a JSON file containing the list of issues filed (and their associated finding(s))" + description: 'Path to a JSON file containing the list of issues filed (and their associated finding(s))' runs: - using: "node24" - main: "bootstrap.js" + using: 'node24' + main: 'bootstrap.js' branding: - icon: "compass" - color: "blue" + icon: 'compass' + color: 'blue' diff --git a/.github/actions/file/src/generateIssueBody.ts b/.github/actions/file/src/generateIssueBody.ts index f0167ff..c67024d 100644 --- a/.github/actions/file/src/generateIssueBody.ts +++ b/.github/actions/file/src/generateIssueBody.ts @@ -1,6 +1,9 @@ import type {Finding} from './types.d.js' -export function generateIssueBody(finding: Finding, screenshotRepo: string): string { +export function generateIssueBody(occurrences: Finding | Finding[], screenshotRepo: string): string { + const findings = Array.isArray(occurrences) ? occurrences : [occurrences] + const finding = findings[0] + const solutionLong = finding.solutionLong ?.split('\n') .map((line: string) => @@ -18,6 +21,16 @@ export function generateIssueBody(finding: Finding, screenshotRepo: string): str ` } + let occurrencesSection = '' + if (findings.length > 1) { + const items = findings.map(f => `- [ ] ${f.html ? `\`${f.html}\` on ${f.url}` : f.url}`).join('\n') + occurrencesSection = ` +## ${findings.length} Other Occurrences: + +${items} +` + } + const categoryNotice = finding.category && finding.category !== 'wcag' ? `> [!NOTE]\n> This is ${ @@ -42,7 +55,7 @@ An accessibility scan ${finding.html ? `flagged the element \`${finding.html}\`` ${screenshotSection ?? ''} To fix this, ${finding.solutionShort}. ${solutionLong ? `\nSpecifically:\n\n${solutionLong}` : ''} - +${occurrencesSection} ${acceptanceCriteria} ` diff --git a/.github/actions/file/src/groupBy.ts b/.github/actions/file/src/groupBy.ts new file mode 100644 index 0000000..35d2c7b --- /dev/null +++ b/.github/actions/file/src/groupBy.ts @@ -0,0 +1,7 @@ +export const GROUP_BY_VALUES = ['finding', 'rule', 'rule+url'] as const + +export type GroupBy = (typeof GROUP_BY_VALUES)[number] + +export function isGroupBy(value: string): value is GroupBy { + return (GROUP_BY_VALUES as readonly string[]).includes(value) +} diff --git a/.github/actions/file/src/index.ts b/.github/actions/file/src/index.ts index 13a3306..b4f989c 100644 --- a/.github/actions/file/src/index.ts +++ b/.github/actions/file/src/index.ts @@ -14,6 +14,7 @@ import {getWontfixIssueNumbers, shouldReopenIssue, WONTFIX_LABEL} from './should import {openIssue} from './openIssue.js' import {reopenIssue} from './reopenIssue.js' import {updateFilingsWithNewFindings} from './updateFilingsWithNewFindings.js' +import {GROUP_BY_VALUES, isGroupBy} from './groupBy.js' import {OctokitResponse} from '@octokit/types' const OctokitWithThrottling = Octokit.plugin(throttling) @@ -36,6 +37,12 @@ export default async function () { ? JSON.parse(fs.readFileSync(cachedFilingsFile, 'utf8')) : [] const shouldOpenGroupedIssues = core.getBooleanInput('open_grouped_issues') + const groupByInput = core.getInput('group_by') || 'finding' + if (!isGroupBy(groupByInput)) { + core.setFailed(`Invalid 'group_by' value: '${groupByInput}'. Must be one of: ${GROUP_BY_VALUES.join(', ')}.`) + return + } + const groupBy = groupByInput const dryRun = core.getBooleanInput('dry_run') const fileBestPracticeIssues = getBooleanInputWithDefault('file_best_practice_issues', true) const fileExperimentalIssues = getBooleanInputWithDefault('file_experimental_issues', true) @@ -45,6 +52,7 @@ export default async function () { core.debug(`Input: 'screenshot_repository: ${screenshotRepo}'`) core.debug(`Input: 'cached_filings_file: ${cachedFilingsFile}'`) core.debug(`Input: 'open_grouped_issues: ${shouldOpenGroupedIssues}'`) + core.debug(`Input: 'group_by: ${groupBy}'`) core.debug(`Input: 'dry_run: ${dryRun}'`) core.debug(`Input: 'file_best_practice_issues: ${fileBestPracticeIssues}'`) core.debug(`Input: 'file_experimental_issues: ${fileExperimentalIssues}'`) @@ -69,7 +77,7 @@ export default async function () { }, }, }) - const filings = updateFilingsWithNewFindings(cachedFilings, findings) + const filings = updateFilingsWithNewFindings(cachedFilings, findings, groupBy) // Suppressed new filings are kept out of the cache const suppressedFilings = new Set() @@ -131,7 +139,7 @@ export default async function () { filing.issue.state = 'closed' } else if (isNewFiling(filing)) { // Open a new issue for the filing - response = await openIssue(octokit, repoWithOwner, filing.findings[0], screenshotRepo) + response = await openIssue(octokit, repoWithOwner, filing.findings, screenshotRepo) ;(filing as Filing).issue = {state: 'open'} as Issue // Track for grouping @@ -151,8 +159,8 @@ export default async function () { // The developer intentionally closed this issue and labeled it 'wontfix', so leave it closed core.info(`Skipping reopen of issue labeled '${WONTFIX_LABEL}': ${filing.issue.url}`) } else { - // Reopen the filing's issue and update the body with the latest finding - response = await reopenIssue(octokit, issue, filing.findings[0], repoWithOwner, screenshotRepo) + // Reopen the filing's issue and update the body with the latest finding(s) + response = await reopenIssue(octokit, issue, filing.findings, repoWithOwner, screenshotRepo) filing.issue.state = 'reopened' } } diff --git a/.github/actions/file/src/openIssue.ts b/.github/actions/file/src/openIssue.ts index 03161c2..67128d0 100644 --- a/.github/actions/file/src/openIssue.ts +++ b/.github/actions/file/src/openIssue.ts @@ -17,26 +17,29 @@ function truncateWithEllipsis(text: string, maxLength: number): string { return text.length > maxLength ? text.slice(0, maxLength - 1) + '…' : text } -export async function openIssue(octokit: Octokit, repoWithOwner: string, finding: Finding, screenshotRepo?: string) { +export async function openIssue(octokit: Octokit, repoWithOwner: string, findings: Finding[], screenshotRepo?: string) { const owner = repoWithOwner.split('/')[0] const repo = repoWithOwner.split('/')[1] + const primary = findings[0] - const labels = [`${finding.scannerType}-scanning-issue`] + const labels = [`${primary.scannerType}-scanning-issue`] // Only include a ruleId label when it's defined - if (finding.ruleId) { - labels.push(`${finding.scannerType} rule: ${finding.ruleId}`) + if (primary.ruleId) { + labels.push(`${primary.scannerType} rule: ${primary.ruleId}`) } // Flag non-WCAG findings so they can be filtered or triaged separately - if (finding.category && finding.category !== 'wcag') { - labels.push(finding.category) + if (primary.category && primary.category !== 'wcag') { + labels.push(primary.category) } + const count = findings.length + const titleSuffix = count > 1 ? ` (${count} occurrences)` : ` on ${new URL(primary.url).pathname}` const title = truncateWithEllipsis( - `Accessibility issue: ${finding.problemShort[0].toUpperCase() + finding.problemShort.slice(1)} on ${new URL(finding.url).pathname}`, + `Accessibility issue: ${primary.problemShort[0].toUpperCase() + primary.problemShort.slice(1)}${titleSuffix}`, GITHUB_ISSUE_TITLE_MAX_LENGTH, ) - const body = generateIssueBody(finding, screenshotRepo ?? repoWithOwner) + const body = generateIssueBody(findings, screenshotRepo ?? repoWithOwner) return octokit.request(`POST /repos/${owner}/${repo}/issues`, { owner, diff --git a/.github/actions/file/src/reopenIssue.ts b/.github/actions/file/src/reopenIssue.ts index 329c695..10705f3 100644 --- a/.github/actions/file/src/reopenIssue.ts +++ b/.github/actions/file/src/reopenIssue.ts @@ -6,13 +6,13 @@ import {generateIssueBody} from './generateIssueBody.js' export async function reopenIssue( octokit: Octokit, {owner, repository, issueNumber}: Issue, - finding?: Finding, + findings?: Finding[], repoWithOwner?: string, screenshotRepo?: string, ) { let body: string | undefined - if (finding && repoWithOwner) { - body = generateIssueBody(finding, screenshotRepo ?? repoWithOwner) + if (findings?.length && repoWithOwner) { + body = generateIssueBody(findings, screenshotRepo ?? repoWithOwner) } return octokit.request(`PATCH /repos/${owner}/${repository}/issues/${issueNumber}`, { diff --git a/.github/actions/file/src/updateFilingsWithNewFindings.ts b/.github/actions/file/src/updateFilingsWithNewFindings.ts index eee1c6a..9c00be5 100644 --- a/.github/actions/file/src/updateFilingsWithNewFindings.ts +++ b/.github/actions/file/src/updateFilingsWithNewFindings.ts @@ -1,25 +1,39 @@ import type {Finding, ResolvedFiling, NewFiling, RepeatedFiling, Filing} from './types.d.js' +import type {GroupBy} from './groupBy.js' function getFilingKey(filing: ResolvedFiling | RepeatedFiling): string { return filing.issue.url } -function getFindingKey(finding: Finding): string { - if (finding.ruleId && finding.html) { - return `${finding.url};${finding.ruleId};${finding.html}` +function getFindingKey(finding: Finding, groupBy: GroupBy): string { + const rule = finding.ruleId + ? `${finding.scannerType};${finding.ruleId}` + : `${finding.scannerType};${finding.problemUrl}` + + switch (groupBy) { + case 'rule': + return rule + case 'rule+url': + return `${finding.url};${rule}` + case 'finding': + default: + if (finding.ruleId && finding.html) { + return `${finding.url};${finding.ruleId};${finding.html}` + } + return `${finding.url};${finding.scannerType};${finding.problemUrl}` } - return `${finding.url};${finding.scannerType};${finding.problemUrl}` } export function updateFilingsWithNewFindings( filings: (ResolvedFiling | RepeatedFiling)[], findings: Finding[], + groupBy: GroupBy = 'finding', ): Filing[] { const filingKeys: { [key: string]: ResolvedFiling | RepeatedFiling } = {} const findingKeys: {[key: string]: string} = {} - const newFilings: NewFiling[] = [] + const newFilingKeys: {[key: string]: NewFiling} = {} // Create maps for filing and finding data from previous runs, for quick lookups for (const filing of filings) { @@ -29,21 +43,23 @@ export function updateFilingsWithNewFindings( findings: [], } for (const finding of filing.findings) { - findingKeys[getFindingKey(finding)] = getFilingKey(filing) + findingKeys[getFindingKey(finding, groupBy)] = getFilingKey(filing) } } for (const finding of findings) { - const filingKey = findingKeys[getFindingKey(finding)] + const key = getFindingKey(finding, groupBy) + const filingKey = findingKeys[key] if (filingKey) { // This finding already has an associated filing; add it to that filing's findings ;(filingKeys[filingKey] as RepeatedFiling).findings.push(finding) + } else if (newFilingKeys[key]) { + newFilingKeys[key].findings.push(finding) } else { - // This finding is new; create a new entry with no associated issue yet - newFilings.push({findings: [finding]}) + newFilingKeys[key] = {findings: [finding]} } } const updatedFilings = Object.values(filingKeys) - return [...updatedFilings, ...newFilings] + return [...updatedFilings, ...Object.values(newFilingKeys)] } diff --git a/.github/actions/file/tests/dryRun.test.ts b/.github/actions/file/tests/dryRun.test.ts index f5fdf2b..8afea3d 100644 --- a/.github/actions/file/tests/dryRun.test.ts +++ b/.github/actions/file/tests/dryRun.test.ts @@ -12,6 +12,7 @@ vi.mock('../src/closeIssue.js', () => ({closeIssue: (...args: unknown[]) => clos const inputs: Record = {} const infoLines: string[] = [] const outputs: Record = {} +const failedMessages: string[] = [] vi.mock('@actions/core', () => ({ getInput: (name: string) => inputs[name] ?? '', getBooleanInput: (name: string) => (inputs[name] ?? 'false') === 'true', @@ -23,7 +24,9 @@ vi.mock('@actions/core', () => ({ }, debug: () => {}, warning: () => {}, - setFailed: () => {}, + setFailed: (msg: string) => { + failedMessages.push(msg) + }, })) // --- Mock fs: feed findings/cached filings in, swallow the output write --- @@ -91,6 +94,7 @@ describe('file action — dry_run', () => { beforeEach(() => { vi.clearAllMocks() infoLines.length = 0 + failedMessages.length = 0 for (const k of Object.keys(inputs)) delete inputs[k] for (const k of Object.keys(outputs)) delete outputs[k] vi.spyOn(console, 'table').mockImplementation(() => {}) @@ -194,4 +198,35 @@ describe('file action — dry_run', () => { expect(reopenIssue).toHaveBeenCalled() expect(closeIssue).toHaveBeenCalled() }) + + it("group_by 'rule' collapses multiple same-rule findings into a single OPEN", async () => { + // Three brand-new color-contrast findings across two URLs, no cached filings. + const ccA1 = {...finding, url: 'https://example.com/a', html: '1'} + const ccA2 = {...finding, url: 'https://example.com/a', html: '2'} + const ccB1 = {...finding, url: 'https://example.com/b', html: '3'} + files['/tmp/findings.json'] = JSON.stringify([ccA1, ccA2, ccB1]) + files['/tmp/cached.json'] = JSON.stringify([]) + inputs.findings_file = '/tmp/findings.json' + inputs.cached_filings_file = '/tmp/cached.json' + inputs.repository = 'org/repo' + inputs.token = 'fake-token' + inputs.dry_run = 'true' + inputs.group_by = 'rule' + + await runFileAction() + + expect(vi.mocked(console.table)).toHaveBeenCalledWith(expect.objectContaining({open: 1})) + }) + + it('fails fast on an invalid group_by value', async () => { + setup() + inputs.group_by = 'bogus' + + await runFileAction() + + expect(failedMessages.join('\n')).toContain("Invalid 'group_by' value: 'bogus'") + expect(openIssue).not.toHaveBeenCalled() + expect(reopenIssue).not.toHaveBeenCalled() + expect(closeIssue).not.toHaveBeenCalled() + }) }) diff --git a/.github/actions/file/tests/generateIssueBody.test.ts b/.github/actions/file/tests/generateIssueBody.test.ts index 7fbabe5..8341438 100644 --- a/.github/actions/file/tests/generateIssueBody.test.ts +++ b/.github/actions/file/tests/generateIssueBody.test.ts @@ -78,6 +78,21 @@ describe('generateIssueBody', () => { expect(body).not.toContain('flagged the element') }) + it('omits the Occurrences section for a single finding', () => { + const body = generateIssueBody(baseFinding, 'github/accessibility-scanner') + + expect(body).not.toContain('Other Occurrences') + }) + + it('renders an Occurrences checklist when given multiple findings', () => { + const second = {...baseFinding, url: 'https://example.com/other', html: 'Link'} + const body = generateIssueBody([baseFinding, second], 'github/accessibility-scanner') + + expect(body).toContain('## 2 Other Occurrences:') + expect(body).toContain(`- [ ] \`${baseFinding.html}\` on ${baseFinding.url}`) + expect(body).toContain(`- [ ] \`${second.html}\` on ${second.url}`) + }) + it('omits the category notice for WCAG findings', () => { expect(generateIssueBody(baseFinding, 'github/accessibility-scanner')).not.toContain('> [!NOTE]') expect(generateIssueBody({...baseFinding, category: 'wcag'}, 'github/accessibility-scanner')).not.toContain( diff --git a/.github/actions/file/tests/openIssue.test.ts b/.github/actions/file/tests/openIssue.test.ts index e9cb46f..890f843 100644 --- a/.github/actions/file/tests/openIssue.test.ts +++ b/.github/actions/file/tests/openIssue.test.ts @@ -28,21 +28,21 @@ function mockOctokit() { describe('openIssue', () => { it('passes screenshotRepo to generateIssueBody when provided', async () => { const octokit = mockOctokit() - await openIssue(octokit, 'org/filing-repo', baseFinding, 'org/workflow-repo') + await openIssue(octokit, 'org/filing-repo', [baseFinding], 'org/workflow-repo') - expect(generateIssueBody).toHaveBeenCalledWith(baseFinding, 'org/workflow-repo') + expect(generateIssueBody).toHaveBeenCalledWith([baseFinding], 'org/workflow-repo') }) it('falls back to repoWithOwner when screenshotRepo is not provided', async () => { const octokit = mockOctokit() - await openIssue(octokit, 'org/filing-repo', baseFinding) + await openIssue(octokit, 'org/filing-repo', [baseFinding]) - expect(generateIssueBody).toHaveBeenCalledWith(baseFinding, 'org/filing-repo') + expect(generateIssueBody).toHaveBeenCalledWith([baseFinding], 'org/filing-repo') }) it('posts to the correct filing repo, not the screenshot repo', async () => { const octokit = mockOctokit() - await openIssue(octokit, 'org/filing-repo', baseFinding, 'org/workflow-repo') + await openIssue(octokit, 'org/filing-repo', [baseFinding], 'org/workflow-repo') expect(octokit.request).toHaveBeenCalledWith( 'POST /repos/org/filing-repo/issues', @@ -55,7 +55,7 @@ describe('openIssue', () => { it('includes the correct labels based on the finding', async () => { const octokit = mockOctokit() - await openIssue(octokit, 'org/repo', baseFinding) + await openIssue(octokit, 'org/repo', [baseFinding]) expect(octokit.request).toHaveBeenCalledWith( expect.any(String), @@ -67,7 +67,7 @@ describe('openIssue', () => { it('adds a category label for non-WCAG findings', async () => { const octokit = mockOctokit() - await openIssue(octokit, 'org/repo', {...baseFinding, category: 'best-practice'}) + await openIssue(octokit, 'org/repo', [{...baseFinding, category: 'best-practice'}]) expect(octokit.request).toHaveBeenCalledWith( expect.any(String), @@ -79,7 +79,7 @@ describe('openIssue', () => { it('does not add a category label for WCAG findings', async () => { const octokit = mockOctokit() - await openIssue(octokit, 'org/repo', {...baseFinding, category: 'wcag'}) + await openIssue(octokit, 'org/repo', [{...baseFinding, category: 'wcag'}]) const labels = octokit.request.mock.calls[0][1].labels expect(labels).not.toContain('wcag') @@ -93,10 +93,20 @@ describe('openIssue', () => { ...baseFinding, problemShort: 'a'.repeat(300), } - await openIssue(octokit, 'org/repo', longFinding) + await openIssue(octokit, 'org/repo', [longFinding]) const callArgs = octokit.request.mock.calls[0][1] expect(callArgs.title.length).toBeLessThanOrEqual(256) expect(callArgs.title).toMatch(/…$/) }) + + it('includes an occurrence count in the title when grouping multiple findings', async () => { + const octokit = mockOctokit() + const second = {...baseFinding, url: 'https://example.com/other', html: 'Another'} + await openIssue(octokit, 'org/repo', [baseFinding, second]) + + const callArgs = octokit.request.mock.calls[0][1] + expect(callArgs.title).toContain('(2 occurrences)') + expect(generateIssueBody).toHaveBeenCalledWith([baseFinding, second], 'org/repo') + }) }) diff --git a/.github/actions/file/tests/reopenIssue.test.ts b/.github/actions/file/tests/reopenIssue.test.ts index f5b34ef..a739a3f 100644 --- a/.github/actions/file/tests/reopenIssue.test.ts +++ b/.github/actions/file/tests/reopenIssue.test.ts @@ -41,16 +41,16 @@ describe('reopenIssue', () => { it('passes screenshotRepo to generateIssueBody when provided', async () => { const octokit = mockOctokit() - await reopenIssue(octokit, testIssue, baseFinding, 'org/filing-repo', 'org/workflow-repo') + await reopenIssue(octokit, testIssue, [baseFinding], 'org/filing-repo', 'org/workflow-repo') - expect(generateIssueBody).toHaveBeenCalledWith(baseFinding, 'org/workflow-repo') + expect(generateIssueBody).toHaveBeenCalledWith([baseFinding], 'org/workflow-repo') }) it('falls back to repoWithOwner when screenshotRepo is not provided', async () => { const octokit = mockOctokit() - await reopenIssue(octokit, testIssue, baseFinding, 'org/filing-repo') + await reopenIssue(octokit, testIssue, [baseFinding], 'org/filing-repo') - expect(generateIssueBody).toHaveBeenCalledWith(baseFinding, 'org/filing-repo') + expect(generateIssueBody).toHaveBeenCalledWith([baseFinding], 'org/filing-repo') }) it('does not generate a body when finding is not provided', async () => { @@ -66,14 +66,14 @@ describe('reopenIssue', () => { it('does not generate a body when repoWithOwner is not provided', async () => { const octokit = mockOctokit() - await reopenIssue(octokit, testIssue, baseFinding) + await reopenIssue(octokit, testIssue, [baseFinding]) expect(generateIssueBody).not.toHaveBeenCalled() }) it('sends PATCH to the correct issue URL with state open', async () => { const octokit = mockOctokit() - await reopenIssue(octokit, testIssue, baseFinding, 'org/filing-repo', 'org/workflow-repo') + await reopenIssue(octokit, testIssue, [baseFinding], 'org/filing-repo', 'org/workflow-repo') expect(octokit.request).toHaveBeenCalledWith( 'PATCH /repos/org/filing-repo/issues/7', @@ -86,7 +86,7 @@ describe('reopenIssue', () => { it('includes generated body when finding and repoWithOwner are provided', async () => { const octokit = mockOctokit() - await reopenIssue(octokit, testIssue, baseFinding, 'org/filing-repo', 'org/workflow-repo') + await reopenIssue(octokit, testIssue, [baseFinding], 'org/filing-repo', 'org/workflow-repo') expect(octokit.request).toHaveBeenCalledWith( expect.any(String), diff --git a/.github/actions/file/tests/updateFilingsWithNewFindings.test.ts b/.github/actions/file/tests/updateFilingsWithNewFindings.test.ts new file mode 100644 index 0000000..008e186 --- /dev/null +++ b/.github/actions/file/tests/updateFilingsWithNewFindings.test.ts @@ -0,0 +1,101 @@ +import {describe, it, expect} from 'vitest' +import {updateFilingsWithNewFindings} from '../src/updateFilingsWithNewFindings.ts' + +const colorContrastFinding = (url: string, html: string) => ({ + scannerType: 'axe', + ruleId: 'color-contrast', + url, + html, + problemShort: 'elements must meet minimum color contrast ratio thresholds', + problemUrl: 'https://dequeuniversity.com/rules/axe/4.10/color-contrast', + solutionShort: 'ensure sufficient contrast', +}) + +describe('updateFilingsWithNewFindings — group_by', () => { + const findings = [ + colorContrastFinding('https://example.com/a', '1'), + colorContrastFinding('https://example.com/a', '2'), + colorContrastFinding('https://example.com/b', '3'), + ] + + it("defaults to 'finding': one filing per individual violation", () => { + const result = updateFilingsWithNewFindings([], findings) + expect(result).toHaveLength(3) + for (const filing of result) expect(filing.findings).toHaveLength(1) + }) + + it("'rule': collapses all occurrences of a rule into a single filing", () => { + const result = updateFilingsWithNewFindings([], findings, 'rule') + expect(result).toHaveLength(1) + expect(result[0].findings).toHaveLength(3) + }) + + it("'rule+url': one filing per rule per URL", () => { + const result = updateFilingsWithNewFindings([], findings, 'rule+url') + expect(result).toHaveLength(2) + const counts = result.map(f => f.findings.length).sort() + expect(counts).toEqual([1, 2]) // 2 on /a, 1 on /b + }) + + it("'rule': appends new occurrences to an existing cached filing instead of opening a new issue", () => { + const cached = [ + { + issue: { + id: 1, + nodeId: 'N1', + url: 'https://github.com/org/repo/issues/1', + title: 'color-contrast', + }, + findings: [colorContrastFinding('https://example.com/a', '1')], + }, + ] + const result = updateFilingsWithNewFindings(cached, findings, 'rule') + // No brand-new filing; all three findings attach to the cached issue. + expect(result).toHaveLength(1) + expect(result[0].issue?.url).toBe('https://github.com/org/repo/issues/1') + expect(result[0].findings).toHaveLength(3) + }) + + it("keeps distinct rules separate under 'rule'", () => { + const mixed = [ + colorContrastFinding('https://example.com/a', '1'), + {...colorContrastFinding('https://example.com/a', '

x

'), ruleId: 'heading-order'}, + ] + const result = updateFilingsWithNewFindings([], mixed, 'rule') + expect(result).toHaveLength(2) + }) + + it("'rule': does not merge findings from different scanners that share a ruleId", () => { + const a = { + ...colorContrastFinding('https://example.com/a', '1'), + scannerType: 'axe', + ruleId: 'duplicate-id', + } + const b = { + ...colorContrastFinding('https://example.com/a', '2'), + scannerType: 'reflow', + ruleId: 'duplicate-id', + } + const result = updateFilingsWithNewFindings([], [a, b], 'rule') + expect(result).toHaveLength(2) + }) + + it("'finding' (default) preserves the original 1:1 behavior with cached filings", () => { + const cached = [ + { + issue: { + id: 1, + nodeId: 'N1', + url: 'https://github.com/org/repo/issues/1', + title: 'color-contrast', + }, + findings: [colorContrastFinding('https://example.com/a', '1')], + }, + ] + const result = updateFilingsWithNewFindings(cached, findings) + // One repeated filing (issues/1) plus two brand-new filings. + expect(result).toHaveLength(3) + const repeated = result.find(f => f.issue?.url === 'https://github.com/org/repo/issues/1') + expect(repeated?.findings).toHaveLength(1) + }) +}) diff --git a/README.md b/README.md index 5c57745..0e34735 100644 --- a/README.md +++ b/README.md @@ -55,6 +55,7 @@ jobs: # skip_copilot_assignment: false # Optional: Set to true to skip assigning issues to GitHub Copilot (or if you don't have GitHub Copilot) # include_screenshots: false # Optional: Set to true to capture screenshots and include links to them in filed issues # open_grouped_issues: false # Optional: Set to true to open an issue grouping individual issues per violation + # group_by: finding # Optional: 'finding' (default, one issue per violation), 'rule' (one per rule), or 'rule+url' (one per rule per URL) # file_best_practice_issues: true # Optional: Set to false to stop filing new issues for best-practice findings (recommendations that are not hard WCAG failures) # file_experimental_issues: true # Optional: Set to false to stop filing new issues for experimental findings (checks that are not yet stable) # dry_run: false # Optional: Set to true to scan and log what would be filed without creating/closing issues or writing the cache @@ -119,7 +120,7 @@ Trigger the workflow manually or automatically based on your configuration. The | Input | Required | Description | Example | | --------------------------- | -------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------- | -| `urls` | No | Newline-delimited list of URLs to scan. Required unless `url_configs` is provided. | `https://primer.style`
`https://primer.style/octicons` | +| `urls` | No\* | Newline-delimited list of URLs to scan. Required unless `url_configs` is provided. | `https://primer.style`
`https://primer.style/octicons` | | `repository` | Yes | Repository (with owner) for issues and PRs | `primer/primer-docs` | | `token` | Yes | PAT with write permissions (see above) | `${{ secrets.GH_TOKEN }}` | | `cache_key` | Yes | Key for caching results across runs
Allowed: `A-Za-z0-9._/-` | `cached_results-primer.style-main.json` | @@ -131,6 +132,7 @@ Trigger the workflow manually or automatically based on your configuration. The | `skip_copilot_assignment` | No | Whether to skip assigning filed issues to GitHub Copilot. Set to `true` if you don't have GitHub Copilot or prefer to handle issues manually | `true` | | `include_screenshots` | No | Whether to capture screenshots of scanned pages and include links to them in filed issues. Screenshots are stored on the `gh-cache` branch of the repository running the workflow. Default: `false` | `true` | | `open_grouped_issues` | No | Whether to create a tracking issue which groups filed issues together by violation type. Default: `false` | `true` | +| `group_by` | No | How to consolidate findings when filing issues: `finding` (one issue per individual violation), `rule` (one issue per rule, aggregating every occurrence across all scanned URLs), or `rule+url` (one issue per rule per scanned URL). Default: `finding` | `rule` | | `file_best_practice_issues` | No | Whether to file issues for best-practice findings (accessibility recommendations that are not hard WCAG failures). Set to `false` to suppress new best-practice issues; existing ones are left untouched. Default: `true` | `false` | | `file_experimental_issues` | No | Whether to file issues for experimental findings (checks that are not yet stable). Set to `false` to suppress new experimental issues; existing ones are left untouched. Default: `true` | `false` | | `reduced_motion` | No | Playwright `reducedMotion` setting for scan contexts. Allowed values: `reduce`, `no-preference` | `reduce` | diff --git a/action.yml b/action.yml index 9c3052b..02cc58c 100644 --- a/action.yml +++ b/action.yml @@ -45,6 +45,10 @@ inputs: description: "In the 'file' step, also open grouped issues which link to all issues with the same problem" required: false default: 'false' + group_by: + description: "How to group findings into issues: 'finding' (one issue per violation, default), 'rule' (one issue per rule), or 'rule+url' (one issue per rule per scanned URL)." + required: false + default: 'finding' file_best_practice_issues: description: 'File issues for best-practice findings (accessibility recommendations that are not hard WCAG failures). Disabling suppresses new issues while existing ones are left untouched.' required: false @@ -141,6 +145,7 @@ runs: cached_filings_file: ${{ steps.normalize_cache.outputs.cached_filings_file }} screenshot_repository: ${{ github.repository }} open_grouped_issues: ${{ inputs.open_grouped_issues }} + group_by: ${{ inputs.group_by }} dry_run: ${{ inputs.dry_run }} file_best_practice_issues: ${{ inputs.file_best_practice_issues }} file_experimental_issues: ${{ inputs.file_experimental_issues }}