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

Axe API integration with Jest and Puppeteer #13241

Merged
merged 4 commits into from Jan 15, 2019

Conversation

@gziolo
Copy link
Member

gziolo commented Jan 8, 2019

Description

Implementation based on #12743.

Hopefully this is th final try to include axe integration with out e2e tests setup. This PR introduces new Jest matcher which takes a Puppeteer page instance and check if it is accesible as shown below:

await expect( page ).toPassAxeTests( {
	include: '.block-editor',
	exclude: '#metaboxes, #post-title-0, .editor-default-block-appender__content',
} );

How has this been tested?

npm run test-e2e with the following patch applied in addition to those changes:

diff --git a/packages/tests-e2e/support/setup-test-framework.js b/packages/tests-e2e/support/setup-test-framework.js
index cc8168644da..89cfc37b127 100644
--- a/packages/tests-e2e/support/setup-test-framework.js
+++ b/packages/tests-e2e/support/setup-test-framework.js
@@ -162,6 +162,7 @@ beforeAll( async () => {
 } );
 
 afterEach( async () => {
+	await expect( page ).toPassAxeTests();
 	await setupBrowser();
 } );

Types of changes

New feature for e2e testing.

Known issues

There is a bug in Jest with async matcher which makes them print differently than regular matchers. See facebook/jest#7066 for more details. They are working on a fix and it looks like it might land in the next major release of Jest.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@gziolo gziolo self-assigned this Jan 8, 2019

@gziolo gziolo force-pushed the add/jest-puppeteer-axe branch from 73baa4d to 5645f37 Jan 8, 2019

@gziolo gziolo removed their assignment Jan 8, 2019

@gziolo gziolo force-pushed the add/jest-puppeteer-axe branch from f66adb2 to e052132 Jan 11, 2019

@gziolo

This comment has been minimized.

Copy link
Member Author

gziolo commented Jan 11, 2019

Logs with accessibility violations (closer to the end of file) for jobs without popular plugins and author role:

To test it locally try the following diff:

diff --git a/packages/tests-e2e/support/setup-test-framework.js b/packages/tests-e2e/support/setup-test-framework.js
index cc8168644da..89cfc37b127 100644
--- a/packages/tests-e2e/support/setup-test-framework.js
+++ b/packages/tests-e2e/support/setup-test-framework.js
@@ -162,6 +162,7 @@ beforeAll( async () => {
 } );
 
 afterEach( async () => {
+	await expect( page ).toPassAxeTests();
 	await setupBrowser();
 } );

@gziolo gziolo force-pushed the add/jest-puppeteer-axe branch from e85c171 to d9699bb Jan 11, 2019

@gziolo gziolo requested review from tofumatt and ntwb Jan 11, 2019

@gziolo

This comment has been minimized.

Copy link
Member Author

gziolo commented Jan 11, 2019

This is ready for review. This PR in the final version adds only new package which exposes the async matcher to validate whether a given page or its part is accessible according to Axe tool analysis.

@stephenmathieson and @dylanb, I would appreciate your feedback on this one as it's based on axe-puppeteer and your great work started at WC US (#12743) :)

@gziolo gziolo requested review from joemcgill and jorgefilipecosta Jan 11, 2019

@greatislander
Copy link
Contributor

greatislander left a comment

I've proposed some changes based on concerns that the function name and documentation could lead users to think that running Axe alone can accurately determine whether or not a page is accessible. Any automated accessibility tool can only catch a certain subset of potential accessibility issues, and must be combined with manual evaluation. It is probably better not to use language like:

expect( page ).toBeAccessible();

But rather to use documentation and function names which indicates that the evaluation is based on criteria of this specific tool, e.g.:

expect( page ).toPassAxeTests();

Open to and grateful for other ideas of how to frame this.

Show resolved Hide resolved packages/jest-puppeteer-axe/README.md Outdated
Show resolved Hide resolved packages/jest-puppeteer-axe/README.md Outdated
Show resolved Hide resolved packages/jest-puppeteer-axe/README.md Outdated
Show resolved Hide resolved packages/jest-puppeteer-axe/README.md Outdated
Show resolved Hide resolved packages/jest-puppeteer-axe/README.md Outdated
Show resolved Hide resolved packages/jest-puppeteer-axe/src/index.js Outdated
Show resolved Hide resolved packages/jest-puppeteer-axe/src/index.js Outdated
Show resolved Hide resolved packages/jest-puppeteer-axe/src/index.js Outdated
Show resolved Hide resolved packages/jest-puppeteer-axe/src/index.js Outdated
Show resolved Hide resolved packages/jest-puppeteer-axe/src/index.js Outdated
@gziolo

This comment has been minimized.

Copy link
Member Author

gziolo commented Jan 14, 2019

I've proposed some changes based on concerns that the function name and documentation could lead users to think that running Axe alone can accurately determine whether or not a page is accessible. Any automated accessibility tool can only catch a certain subset of potential accessibility issues, and must be combined with manual evaluation.

Totally agree with your reasoning 👍 I didn't think about it at all. I was too focused on the technical aspects. Thanks for your review. I applied all the changes you proposed as they better explain the intents of this new Jest matcher introduced. I'm not 100% sure if tests in expect( page ).toPassAxeTests(); is the best fit but I'm having hard time finding out a better alternative. Some ideas I had are:

expect( page ).toPassAxeChecks();
expect( page ).toPassAxeAnalysis();
expect( page ).toPassAxeVerification();

We can totally proceed as is if there isn't a better proposal.

@greatislander

This comment has been minimized.

Copy link
Contributor

greatislander commented Jan 14, 2019

@gziolo thanks for accepting the suggestions! I hear the concern around using Tests in the function name. Of your suggestions I think I'd lean towards:

expect( page ).toPassAxeChecks();

I also thought of:

expect( page ).toPassAxeAudit();

Open to other suggestions too.

@aduth

aduth approved these changes Jan 15, 2019

Copy link
Member

aduth left a comment

The messaging of a failing test is non-conventional, with only an "Error" text being highlighted. In direct conversation, it was mentioned this is a bug present in Jest for the moment. Pending @gziolo to share.

In the meantime, this appears good as it stands. 👍

Show resolved Hide resolved packages/jest-console/package.json Outdated
Show resolved Hide resolved packages/jest-puppeteer-axe/package.json Outdated
Show resolved Hide resolved packages/tests-e2e/support/setup-test-framework.js Outdated

@gziolo gziolo force-pushed the add/jest-puppeteer-axe branch from f96302a to d8c0c28 Jan 15, 2019

@gziolo gziolo force-pushed the add/jest-puppeteer-axe branch from d8c0c28 to 81ea2c5 Jan 15, 2019

Apply suggestions from code review
Co-Authored-By: gziolo <grzegorz@gziolo.pl>

@gziolo gziolo merged commit 2368662 into master Jan 15, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@gziolo gziolo deleted the add/jest-puppeteer-axe branch Jan 15, 2019

@gziolo gziolo added this to the 4.9 (Gutenberg) milestone Jan 15, 2019

@dylanb

This comment has been minimized.

Copy link

dylanb commented Jan 26, 2019

I am a little late on this but this looks like great progress

@dylanb

This comment has been minimized.

Copy link

dylanb commented Jan 26, 2019

@gziolo seems like a good improvement would be to publish a axe-jest-puppeteer module that implements the matcher and use that - WDYT?

cc/ @WilcoFiers @stephenmathieson

@gziolo

This comment has been minimized.

Copy link
Member Author

gziolo commented Jan 28, 2019

This is going to be published to npm as @wordpress/jest-puppeteer-axe closer to the end of February, is it what you mean?

@gziolo gziolo referenced this pull request Jan 29, 2019

Closed

test: add a11y reporting using axe-puppeteer #12743

4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment