-
Notifications
You must be signed in to change notification settings - Fork 334
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Switch Accessibility tests to @axe-core/puppeteer
#3522
Conversation
We were previously on |
<div class="govuk-width-container"> | ||
{% block beforeContent %}{% endblock %} | ||
</div> | ||
<main class="govuk-main-wrapper {{ mainClasses }}" id="main-content" role="main"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've created full-width.njk
(without wrapping landmarks) alongside full-width-landmarks.njk
This is so Axe can pass some of the "landmark-in-landmark" violations
You'll see some Percy related diff changes as a result
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks ace @colinrotherham, thanks! Seems like a sensible approach, and great commit structure.
I think it makes sense to keep a accessibility.test.mjs
for each component whilst we have individual rule exceptions, but is there any other reason for doing it that way? Wondering if in the future we might move to having this in a single place, like we have for HTML validation.
Can we make sure we add the accessibility tests to the required status checks for the main branch after this is merged?
Thanks @36degrees 🙌 Ahh yep, it's to align with Running individual tests in The entire test suite is quite slow, so if you're only working on one component you can narrow things down: Run Accordion tests onlyTotal time: 5s npx jest --watch src/govuk/components/accordion You can try
Run Accessibility tests onlyTotal time: 55s npx jest --selectProjects "Accessibility tests" Run all non-build testsTotal time: 1m 20s (previously 25s) npm test Running The added test time was a concern but well worth it |
226c80c
to
8f1983b
Compare
That makes sense, thanks for explaining 👍🏻 |
@36degrees Great idea I've just fixed the conflicts and pushed again Some extra comments added to Addresses a minor concern that 12x concurrent Chromium processes on my machine was a bit much! npx jest --selectProjects "Accessibility tests" Support for macOS ARM CPUsOut of interest, but performance related, can you test this for me? I've got an Intel Mac Enabling the ARM CPU experiment to see how much faster things run: See config puppeteer.config.js module.exports = {
cacheDirectory: join(__dirname, '.cache', 'puppeteer'),
experiments: {
macArmChromiumEnabled: true
}
} Thanks Update From @domoscargin: "Accessibility tests" total time 40% reduced to 24 seconds (~30% for all tests) |
a5536dd
to
4f5f7e5
Compare
4f5f7e5
to
271bee0
Compare
Thought it best to push more commits to explain how to see these tests in action Since our headless browser tests can be made visible with
Update: With video demo Radio.button.accessibility.tests.mp4 |
271bee0
to
c47c6d0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new commits look good to me, I'm just not sure about the documentation.
@@ -76,6 +76,8 @@ We write functional tests for every component to check the output of our Nunjuck | |||
|
|||
If a component uses JavaScript, we also write functional tests in a `[component name].test.js` file, for example [checkboxes.test.js](../../src/govuk/components/checkboxes/checkboxes.test.js). These component tests check that interactions, such as a mouse click, have the expected result. | |||
|
|||
When running tests in [headless Chrome](https://developers.google.com/web/updates/2017/04/headless-chrome) it's useful to configure [`{ headless: false }`](../../jest-puppeteer.config.js) and run `await jestPuppeteer.debug()` from the test. This pauses an open browser for inspection. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels a bit backwards to me as it's not clear when you'd want to use it when you start reading the sentence. Would it make sense to flip it? Maybe something like:
If you want to inspect a test that's running in the browser, configure Jest Puppeteer to use non-headless mode by setting
headless: false
in the config and then use Jest Puppeteer's debug mode to pause the test execution.
Not blocking – just a suggestion.
Also, it might be nice to be able to change mode without editing the config – the default config seems to allow changing it using a HEADLESS
env variable. Could we do similar? (Again, not-blocking, could be a follow up PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds fab let's do it
Allows the header/footer to pass landmark tests without the `main` wrapper
This makes “headful” browser inspection possible by preventing: 1. Multiple browser windows opening concurrently 2. Tests from closing down browsers after they complete
Includes tip regarding `await jestPuppeteer.debug()` with ESLint config changes to allow `jestPuppeteer` as a known global
c47c6d0
to
4f0e257
Compare
I've made the suggested documentation change but with Branch protection settings also have Accessibility tests added 👍 (Other open PRs will need updating to include the new tests) |
These updates reflect the changes to our automated testing processes. These can be explored in detail in the PR: alphagov/govuk-frontend#3522 - Added reference to @axe-core/puppeteer - Removed notice about known limitations of our automated testing setup - Added section explaining 4 of the recent updates we've made
This PR closes:
All components + examples are now tested in
axe-core@4.7.0
via PuppeteerThe tests run locally via
npm test
or separately with:npx jest --selectProjects "Accessibility tests"
Colour contrast checks are built in ✅
Known issues
This switch has flagged a few known issues which are now ignored + documented, including some tests with positive
tabindex
and "landmark-inside-landmark" issues due to preview HTMLAll page content must be contained by landmarks
Axe issue: "All page content must be contained by landmarks" #1604
https://dequeuniversity.com/rules/axe/4.7/region
Accordion "
aria-labelledby
attribute cannot be used on a div with no valid role attribute"Accordion - Elements must only use allowed ARIA attributes #2472
https://dequeuniversity.com/rules/axe/4.7/aria-allowed-attr
Conditional reveal "ARIA attribute is not allowed:
aria-expanded="false"
"Radios that conditionally reveal content use
aria-expanded
which is not a valid aria-attribute for those roles #979https://dequeuniversity.com/rules/axe/4.7/aria-allowed-attr
Invalid ARIA attribute value:
aria-controls="example-id"
for missing IDshttps://dequeuniversity.com/rules/axe/4.7/aria-valid-attr-value
Colour contrast for disabled form controls
https://dequeuniversity.com/rules/axe/4.7/color-contrast
Test reports
I've kept the
jest-axe
reporter but generated the report using@axe-core/puppeteer
insteadGitHub Actions will log failures as below:
Test workflow
Test progress appears on the GitHub Actions "Checks" tab (see example Accessibility tests)