-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Jest axe a11y testing #127185
Jest axe a11y testing #127185
Conversation
…aration of concerns. Also added tests for indices tab in Index Management.
impactLevel?: ImpactValue | ||
): Promise<void> => { | ||
const violations = await getA11yViolations(component, impactLevel); | ||
expect(violations).toHaveLength(0); |
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.
Will the output for this include all of the violations?
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.
That is a great question! I think I need to update the logic here, because it will only get 'critical' violations by default. I don't even think I need a parameter for impactLevel
here. I'll ask in the a11y channel which level we want to test for.
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.
Great call removing the impactLevel
. Our goal is to find and remove all accessibility issues, regardless of their impact. It has been my experience that serious and moderate issues can cause significant UX problems in large enough quantities. They're not breaking UX issues per se, but each one causes friction and adds to the accumulated cognitive load.
|
||
server.respondWith('GET', `${API_BASE_PATH}/mapping/:name`, [ | ||
status, | ||
{ 'Content-Type': 'application/json' }, |
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.
would it make sense to represent this content type block as a constant HEADERS or something to avoid repeating or is there an expectation these might be different across tests?
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.
yes, this file is very repetitive. I'll see how all the same lines can be refactored into a function or similar.
…es that are now in kbn-test package
…es that are now in kbn-test package
…actored http requests helper file.
Pinging @elastic/platform-deployment-management (Team:Deployment Management) |
Pinging @elastic/kibana-accessibility (Project:Accessibility) |
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.
Enterprise Search changes LGTM
Pinging @elastic/apm-ui (Team:apm) |
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.
nice work @yuliacech! Changes lgtm! Would love to see an issue created to patch the tests up once that eui#5709 lands in kibana 🚀
}); | ||
const { component } = testBed; | ||
component.update(); | ||
// this is expected to fail and needs to be updated when eui#5674 is available in Kibana |
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.
👍🏼 Wondering if we should perhaps create a new issue to update this bit of code when this eui issue is fixed
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 changed the comment to mention that the problem will be resolved with EUI 52.0.0 update (probably in this PR). I think we don't need an extra issue for that because when EUI version is updated, the tests will start to fail and will need to be updated as well.
]); | ||
}; | ||
export const init = () => { | ||
const server = sinon.fakeServer.create(); |
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.
Slightly unrelated, but there's another pr that is trying to fix the incorrect usage of axios+sinon that we have throughout our apps in order to use HttpSetup
instead. There's a few very nice abstractions that are created there that I can see you also thought about here! I'll create an issue to patch this up later on when the other PR is merged
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.
kbn-test changes LGTM
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.
Thank you for adding these tests @yuliacech. I added one comment affirming your decision to remove the impactLevel
argument from the expectToBeAccessible()
calls.
impactLevel?: ImpactValue | ||
): Promise<void> => { | ||
const violations = await getA11yViolations(component, impactLevel); | ||
expect(violations).toHaveLength(0); |
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.
Great call removing the impactLevel
. Our goal is to find and remove all accessibility issues, regardless of their impact. It has been my experience that serious and moderate issues can cause significant UX problems in large enough quantities. They're not breaking UX issues per se, but each one causes friction and adds to the accumulated cognitive load.
💚 Build SucceededMetrics [docs]Public APIs missing comments
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Summary
This PR adds
jest-axe
library to run axe automated a11y testing in jest. Anaxe_helpers
file inkbn/test-jest-helpers
exports aexpectToBeAccessible
function that can be used directly in tests. As an example, I added a11y tests for Indices tab in Index Management. To reuse a Kibana wide axe configuration, I moved a file with constants intokbn-test
package.Motivation
We can already run axe tests in functional and Cypress tests. Those tests are great for e2e testing as they work with real ES resources in a test cluster. The downside is that those tests can be slow to re-run, because often we need to spin up a new ES test cluster for a "clean slate" (for example, if a test creates an index and fails before clean up has finished). Also it's more difficult to setup an error state in these tests, for example, if a connection to ES is lost. And some conditions can't be changed from inside the test, for example when testing Cloud vs on-prem UI differences.
Because of these reasons, I personally prefer to use CITs - component integration tests. In those tests, we mount a high-order component, for example, Index Management component with tabs for indices, data streams and index templates. We mock Kibana dependencies and API requests and can test all UI interactions including loading and error states. This PR adds a possibility to also test for a11y violations on those UI states.
The downside of a11y testing in CITs that I noticed is that mocked versions of child components can cause false positives or false negatives. For example, EUI components are replaced by their mocked version defined in
testenv
files. This can be updated in the EUI repo (see elastic/eui#5702 and elastic/eui#5709)