Skip to content
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

[APM] add cypress-axe to e2e tests to check A11y #126610

Merged
merged 3 commits into from
Mar 3, 2022

Conversation

MiriamAparicio
Copy link
Contributor

Added cypress-axe to e2e tests to check A11y

#104692

For now, we are going to set the parameter skipFailures to true, this will log the errors but won't make the test fails, issues to tackle the failures will be created afterwards

@MiriamAparicio MiriamAparicio added Project:Accessibility Team:APM All issues that need APM UI Team support release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v8.2.0 labels Mar 1, 2022
@MiriamAparicio MiriamAparicio requested a review from a team as a code owner March 1, 2022 16:01
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-accessibility (Project:Accessibility)

@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

@@ -93,6 +105,9 @@ describe('Dependencies', () => {
cy.contains('a', 'postgresql').click({ force: true });

cy.contains('h1', 'postgresql');

// set skipFailures to true to not fail the test when there are accessibility failures
checkA11y(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be in a separate it

Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one comment about a best practice option for bumping code coverage. Thank you!

timeRange
)}`
);
cy.contains('a[role="tab"]', 'Dependencies');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is a second tab that can be interacted with, it'd be great to click on that tab and make a second checkA11y call to bump code coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @1Copenut, this is our first approach to the A11y checks in our pages, we are going to add more checks in the future for now we are just checking the first render and not the interactions.

APM Service Overview contains multiple tabs, it's good to know that we can bump the code coverage if we check A11y in all of them.


const axeConfig = {
...AXE_CONFIG,
rules: [...AXE_CONFIG.rules],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this the same with just using AXE_CONFIG?

it('has no detectable a11y violations on load', () => {
cy.visit(serviceTransactionsHref);
cy.contains('a[role="tab"]', 'Transactions').should(
'have.class',
Copy link
Contributor

@gbamparop gbamparop Mar 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to avoid depending on eui classes. If we need to check that a tab is selected, we could instead use aria-selected="true" for example.

runOnly: [...AXE_OPTIONS.runOnly, 'best-practice'],
};

export const checkA11y = (skipFailures: boolean) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we wrap cy.checkA11y in our own command instead of calling the function?

@@ -53,6 +54,12 @@ describe('When navigating to the service inventory', () => {
cy.visit(serviceInventoryHref);
});

it('has no detectable a11y violations on load', () => {
cy.contains('Services');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking some text on the page could be included in another test as it's not specific to accessibility (same with other files)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shows an error if trying to call checkA11y() directly

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean without having anything else in the test? What error do you get?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think cy.contains('Services'); is needed to wait for that element to be visible before running the a11y check. Would be better if this was more obvious from the code

Copy link
Contributor

@gbamparop gbamparop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks for adding this! It's great that we now have automation for accessibility testing and we can start adding more to it! I've added some minor comments on the PR.

);
cy.contains('a[role="tab"]', 'Dependencies');
// set skipFailures to true to not fail the test when there are accessibility failures
checkA11y(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make it clear what true means:

Suggested change
checkA11y(true);
checkA11y({ skipFailures: true });

cy.injectAxe();
cy.configureAxe(axeConfig);
const context = '.kbnAppWrapper'; // Scopes a11y checks to only our app
cy.checkA11y(context, axeOptions, undefined, skipFailures);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's undefined ?

I'd prefer this:

Suggested change
cy.checkA11y(context, axeOptions, undefined, skipFailures);
cy.checkA11y( { context, axeOptions, skipFailures });

But I assume that's out of our control.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't change that unfortunately

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we can get rid of the last two params when we don't need to add skipFailures, I added it now until we fix the failures.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sqren The undefined here is a placeholder for an optional violations callback function that ultimately would be passed to the axe-core lib. The cypress-axe docs do a good job unpacking the arguments if you're curious: https://www.npmjs.com/package/cypress-axe#user-content-parameters-on-cychecka11y-axerun

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@MiriamAparicio MiriamAparicio merged commit 2880a75 into elastic:main Mar 3, 2022
@MiriamAparicio MiriamAparicio deleted the add-a11y-test-to-apm branch March 3, 2022 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Project:Accessibility release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants