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
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/
import { synthtrace } from '../../../synthtrace';
import { opbeans } from '../../fixtures/synthtrace/opbeans';
import { checkA11y } from '../../support/commands';

const start = '2021-10-10T00:00:00.000Z';
const end = '2021-10-10T00:15:00.000Z';
Expand Down Expand Up @@ -43,6 +44,17 @@ describe('Dependencies', () => {

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

it('has no detectable a11y violations on load', () => {
cy.visit(
`/app/apm/services/opbeans-java/dependencies?${new URLSearchParams(
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.

// 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 });

});
});

describe('dependency overview page', () => {
Expand All @@ -62,6 +74,18 @@ describe('Dependencies', () => {

cy.contains('h1', 'opbeans-java');
});

it('has no detectable a11y violations on load', () => {
cy.visit(
`/app/apm/backends/overview?${new URLSearchParams({
...timeRange,
backendName: 'postgresql',
})}`
);
cy.contains('h1', 'postgresql');
// set skipFailures to true to not fail the test when there are accessibility failures
checkA11y(true);
});
});

describe('service overview page', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import url from 'url';
import { synthtrace } from '../../../../synthtrace';
import { checkA11y } from '../../../support/commands';
import { generateData } from './generate_data';

const start = '2021-10-10T00:00:00.000Z';
Expand Down Expand Up @@ -39,6 +40,13 @@ describe('Error details', () => {
await synthtrace.clean();
});

it('has no detectable a11y violations on load', () => {
cy.visit(errorDetailsPageHref);
cy.contains('Error group 00000');
// set skipFailures to true to not fail the test when there are accessibility failures
checkA11y(true);
});

describe('when error has no occurrences', () => {
it('shows an empty message', () => {
cy.visit(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import url from 'url';
import { synthtrace } from '../../../../synthtrace';
import { checkA11y } from '../../../support/commands';
import { generateData } from './generate_data';

const start = '2021-10-10T00:00:00.000Z';
Expand Down Expand Up @@ -41,6 +42,13 @@ describe('Errors page', () => {
await synthtrace.clean();
});

it('has no detectable a11y violations on load', () => {
cy.visit(javaServiceErrorsPageHref);
cy.contains('Error occurrences');
// set skipFailures to true to not fail the test when there are accessibility failures
checkA11y(true);
});

describe('when service has no errors', () => {
it('shows empty message', () => {
cy.visit(nodeServiceErrorsPageHref);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import url from 'url';
import { synthtrace } from '../../../../synthtrace';
import { opbeans } from '../../../fixtures/synthtrace/opbeans';
import { checkA11y } from '../../../support/commands';

const timeRange = {
rangeFrom: '2021-10-10T00:00:00.000Z',
Expand Down Expand Up @@ -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

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

it('has a list of services', () => {
cy.contains('opbeans-node');
cy.contains('opbeans-java');
Expand Down Expand Up @@ -93,7 +100,7 @@ describe('When navigating to the service inventory', () => {
cy.wait(aliasNames);
});

it.skip('when selecting a different time range and clicking the update button', () => {
it('when selecting a different time range and clicking the update button', () => {
cy.wait(aliasNames);

cy.selectAbsoluteTimeRange(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import url from 'url';
import { synthtrace } from '../../../../synthtrace';
import { opbeans } from '../../../fixtures/synthtrace/opbeans';
import { checkA11y } from '../../../support/commands';

const start = '2021-10-10T00:00:00.000Z';
const end = '2021-10-10T00:15:00.000Z';
Expand Down Expand Up @@ -102,6 +103,13 @@ describe('Service Overview', () => {
cy.loginAsReadOnlyUser();
cy.visit(baseUrl);
});

it('has no detectable a11y violations on load', () => {
cy.contains('opbeans-node');
// set skipFailures to true to not fail the test when there are accessibility failures
checkA11y(true);
});

it('transaction latency chart', () => {
cy.get('[data-test-subj="latencyChart"]');
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,12 @@
import url from 'url';
import { synthtrace } from '../../../../synthtrace';
import { opbeans } from '../../../fixtures/synthtrace/opbeans';
import { checkA11y } from '../../../support/commands';

const start = '2021-10-10T00:00:00.000Z';
const end = '2021-10-10T00:15:00.000Z';

const serviceOverviewHref = url.format({
const serviceTransactionsHref = url.format({
pathname: '/app/apm/services/opbeans-node/transactions',
query: { rangeFrom: start, rangeTo: end },
});
Expand All @@ -35,8 +36,18 @@ describe('Transactions Overview', () => {
cy.loginAsReadOnlyUser();
});

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.

'euiTab-isSelected'
);
// set skipFailures to true to not fail the test when there are accessibility failures
checkA11y(true);
});

it('persists transaction type selected when navigating to Overview tab', () => {
cy.visit(serviceOverviewHref);
cy.visit(serviceTransactionsHref);
cy.get('[data-test-subj="headerFilterTransactionType"]').should(
'have.value',
'request'
Expand Down
23 changes: 23 additions & 0 deletions x-pack/plugins/apm/ftr_e2e/cypress/support/commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@
*/
import 'cypress-real-events/support';
import { Interception } from 'cypress/types/net-stubbing';
import 'cypress-axe';
import {
AXE_CONFIG,
AXE_OPTIONS,
} from 'test/accessibility/services/a11y/constants';

Cypress.Commands.add('loginAsReadOnlyUser', () => {
cy.loginAs({ username: 'apm_read_user', password: 'changeme' });
Expand Down Expand Up @@ -78,3 +83,21 @@ Cypress.Commands.add(
});
}
);

// A11y configuration

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?

};
const axeOptions = {
...AXE_OPTIONS,
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?

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

};