Repository navigation
ci: run a touched component's own accessibility contracts beside the tests, and block the merge on them - #9707
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
PR Analysis Report🔎 Preview ·
|
| Before | After | Change | |
|---|---|---|---|
| Job-minutes per PR update | 92.7 | 91.9 | −0.8 |
| Runner pool, runner-minutes a day | 32,717 | 32,440 | −277 (−0.8%) |
| Average concurrent jobs | 22.7 | 22.5 | −0.2 |
| Time to green (median → this run) | — | 16.7 min | — |
| Runs a day | 353 | 353 | 0 |
Changed jobs: Component accessibility contracts added: standard · Touched components changed: standard · test changed: standard
Not measured (the jobs that use these keep their recent minutes above): .github/scripts/a11y-pr-contracts.mjs, .github/scripts/a11y-pr-contracts.test.mjs, .github/scripts/ci-admission.test.mjs, .github/scripts/ci-test-join.mjs, .github/scripts/ci-test-join.test.mjs, .github/scripts/ci-test-routing.test.mjs, .github/scripts/ci-tooling-routing.test.mjs, .github/scripts/interaction-legs.test.mjs, .github/scripts/touched-components.test.mjs.
Assumptions: before is each job's average over the last 20 pull-request runs; after keeps that for unchanged jobs and uses this run's minutes for added or changed ones, weighted by how often each changed job ran in those runs; runs a day averages the seven whole days before today; time to green compares this run with the median of those runs that ran the same jobs.
Generated by PR Enrichment workflow | 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.
This needs changes before it can merge. The contract job and its test wiring follow AST-030 FR11, FR12 and DEC-22; these points remain:
- A changed spec skips the recorded-time check in
planDefaultCheck, so editingTokenizerEndLane.a11y.chromium.spec.ts(551.8 s) orTextInput.a11y.chromium.spec.tsstarts a file the 8-minute stop cuts and failstest, where AST-030 FR12 says such a file is reported as not run. ownedSpecsgets only the changed-spec list in CI, so its folder-change branch never fires, and edits in Chat, Layer or NavMenu skip owned contracts the labelled run selects, such asChatLayoutScrollButtonSurfaceanduseChatStreamScroll.- Remove the proof-run comment at
packages/core/src/Switch/Switch.tsx:1152before review completes. - Add the FR19
CI impact:line (runner class, job-minutes per update before and after, runner-pool usage, time to green) from the proof runs. - The description says every file starts only when its recorded time fits and that a folder change owns its specs; fixing the two code issues makes that true, or else narrow the wording.
The six changed test files pass; a test with a changed over-budget spec, and a workflow test that passes the full changed-file list to the owned-spec step, would cover both fixes.
[Automated review]
| workers, | ||
| budgetMs = DEFAULT_CHECK_BUDGET_MS, | ||
| }) { | ||
| const {run, notRun} = withinBudget(changed); |
There was a problem hiding this comment.
Changed specs go straight into run here with no recorded-time check, so a changed TokenizerEndLane.a11y.chromium.spec.ts (551.8 s, about 720 s with the margin) starts and is cut by --global-timeout, failing the required test join. Running each changed file through the same STARTUP_MS + projectedMs(...) <= budgetMs test as the owned files keeps an overflow as "not run" (AST-030 FR12).
| ? withinBudget(changedOnly(routed, changedPaths)) | ||
| ? planDefaultCheck({ | ||
| changed: changedOnly(routed, changedPaths), | ||
| owned: ownedSpecs({specs, components: ownedBy, changedPaths}), |
There was a problem hiding this comment.
Under --changed-only, changedPaths is the changed-spec list, so the folder branch in ownedSpecs never sees a component file. An edit to packages/core/src/Chat/ChatLayout.tsx then skips ChatLayoutScrollButtonSurface and useChatStreamScroll (release-only), which the labelled run selects by folder. Passing the scope's full changed-file list here would fix it.
| } | ||
|
|
||
| Switch.displayName = 'Switch'; | ||
| // CI proof run only: removed before merge. |
There was a problem hiding this comment.
Proof-run line: please drop this commit before review completes.
…tests, and block on them
630ad35 to
e8d8dd5
Compare
There was a problem hiding this comment.
This still needs changes before it can merge. The contract job and its test wiring follow AST-030 FR11, FR12 and DEC-22, and the six changed test files pass; these points remain:
- A changed spec skips the recorded-time check in
planDefaultCheck, so editingTokenizerEndLane.a11y.chromium.spec.ts(551.8 s) orTextInput.a11y.chromium.spec.ts(478.7 s) starts a file the 8-minute stop cuts and failstest, where AST-030 FR12 says such a file is reported as not run. ownedSpecsgets only the changed-spec list in CI, so its folder-change branch never fires, and an edit toChat/ChatLayout.tsxruns none of the Chat folder's contracts that the labelled run selects, three of themrelease-only.- The proof-run comment at
packages/core/src/Selector/Selector.tsx:2211turns the requiredlintcheck red (no Changeset for@astryxdesign/core), so drop that commit as planned. - Add the FR19
CI impact:line (runner class, job-minutes per update before and after, runner-pool usage, time to green) to the description. - The description says every file starts only when its recorded time fits and that a folder change owns its specs; fixing the first two points makes that true, or else narrow the wording.
A test with a changed over-budget spec, and a workflow test that passes the full changed-file list to the owned-spec step, would cover both code fixes.
[Automated review]
| } | ||
|
|
||
| Selector.displayName = 'Selector'; | ||
| // CI proof run only: removed before merge. |
There was a problem hiding this comment.
Proof-run line: the required lint check fails on it (no Changeset for @astryxdesign/core). Please drop this commit before merge.
e8d8dd5 to
07119da
Compare
There was a problem hiding this comment.
This needs changes before it can merge. The contract job and its test wiring follow AST-030 FR11, FR12 and DEC-22, and the six changed test files pass; these points remain:
- A changed spec skips the recorded-time check in
planDefaultCheck(.github/scripts/a11y-pr-contracts.mjs:445), so a pull request that editsTokenizerEndLane.a11y.chromium.spec.ts(551.8 s recorded) orTextInput.a11y.chromium.spec.ts(478.7 s) starts a file the 8-minute stop cuts and failstest, where AST-030 FR12 reports such a file as not run. ownedSpecsgets only the changed-spec list in CI (:754), so its folder-change branch never sees a component file, and an edit toChat/ChatLayout.tsxskipsChatLayoutScrollButtonSurface,useChatStreamScrolland two scoped Chat specs that the labelled run's mapping selects.- The proof-run comment at
packages/core/src/RadioList/RadioList.tsx:449turns the requiredlintcheck red (no Changeset for@astryxdesign/core), so drop that commit as planned. - Add the FR19
CI impact:line (runner class, job-minutes per update before and after, runner-pool usage, time to green) to the description, from the Switch and Selector proof runs as well as this head's RadioList run. - The description says every file starts only when its recorded time fits and that a folder change owns its specs; fixing the first two points makes that true, or else narrow the wording.
A test with a changed over-budget spec, and a workflow test that passes the full changed-file list to the owned-spec step, would cover both code fixes.
[Automated review]
| workers, | ||
| budgetMs = DEFAULT_CHECK_BUDGET_MS, | ||
| }) { | ||
| const {run, notRun} = withinBudget(changed); |
There was a problem hiding this comment.
Changed specs go straight into run here with no recorded-time check, so a changed TokenizerEndLane.a11y.chromium.spec.ts (551.8 s, about 720 s with the margin) starts and is cut by --global-timeout, failing the required test join. Running each changed file through the same STARTUP_MS + projectedMs(...) <= budgetMs test as the owned files keeps an overflow as "not run" (AST-030 FR12).
| ? withinBudget(changedOnly(routed, changedPaths)) | ||
| ? planDefaultCheck({ | ||
| changed: changedOnly(routed, changedPaths), | ||
| owned: ownedSpecs({specs, components: ownedBy, changedPaths}), |
There was a problem hiding this comment.
Under --changed-only, changedPaths is the changed-spec list, so the folder branch in ownedSpecs never sees a component file. An edit to packages/core/src/Chat/ChatLayout.tsx then skips ChatLayoutScrollButtonSurface and useChatStreamScroll (release-only), which the labelled run selects by folder. Passing the scope's full changed-file list here would fix it.
| } | ||
|
|
||
| RadioList.displayName = 'RadioList'; | ||
| // CI proof run only: removed before merge. |
There was a problem hiding this comment.
Proof-run line: the required lint check fails on it (no Changeset for @astryxdesign/core). Please drop this commit before merge.
07119da to
feb9405
Compare
There was a problem hiding this comment.
This needs changes before it can merge. The earlier points are fixed: changed specs now go through the 8-minute fit, folder ownership sees every changed path, the proof-run lines are gone, and the CI impact: line is in place. One issue remains, in three parts:
ownedSpecsignores every pathscopetoken (.github/scripts/a11y-pr-contracts.mjs:378-380), so aSlider.tsxedit never runsinternal/a11y-spec/src/patterns/slider.chromium.spec.ts, whose routing line namespackages/core/src/Slider/, and the same holds for the pattern contracts of 15 other components (Table, DropdownMenu, Tooltip and more), which contradicts AST-030 FR12's "same@pr-a11yscope and spec-location mapping the labelled run uses".ownedSpecscopies the owner loop ofselectSpecsinstead of reusing it, which is where the two mappings drifted apart; deriving the owned set fromselectSpecswith only FR13's exclusions keeps one mapping.- The title and description say the job runs each touched component's own scoped contracts and skips path-token matches only on shared modules, which becomes true once a path token that names a touched component's folder counts, or else the wording should be narrowed.
An ownership test with the slider pattern and a Slider/Slider.tsx change would cover the fix.
[Automated review]
| const token = routing.scope.find( | ||
| t => !t.startsWith('^') && componentMatches(t, components), | ||
| ); |
There was a problem hiding this comment.
This drops every path token, but most name a component's own folder: internal/a11y-spec/src/patterns/slider.chromium.spec.ts is scoped to packages/core/src/Slider/, so a Slider.tsx edit never runs it, and 14 more pattern contracts are bound the same way. AST-030 FR12 asks for the same mapping the labelled run uses; counting a path token that matches a changed file in a touched component's folder keeps shared modules out.
There was a problem hiding this comment.
This needs one more change before it can merge. The earlier points are fixed: owned specs now come from selectSpecs, so a pattern contract bound by a path token (the slider pattern for a Slider.tsx edit) runs, and the description matches what the job selects.
- When the scope step cannot list the changed files it sets no
changed_files_path, so thereadLines(changedFilesFile) ?? changedPathsfallback (.github/scripts/a11y-pr-contracts.mjs:759) leaves only contracts named for a touched component (aChat/ChatLayout.tsxchange drops from 6 owned specs to 0), reports none of the dropped ones as not run, and letstestpass, against AST-030 FR12's "a skip never reads as a pass"; failing the job when the scope applies and the list is empty would close it.
Separately, the $comment in .github/a11y-contract-durations.json still says only shard balance depends on those numbers, which stops being true once they decide what the required job starts.
[Automated review]
| owned: ownedSpecs({ | ||
| specs, | ||
| components: ownedBy, | ||
| changedPaths: readLines(changedFilesFile) ?? changedPaths, |
There was a problem hiding this comment.
When the scope step cannot list the changed files, FILES_PATH is empty, readLines('') returns null, and this falls back to the changed-spec list. Folder- and path-token-owned contracts then drop without a not-run notice and the job still passes test (a Chat/ChatLayout.tsx change goes from 6 owned specs to 0). Failing when --changed-files is given empty, or when the scope applies with no changed_files_path, keeps a skip from reading as a pass (AST-030 FR12).
There was a problem hiding this comment.
This looks good to merge once the CI run on this head finishes green. The contract job and its test wiring follow AST-030 FR4, FR11, FR12 and DEC-22, and the earlier point is fixed: the job now fails when the scope step cannot list the changed files, and the runner refuses owners without that list.
One optional follow-up: the guard's "Re-run the job" message could name the scope step's warning, since a pull request whose file list GitHub truncates at 3,000 (or returns empty) fails on every re-run.
[Automated review]
| - name: Require the changed-file list | ||
| if: steps.scope.outputs.applies == 'true' && steps.scope.outputs.changed_files_path == '' | ||
| run: | | ||
| echo "::error::The scope step could not list the changed files, so the contracts to run are unknown. Re-run the job." |
There was a problem hiding this comment.
Optional: when GitHub truncates the file list at 3,000 or returns none, the scope step gives up for good (.github/actions/test-scope/action.yml:195-196), so a re-run fails the same way. Pointing at the scope step's warning here would name the real cause.
There was a problem hiding this comment.
This looks ready to merge once test passes on this head. This push deferred the heavy suites, so test has not run yet. The new contract job and its test wiring follow AST-030 FR4, FR11, FR12 and DEC-22, and the changed tests pass.
Two optional wording fixes, neither blocking:
- The guard at
.github/workflows/ci.yml:3746says "Re-run the job", but a list of 3,000 or more files, or an empty list, fails the scope step on every run. Pointing to the scope step's warning would name the real cause. - On a wide change that also edits a spec, the job summary says "No touched component." (
.github/scripts/a11y-pr-contracts.mjs:471). Saying the change was wide, with its component count, would be accurate.
[Automated review]
| - name: Require the changed-file list | ||
| if: steps.scope.outputs.applies == 'true' && steps.scope.outputs.changed_files_path == '' | ||
| run: | | ||
| echo "::error::The scope step could not list the changed files, so the contracts to run are unknown. Re-run the job." |
There was a problem hiding this comment.
Optional: when GitHub lists 3,000 or more files, or none, the scope step gives up on every attempt (.github/actions/test-scope/action.yml:195-196), so "Re-run the job" cannot help there. Pointing to the scope step's warning would name the real cause.
| lines.push( | ||
| components.length | ||
| ? `Touched components: ${components.map(c => `\`${c}\``).join(', ')}.` | ||
| : 'No touched component.', |
There was a problem hiding this comment.
Optional: on a wide change the workflow passes an empty --owned-by, so this prints "No touched component." even when the change touched more than five components. Passing the wide flag and count would let the summary say why no owned contract ran.
humbertovirtudes
left a comment
humbertovirtudes
left a comment
There was a problem hiding this comment.
Re-reviewed exact head f84d638144656e916c10a43cc31c56ab49028e98. The contract runner, its regression tests, test join, scope action and Playwright configuration are byte-identical to the previously reviewed implementation; the current own merge-base patch remains confined to the ten CI/tooling files. The fail-closed changed-file guard, canonical scope selection, duration-budget checks and required-test wiring address the prior findings. No blocking code finding remains.
This is a code approval, not a passing-CI certificate. The current build, test and docsite-test jobs stopped at the explicit Heavy CI deferred step under AST-030 FR20. Required execution must still finish green before merge.
There was a problem hiding this comment.
This needs one change before it can merge. The new contract job and its test wiring follow AST-030 FR4, FR11, FR12 and DEC-22.
- Regression: merging
mainbrought in.github/scripts/interaction-legs.test.mjs, whose join case (lines 247-267) runsci-test-join.mjswithoutPR_COMPONENT_A11Y_CONTRACTS_RESULT, so the join now exits 1 andtest-nodefails; addPR_COMPONENT_A11Y_CONTRACTS_RESULT: 'success'to itspassingenv.
The two optional wording fixes from the last review still apply: the guard's "Re-run the job" (.github/workflows/ci.yml:3830) and "No touched component." on a wide change (.github/scripts/a11y-pr-contracts.mjs:471). CI has not run the heavy suites on this head yet.
[Automated review]
| [ | ||
| 'pr-component-a11y-contracts', | ||
| process.env.PR_COMPONENT_A11Y_CONTRACTS_RESULT, | ||
| ], |
There was a problem hiding this comment.
Merging main brought in .github/scripts/interaction-legs.test.mjs, whose join case builds its passing env without this result (lines 247-267). The join now exits 1 with pr-component-a11y-contracts: missing, so that test fails and test-node goes red. Adding PR_COMPONENT_A11Y_CONTRACTS_RESULT: 'success' to that object fixes it.
The interaction-legs contract hard-coded the lanes the test join reads, so adding the component accessibility contracts lane to the join left the all-passing fixture without that lane and the join returned 1. The fixture now reads every needs.<lane>.result and scope output from the join step, checks each lane is in the job needs, and proves a failed, cancelled, skipped or empty result of every lane fails the join.
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet, because the pull request is still a draft. Marking it ready for review starts CI, and the merge still needs test to pass.
The new contract job and its test wiring follow AST-030 FR4, FR11, FR12 and DEC-22, and the join fixture in .github/scripts/interaction-legs.test.mjs now covers every lane the join reads, so the earlier test failure is fixed.
Two optional wording fixes from earlier rounds still apply: the guard's "Re-run the job" (.github/workflows/ci.yml:3830) cannot help when the file list is truncated or empty, and the summary's "No touched component." (.github/scripts/a11y-pr-contracts.mjs:471) hides a wide change.
[Automated review]
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet because the pull request is still a draft; marking it ready for review starts CI, and the merge still needs test to pass.
The contract job and its test wiring follow AST-030 FR4, FR11, FR12 and DEC-22, the latest merge from main leaves them unchanged, and the changed tests pass on this head.
Two optional wording fixes still apply: the guard's "Re-run the job" (.github/workflows/ci.yml:3832) cannot help when GitHub lists 3,000 or more files or none, and the job summary's "No touched component." (.github/scripts/a11y-pr-contracts.mjs:471) hides a wide change.
[Automated review]
Maintainer impact
A component's own browser accessibility contracts do not run on that component's pull request. About 50 spec files across about 30 components are
release-only, so the first check that runs them is main's release check. Example: #9636 merged green while Switch's aborted-press contract (WCAG 2.5.2) failed. The release check found it, and the release waited for a fix-forward. #9611 (Selector) and #9169 (RadioList) skipped their contracts the same way.Intended invariant
spec:AST-030FR11, FR12, FR13, DEC-7 and DEC-22, as amended by #9693:scopedandrelease-onlyspecs. It runs no spec of another component or a reverse dependent, no path-token match on a shared module, and no unchangedauditoralwaysspec. A wide change runs only its changed specs.testjoin. No new required context is added.ci:run-a11yrun also gets therelease-onlyspecs that a changed component owns directly, but never through a scope token or an unknown scope.Change
.github/scripts/a11y-pr-contracts.mjs--owned-by <components>and--changed-files <file>, used with--changed-only, addownedSpecs(). It is the labelled run's ownselectSpecs()with two limits: onlyscopedandrelease-onlyspecs, and only changes inside a component folder (packages/<pkg>/src/<Name>/). So it uses the same mapping. A spec is owned when:scopedand ascopetoken names a touched component, or matches a change inside a component folder (for example, the slider pattern contract for aSlider/Slider.tsxedit).A change to a shared module, the contract harness or a story owns nothing.
--changed-filesgives every changed path, and the workflow passes the scope step'schanged_files_path.planDefaultCheck()runs the changed specs first (at most four), then the owned specs in path order.projectedMs()projects the run on the job's workers from.github/a11y-contract-durations.json, with a 1.25× margin and 30 s for start-up. Every file, changed or owned, starts only if the projected run still ends inside 8 minutes. The other files are not run. An editedTokenizerEndLanespec, for example, is not run, so the stop never cuts it.summaryMarkdown()writes the job summary. Each not-run file also gets a::notice, andselection.jsonrecords the plan.selectSpecs(): on a pull request, arelease-onlyspec is selected only by direct ownership (the spec changed, its folder changed, or its named component is in scope). Never by a scope token, and never on an unknown scope..github/workflows/ci.ymlpr-component-a11y-contracts("Component accessibility contracts"). It needs[room, admission, heavy-ci, build-storybook], notpr-visualor the touched check. It runs onubuntu-latestwith 4 Playwright workers and a 15-minute limit. It gets the touched components from the sametouched-components.mjs scopecall the touched check makes. Outside a pull request, on a specialized lane (spec, docsite, tooling or Lab only), or with nothing inpackages/*/src/orinternal/a11y-spec/changed, it skips its work and reports success.--owned-bywithout--changed-files. A contract that was not chosen therefore never reads as passed.testneeds it, andci-test-join.mjsrequires itssuccess.$commentin.github/a11y-contract-durations.jsonnow says the numbers also plan the pull-request job.pr-touched-componentsno longer runs the changed specs. That run moved into the new job, so the touched check stays advisory: visual, axe and RTL.Required-check mechanism
The existing required
testjoin waits on the new job (option ii). Branch protection does not change, no new required name is added, and the merge guard needs no change. This adds nothing to time to green only because the job ends beforetest's slowest current need. The evidence below shows that.Evidence
Times are minutes after the run's
admissionjob ends, so CI-slot queueing is left out. "Slowest need" is the latest oftest's current needs (test-ui and test-node legs, test-build, pr-interaction, pr-interaction-webkit). In every run it wastest-node (4/4).test(time to green)teststarts when it did before. The after-minus-before differences intestare run-to-run variation intest-node (4/4), which this PR does not touch. The fix(RadioList, Switch): required inputs take their invalid state from status #9042 run predates the current admission jobs.Switch.a11y.chromium.spec.ts(release-only), 12 tests passed in 58 s on 1 worker.ListboxandSelectorare release-only) plus thecomboboxpattern, through itscore/Selectorscope token. 60 tests passed in 2.6 min on 4 workers.RadioGroup.a11y.chromium.spec.ts(release-only), 28 tests passed in 1.7 min.TokenizerEndLane(552 s recorded) andTextInput(479 s recorded, too long with the margin).FieldDisabledForcedColorsFocus(its path token names the Selector folder). That file has one test and no recorded time, so it is planned at the median of 17 s.Listbox(325 s) still sets the end of the Selector run on 4 workers. Switch and RadioList select the same files as in their proof runs.peakof 16 still covers this, so it is unchanged.Switch.tsx,Selector.tsxandRadioList.tsxwith a one-line comment, one at a time. The final head drops it and carries only the CI change, so on this PR the new job skips as tooling-only. The workflow is the same as in the three runs above. On the proof heads, Lint's Changeset coverage check failed because of the temporary component touch. The final head has no such touch.CI impact:
pr-component-a11y-contractsjob runs on standardubuntu-latest.pr-touched-componentsstays on standardubuntu-24.04-arm, and one step moves out of it.pr-touched-components, and the new job spends about the same. A PR outside the job's scope gets a scope-only run of about 0.1–0.2 min. Specialized lanes skip.Scope
Testing
vitest run .github/: 107 files and 2,473 tests pass.a11y-pr-contracts.test.mjs:Slider/Slider.tsx). Not owned: a path token matching a shared module or the harness, a shared pattern change, a reverse dependent, anauditoralwaysspec. Nothing is owned on a wide change. Against the repository's real specs, aChat/ChatLayout.tsxchange ownsuseChatStreamScrollandChatLayoutScrollButtonSurface, and aSlider/Slider.tsxchange owns the slider pattern contract.release-onlyrouting.--changed-files). A workflow test checks the fail step's condition and that it comes before the run step.TokenizerEndLanespec and aChat/ChatLayout.tsxchange, ownercore/ChatLayout, selects the 6 Chat folder specs. It reports the Tokenizer spec as not run.touched-components.test.mjs: the new job's needs and gate, thetestjoin wiring, skip-with-success, the run command and env (including the full changed-file list), the step limit under the job limit. The touched check runs no contract.ci-test-join.test.mjs: failure, skipped, cancelled or missing contract result fails the join.ci-admission,ci-test-routing,ci-tooling-routing: the new job carries the admission guard and asks for the component-audit outputs. It is part oftest's needs.actionlint(with shellcheck) reports no new finding, andbash -npasses on every changedrun:script.