[test] Add an assertions mode to the axe regression harness - #48915
[test] Add an assertions mode to the axe regression harness#48915michelengelen wants to merge 1 commit into
assertions mode to the axe regression harness#48915Conversation
Deploy previewBundle size
Check out the code infra dashboard for more information about this PR. |
a5a6319 to
3da7a80
Compare
5dd3b74 to
ca61860
Compare
ca61860 to
91ee8d7
Compare
dc0eab5 to
e13e971
Compare
e13e971 to
fca727f
Compare
Lets a fixture assert every axe rule it exercises rather than only the CSS-dependent visual ones, which is what the per-component WCAG reports need.
fca727f to
4475eb3
Compare
siriwatknp
left a comment
There was a problem hiding this comment.
👍 Thanks for splitting this out of #48708. The assertions refactor reads well — shouldAssert keeps the visual default exactly as before, and a11yRule cannot be undefined at the new call site.
A few things I found.
| expect(parseRoute('/regression-Rating/FocusVisibleRating')).to.equal(null); | ||
| }); | ||
|
|
||
| it('parses a docs-components route into path/slug/demo', () => { |
There was a problem hiding this comment.
- Is there a reason to remove these three tests?
From what I checked, parseRoute and the buttons rule are both unchanged by this PR, so all three still pass on this branch. They cover the route parser and the brace-glob matching, which is what the a11y enrolment depends on.
If the goal is to not depend on A11Y_RULES churn, maybe rewrite the two getConfig ones against a local array, the way honours last-match-wins when multiple rules apply already does?
| (v) => VISUAL_RULES.includes(v.id) && !skip.has(v.id), | ||
| ); | ||
| const shouldAssert = (ruleId: string) => | ||
| !skip.has(ruleId) && (assertions === 'all' || VISUAL_RULES.includes(ruleId)); |
There was a problem hiding this comment.
- This branch never runs in CI.
A11Y_RULEShas one entry and it does not setassertions, and there is no test file forrecordA11yundertest/regressions/a11y/.
recordA11y is easy to call directly, so a small unit test would cover it. Could be something like:
recordA11y({ task: { meta: {} } } as TestContext, results, {
slug: 'buttons',
demo: 'BasicButtons',
assertions: 'all',
});Three cases would be enough — visual ignores a non-visual violation, all throws on it, and all plus skipAssertions stays silent.
| }); | ||
| }); | ||
|
|
||
| describe('Avatar', () => { |
There was a problem hiding this comment.
- This looks out of scope for the harness layer. It is a WCAG 1.4.12 conformance check for Avatar, and
avatarsis not enrolled inA11Y_RULES.
Should it live in the Avatar report PR instead, so this one stays only the assertions change?
| (node) => node.textContent === 'OP', | ||
| ); | ||
| const range = document.createRange(); | ||
| range.selectNodeContents(avatar); |
There was a problem hiding this comment.
.find()above returnsundefinedifLetterAvatarsever changes its initials, and then this line throwsparameter 1 is not of type 'Node'. The test still fails, which is good, but the message does not say what happened.
| range.selectNodeContents(avatar); | |
| if (!avatar) { | |
| throw new Error('LetterAvatars no longer renders an "OP" avatar'); | |
| } | |
| range.selectNodeContents(avatar); |
| const shouldAssert = (ruleId: string) => | ||
| !skip.has(ruleId) && (assertions === 'all' || VISUAL_RULES.includes(ruleId)); | ||
| const assertedViolations = results.violations.filter((v) => shouldAssert(v.id)); | ||
| const assertedIncomplete = results.incomplete.filter((v) => shouldAssert(v.id)); |
There was a problem hiding this comment.
shouldAssertis applied toincompletethe same asviolations, soassertions: 'all'also turns every axeincompleteinto a hard failure.
incomplete means "needs human review", so it is the non-deterministic bucket. Under visual it is bounded to two rule ids, under all it is every WCAG-tagged rule. GLOBAL_DISABLED_RULES plus the tag-scoped runOnly limit this a lot, so maybe it is fine.
Was this intended? If yes, I think a line in the assertions JSDoc saying skipAssertions is the escape hatch for noisy incompletes would help.
| * Scope: the components with a conformance report under | ||
| * `packages/mui-material/src/<Component>/accessibility.md`. Others onboard | ||
| * incrementally. |
There was a problem hiding this comment.
packages/mui-material/src/<Component>/accessibility.mddoes not exist yet on this branch, so a reader of this layer cannot act on it. I think this describes the end state of the series.
| * Scope: the components with a conformance report under | |
| * `packages/mui-material/src/<Component>/accessibility.md`. Others onboard | |
| * incrementally. | |
| * Initial PR scope: `buttons` only. Other components onboard incrementally. |
The new sentence could move to the layer that adds the first accessibility.md.
| // test/regressions/demoMeta.ts | ||
| { test: 'docs/data/material/components/alert/*', enabled: true, skipAssertions: ['color-contrast'] }, | ||
| { test: 'docs/data/material/components/buttons/{BasicButtons,ColorButtons}', enabled: true }, | ||
| { test: 'docs/data/material/components/buttons/{BasicButtons,ColorButtons}', enabled: true, assertions: 'all' }, |
There was a problem hiding this comment.
- This block is labelled
// test/regressions/demoMeta.ts, so it reads as a copy of the file, but the real rule atdemoMeta.ts:163does not setassertions. Rules are last-match-wins with no inheritance, so what the rule literally contains matters.
Which one is intended? If buttons is meant to run in all mode, setting it on the real rule would make the docs correct and also make the new branch run in CI, which covers 3 as well.
|
|
||
| ```ts | ||
| { test: 'docs/data/material/components/buttons/{BasicButtons,ColorButtons}', enabled: true } | ||
| { test: 'docs/data/material/components/buttons/{BasicButtons,ColorButtons}', enabled: true, assertions: 'all' } |
There was a problem hiding this comment.
- Same as
AGENTS.md— this example setsassertions: 'all'onbuttons, but the real rule indemoMeta.tsomits it.
Adds an
assertionsmode to the axe regression harness so a fixture can assertevery axe rule it exercises, not only the CSS-dependent visual ones. This is the
foundation the per-component WCAG conformance reports build on.
Also stops excluding the
progressslug from the demo bundle so axe can reachthe LinearProgress demos; screenshots stay disabled slug-wide for it, since the
animated bars are flaky.
Original work by @mj12albert, extracted from #48708 so the harness change reviews
separately from the Button report.
Part of the WCAG conformance effort (see #14187).
Important
How to review this PR
Layer 1 of 14 in a stacked series (#48915 → #48926). Its branch is the base of the series. GitHub cannot chain PR bases across a fork, so every PR in the series targets
masterinstead.Its diff is exactly its own change —
4475eb3.Part of the WCAG conformance effort (#14187).