Repository navigation
ci: a changed knowledge record alone runs no browser accessibility spec - #8674
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewNo new or modified components detected. Bundle Size SummaryNo component packages changed. Accessibility AuditStatus: No accessibility violations detected. Visual Regression55 of 502 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 35 more. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for closing the record gap in the browser-spec runner: a knowledge-record change no longer pulls in browser specs, and code beside a record still selects its folder's specs. One fix is needed first: isKnowledgeRecord treats every *.spec.md as a record, including the fixture, test, and *.generated.spec.md paths that docs/architecture/knowledge-contracts.md (INV27 and Owning code) says are not records and that the trusted audit-scope classifier counts as component source, so such a file would start pr-a11y yet skip its folder's pattern specs. Excluding those paths the way knowledge-paths.cjs does, with a test that a fixture .spec.md still selects its folder's spec, resolves it, and a matching clause in the routing paragraph of CONTRIBUTING.md would keep the guide accurate.
[Automated review]
| * @param {string} file | ||
| */ | ||
| export function isKnowledgeRecord(file) { | ||
| return file.endsWith('.spec.md'); |
There was a problem hiding this comment.
This accepts every *.spec.md, including paths that docs/architecture/knowledge-contracts.md says are not records: INV27, "the paths discovery already ignores are not candidates", and Owning code, fixture, test, generated, and *.generated.spec.md paths "are ignored consistently by discovery and PR routing". component-audit-scope.cjs reads that rule from knowledge-paths.cjs, so a file such as packages/core/src/Collapsible/__fixtures__/Collapsible.spec.md starts pr-a11y as Collapsible source, and this predicate then drops it, so accordion.chromium.spec.ts no longer runs. A local copy of the ignored segments keeps the runner self-contained, for example:
// Mirrors knowledge-paths.cjs: paths discovery ignores are not records.
const IGNORED_RECORD_SEGMENTS = new Set([
'__fixtures__',
'__generated__',
'__snapshots__',
'__tests__',
'build',
'coverage',
'dist',
'fixtures',
'generated',
'node_modules',
'test',
'test-utils',
'tests',
]);
export function isKnowledgeRecord(file) {
if (!file.endsWith('.spec.md') || file.endsWith('.generated.spec.md')) {
return false;
}
return !file
.split('/')
.some(segment => segment.startsWith('.') || IGNORED_RECORD_SEGMENTS.has(segment));
}A test that such a fixture still selects its folder's pattern spec would pin it.
meta-cla
Bot
added
the
CLA Signed
…erated specs still select specs
There was a problem hiding this comment.
Thanks for the follow-up: fixture, test, and generated .spec.md paths now keep selecting their specs, the contributor guide states the rule, and #8288's changed paths now select only the 21 always-on browser specs instead of 73. One small change remains before this can be approved: isKnowledgeRecord checks only directory segments (.slice(0, -1)), so a dot-named file such as packages/core/src/Collapsible/.Collapsible.spec.md, which knowledge-paths.cjs and the audit-scope classifier ignore, still counts as a record, contrary to the Owning code rule in docs/architecture/knowledge-contracts.md that discovery and PR routing ignore hidden paths consistently. Checking every segment, the file name included, plus one test for a dot-named record would close it.
[Automated review]
| } | ||
| return !file | ||
| .split('/') | ||
| .slice(0, -1) |
There was a problem hiding this comment.
This checks directory segments only, so packages/core/src/Collapsible/.Collapsible.spec.md counts as a record here, while knowledge-paths.cjs (which also checks the file name) and the audit-scope classifier treat it as Collapsible source; docs/architecture/knowledge-contracts.md (Owning code) has discovery and PR routing ignore hidden paths consistently. Dropping .slice(0, -1) closes it: no ignored folder name ends in .spec.md, so only the leading-. rule can match the file name. A test such as expect(isKnowledgeRecord('packages/core/src/Chat/.ChatLayout.spec.md')).toBe(false) would pin it.
There was a problem hiding this comment.
Recommended for approval: a changed knowledge record no longer pulls browser accessibility specs into pr-a11y through its folder or a ^ path token, while code beside a record, fixture, test, generated, and hidden .spec.md paths (dot-named files now included), unknown scope, and release checks select exactly as before. With this change, #8288's changed paths select the 21 always-on browser specs instead of 73, and the new tests fail against both the base runner and the previous revision of this pull request. Nothing further is needed from this review.
[Automated review]
|
Posted by Cindy's agent on her behalf. Decision: approved under Cindy's standing rule for CodeBunny-approved pull requests. CodeBunny recommends approval at this exact head with no remaining items. What this PR needs: nothing further; it merges when checks are green. After it lands, the knowledge-record migration (#8288) picks up the narrower accessibility-spec selection on its next run. |
|
/approve-spec 7cfb0a7 |
Maintainer impact
A pull request that edits components' knowledge records (
<Name>.spec.md) runs the browser accessibility specs for every one of those components in pr-a11y, even when it changes no rendering code. #8467 stopped records from counting in the trusted audit-scope classifier and the PR analysis. Buta11y-pr-contracts.mjs, which picks the Playwright specs, still matches raw changed paths: a changedCollapsible/Collapsible.spec.mdselects the accordion pattern spec through its folder and path token, and so on. On the 147-record schema migration that was 73 specs (568 tests), and pr-a11y hit its 15-minute timeout three times with no violation.Intended invariant
A changed knowledge record selects no browser accessibility spec, by folder or by path token. Code changed beside it still selects the spec as before, and always-on specs still run.
Change
.github/scripts/a11y-pr-contracts.mjs:selectSpecsmatches a spec's folder and its^…path tokens against the changed paths minus knowledge records.isKnowledgeRecordis a*.spec.mdoutside hidden, fixture, test, generated, build-output, coverage and dependency folders, and not*.generated.spec.md: exactly whatknowledge-paths.cjsdiscovers. The runner uses only Node built-ins, so it keeps its own copy of the folder list, and a test keeps that copy equal toknowledge-paths.cjs. A changed spec file, component-scope tokens, unknown scope, audit titles, and always-on specs are unchanged. The header documents the rule..github/scripts/a11y-pr-contracts.test.mjs:.spec.mdstill selects its folder's spec;knowledge-paths.cjs.CONTRIBUTING.md: the browser-spec routing paragraph says a changed record selects no spec.Evidence
Scope
Testing
pnpm vitest run --project node .github/scripts/a11y-pr-contracts.test.mjs(44 passed).