Support aXe test runner #408

rowanmanning · 2018-05-24T11:54:02Z

We now support both the aXe and HTML CodeSniffer test runners - You can
run Pa11y using one or both of these. I've done this by untangling HTML
CodeSniffer from the core code and writing two new modules:

These come bundled with Pa11y so there's no need to install separately
for now. They'll be considered stable when we're ready to release the
code on this branch.

Looking for code review but also for people to test this work out.

How to try this out before we merge/release

Install the in-progress version with:

npm install -g git+ssh://git@github.com:pa11y/pa11y.git#multiple-test-runners

Test with HTML CodeSniffer (same as now):

pa11y https://www.example.com/

Test with aXe:

pa11y --runner axe https://www.example.com/

Test with aXe and HTML CodeSniffer (there will be some duplicates):

pa11y --runner axe --runner htmlcs https://www.example.com/

This PR resolves #399.

andrewmee

Just a few minor things I've caught reading through this;

andrewmee · 2018-05-25T08:35:24Z

README.md

+Runners
+-------
+
+Pa11y supports multiple test runners. which return different results. The built-in test runners are:


Accidental . after runners

andrewmee · 2018-05-25T08:56:58Z

lib/runner.js

@@ -226,29 +108,138 @@
 		}

 		/**
-		 * Check whether an issue is in the test area specified by rootElement.
+		 * Check whether an element is in the test area specified by rootElement.


I like this change; using element rather than issue. Makes more sense to me!

andrewmee · 2018-05-25T10:06:23Z

test/unit/lib/runner.test.js


-				rootElement.contains.withArgs(insideElement).returns(true);
-				rootElement.contains.withArgs(outsideElement).returns(false);
+			describe('when the element HTML is short enough to not need truncating', () => {


Duplicate test

andrewmee · 2018-05-25T10:09:11Z

test/unit/lib/runner.test.js

+				});
+
+				it('returns the element HTML unmodified', () => {
+					assert.isNull(returnValue);


The test case description doesn't match the result; "returns null"

andrewmee

👌

hollsk

I think this looks great. Found it very easy to read, lots of debugging messages, good docu 👍

(haven't tried actually running it yet 😳- may have more comments once I do!)

hollsk · 2018-06-08T17:35:19Z

lib/pa11y.js

+function loadRunnerFile(runner) {
+	try {
+		return require(`pa11y-runner-${runner}`);
+	} catch (error) {}


genuine question, not a review comment per se: why doesn't this catch block do anything?

Oh because if requiring pa11y-runner-XXX fails, then we want to try requiring XXX immediately afterwards. So we ignore this error :)

Kolombiken · 2018-06-11T14:52:50Z

Hello!
Tried running it with different flags on the same site. Noticed one thing that was a bit strange to me. I got a number of errors in the first run and a different number in the second run. Any clues as to why?

This is what I did:

Run pa11y --runner axe --runner htmlcs https://my-test-site.com got 18 errors
Run pa11y https://my-test-site.com and got 14 errors
Run pa11y --runner axe --runner htmlcs https://my-test-site.com got 20 errors

After this I tested three other sites but they produced a consistent number of errors. So maybe it was just a glitch?

Also running pa11y --runner axe https://my-other-test-site.com got 8 errors and running pa11y --runner htmlcs https://my-other-test-site.com got me 79 errors. But Running them both got me 99 errors.
Now I'm not an export in math but I did expected 87 errors (79+8) instead of 99.

Last feedback. Would be cool if/when running pa11y --runner axe --runner htmlcs https://my-test-site.com I could see which errors comes from which tool.
I'm able to figure it out by looking at the messages but the output makes it a bit hard to scan.

All in all good job! 👍

hollsk · 2018-07-02T11:05:42Z

I've given it a run through to try to replicate the problems described by @Kolombiken above. I do see inconsistencies, but they're well within normal variance caused by third-party changes or things failing to load. Do you have ads running on any of your test pages, Kolombiken? That's usually my first suspect 😉

Here are my test results:

** bbc.co.uk **

axe + htmlcs = 6
axe + htmlcs = 7
axe + htmlcs = 6

axe = 2
axe = 2
axe = 2

htmlcs = 4
htmlcs = 4
htmlcs = 5

** github.com **

axe + htmlcs = 15
axe + htmlcs = 15
axe + htmlcs = 15

axe = 8
axe = 8
axe = 8

htmlcs = 7
htmlcs = 7
htmlcs = 7

** lemond.fr **

axe + htmlcs = 329
axe + htmlcs = 328
axe + htmlcs = 329

axe = 10
axe = 10
axe = 9

htmlcs = 315
htmlcs = 316
htmlcs = 318

I also tried The Verge because they have a ton of ads and I was expecting some very irregular results! But it doesn't run Axe - I get the following error:

Launching Headless Chrome
Opening URL in Headless Chrome
Setting request method
Setting request headers
Loading runner: axe
Injecting Pa11y
Injecting runner: axe
Running Pa11y on the page
(node:9841) UnhandledPromiseRejectionWarning: Error: Evaluation failed: DOMException: Failed to execute 'querySelector' on 'Document': The provided selector is empty.
    at processIssue (<anonymous>:56:35)
    at processIncomplete (<anonymous>:43:10)
    at Array.map (<anonymous>)
    at runAxeCore (<anonymous>:20:22)
    at ExecutionContext.evaluateHandle (/Users/hollsk/repositories/git/pa11y/node_modules/puppeteer/lib/ExecutionContext.js:88:13)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:188:7)
(node:9841) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:9841) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

Are we able to catch that, or is it something we should bump upstream? HTML_CS runs fine.

rowanmanning · 2018-07-24T09:01:58Z

Hmm, the aXe Chrome extension works fine on The Verge, I'll investigate

thibaudcolas · 2019-04-08T13:38:07Z

Is there any way to help getting this PR merged? I've been looking into pa11y for my automated accessibility testing needs, and axe support is the only thing that I find lacking – looking at this PR, it feels really close!

I have tried this WIP briefly and everything seems to be working fine. Can do more testing if it helps!

josebolos · 2019-05-15T12:31:41Z

This looks great! I just have a few comments concerning the ability to run two different runners at the same time, regarding mainly the consistency between CLI/webservice/dashboard and the ease of maintenance.

In my opinion, being able to run two runners at the same time in the same pa11y run introduces some complexity that may not be desirable:

Most CLI tools tend to adhere to the UNIX philosophy (Make each program do one thing well. To do a new job, build afresh rather than complicate old programs by adding new "features".) and, in that regard, running pa11y twice with different settings seems to be easier to understand and more consistent with what other tools do, that running pa11y once with both runners and getting two different lists of results.
Due to this, there's the potential for users that opt in for this feature but don't read the docs to get confused by this behaviour ("What I'm getting two different lists of issues?", "Why are they formatted differently?", "Why their results don't match?") which means more support work for us (and we're already stretched thin enough as it is).
In a similar way, there's the potential for users wanting or expecting both results to be made available simultaneously to pa11y-webservice/pa11y-dashboard, which is not simple at all both in terms of UX and amount of work required, compared to just adding a toggle to choose between CS and axe. I understand that there has been mention of standardising error codes, but that doesn't seem particularly easy to do (again, considering our currently available resources).
As a result, inconsistency from the users point of view between running pa11y in the CLI (where you can choose one runner, or the other, or both) and running it from pa11y-dashboard, where we could hopefully make a CS/AXE toggle available easily, but making the data from both available may be much harder.

I would say that allowing to use one runner, or the other, but not both in the same run, may probably make our life easier now and in the future.

Other than that, top job 💯!

thibaudcolas · 2019-07-04T16:29:14Z

Update from #408 (comment) – I’ve been using this branch for my accessibility tests over the last 3 months, and it’s been working really well as far as I can tell.

I was mostly interested in using Axe instead of HTML_CS, but I ended up using the two on the assumption that this would potentially surface more errors, which I think has proven true – but not necessarily by that much. Personally I didn’t find it particularly confusing that it would be returning two sets of results, with different error codes and a lot of duplicates – that’s what you get for running two tools at once 😄Having said that I’d agree that this is a potential source of confusion, especially if surfaced in a web interface (CLI users would presumably have a much greater awareness of how exactly Pa11y has been configured).

I looked into de-duplicating Axe and HTML_CS errors (or at least standardising error format as @josebolos suggests), and tried a few options – what I settled on was to map both types of results to the corresponding WCAG success criteria, as a common reference. This wouldn’t de-duplicate the errors, but at least group them so duplicate errors are likely to be displayed side by side in my reports (thus easy to de-dupe by hand if desirable). The corresponding mapping is available here if it helps anyone, along with a lot more code that generates my reports: https://github.com/thibaudcolas/wagtail-tooling/blob/master/accessibility/docs/success-criteria-mapping.json. This is still very WIP, but felt like it would be worth sharing nonetheless.

For the purpose of this PR – If the API changes to only support a single runner, then I think people who want both Axe and HTML_CS will still be able to create their own custom pa11y-runner-axe-and-htmlcs using both under the hood if they wish to, so this doesn’t seem like a big departure from the perspective of users.

Again, I’m really interested in helping getting Axe support merged and released, so do let me know if there is anything I can help with beyond testing.

joeyciechanowicz · 2019-07-05T08:05:58Z

In my opinion, being able to run two runners at the same time in the same pa11y run introduces some complexity that may not be desirable

I've been testing out this branch and it seems great. Initially I was shared your view @josebolos; that two runners complicates things. But the more i've mulled it over, the more I beleive it's the correct approach.
By supporting multiple runners we can can easily enhance pa11y with additional functionality. Say someone brings out a tool that can check for sentance complexity, that could be added as an additional runner and easily incorporated into peoples workflows.

rowanmanning

This looks great to me! Thanks for getting everything fixed up :)

This will need to be done before we can ship though, otherwise we regress from HTMLCS 2.4 to 2.2 pa11y/pa11y-runner-htmlcs#2

joeyciechanowicz · 2019-07-22T10:23:36Z

@rowanmanning Good catch! pa11y was updated to use htmlcs from NPM already, so should be simple to make it work in the runner as well.

We now support both the aXe and HTML CodeSniffer test runners - You can run Pa11y using one or both of these. I've done this by untangling HTML CodeSniffer from the core code and writing two new modules: - https://github.com/pa11y/pa11y-runner-axe - https://github.com/pa11y/pa11y-runner-htmlcs These come bundled with Pa11y so there's no need to install separately for now. They'll be considered stable when we're ready to release the code on this branch.

This fixes the issue with The Verge

rowanmanning mentioned this pull request May 24, 2018

Untangle HTML CodeSniffer and investigate supporting different runners #399

Closed

andrewmee requested changes May 25, 2018

View reviewed changes

andrewmee approved these changes May 30, 2018

View reviewed changes

hollsk approved these changes Jun 8, 2018

View reviewed changes

rowanmanning force-pushed the multiple-test-runners branch from ac52263 to 00174f1 Compare July 24, 2018 13:23

hollsk mentioned this pull request Sep 18, 2018

pa11y WCAG 2.1 support #425

Closed

rowanmanning force-pushed the multiple-test-runners branch from 114cd28 to 82e957b Compare October 23, 2018 15:49

This was referenced Apr 8, 2019

[Discussion] choose appropriate tool for WCAG auto testing / visual regression tests wagtail/wagtail#4871

Closed

WCAG 2.1 Support #436

Closed

rowanmanning force-pushed the multiple-test-runners branch from 5d35cba to fa939bf Compare April 16, 2019 11:22

joeyciechanowicz force-pushed the multiple-test-runners branch from fa939bf to a797862 Compare June 25, 2019 09:57

rowanmanning commented Jul 22, 2019

View reviewed changes

rowanmanning and others added 7 commits July 23, 2019 14:27

Fix a copy/paste error

2e22f4c

Action review comments from @andrewmee

1296ee8

Update to the latest version of the aXe runner

b54c206

This fixes the issue with The Verge

Update documentation for HTMLCS-only configurations

5a8f6c0

Use stable versions of HTMLCS and aXe test runners

d0223f1

Tests error is thrown when ignoreUrl is used without a page

eb704f3

joeyciechanowicz force-pushed the multiple-test-runners branch from a797862 to eb704f3 Compare July 23, 2019 13:28

joeyciechanowicz added 2 commits July 23, 2019 14:35

Sync package-lock.json to package.json

cd12459

Caches promises for runner scripts rather than global state

1c7a282

joeyciechanowicz merged commit e044480 into master Jul 24, 2019

joeyciechanowicz deleted the multiple-test-runners branch July 24, 2019 10:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support aXe test runner #408

Support aXe test runner #408

rowanmanning commented May 24, 2018 •

edited

andrewmee left a comment

andrewmee May 25, 2018

andrewmee May 25, 2018

andrewmee May 25, 2018

andrewmee May 25, 2018

andrewmee left a comment

hollsk left a comment •

edited

hollsk Jun 8, 2018

rowanmanning Jun 11, 2018

hollsk Jun 11, 2018

Kolombiken commented Jun 11, 2018 •

edited

hollsk commented Jul 2, 2018 •

edited

rowanmanning commented Jul 24, 2018

thibaudcolas commented Apr 8, 2019

josebolos commented May 15, 2019

thibaudcolas commented Jul 4, 2019 •

edited

joeyciechanowicz commented Jul 5, 2019

rowanmanning left a comment

joeyciechanowicz commented Jul 22, 2019

Support aXe test runner #408

Support aXe test runner #408

Conversation

rowanmanning commented May 24, 2018 • edited

How to try this out before we merge/release

andrewmee left a comment

Choose a reason for hiding this comment

andrewmee May 25, 2018

Choose a reason for hiding this comment

andrewmee May 25, 2018

Choose a reason for hiding this comment

andrewmee May 25, 2018

Choose a reason for hiding this comment

andrewmee May 25, 2018

Choose a reason for hiding this comment

andrewmee left a comment

Choose a reason for hiding this comment

hollsk left a comment • edited

Choose a reason for hiding this comment

hollsk Jun 8, 2018

Choose a reason for hiding this comment

rowanmanning Jun 11, 2018

Choose a reason for hiding this comment

hollsk Jun 11, 2018

Choose a reason for hiding this comment

Kolombiken commented Jun 11, 2018 • edited

hollsk commented Jul 2, 2018 • edited

rowanmanning commented Jul 24, 2018

thibaudcolas commented Apr 8, 2019

josebolos commented May 15, 2019

thibaudcolas commented Jul 4, 2019 • edited

joeyciechanowicz commented Jul 5, 2019

rowanmanning left a comment

Choose a reason for hiding this comment

joeyciechanowicz commented Jul 22, 2019

rowanmanning commented May 24, 2018 •

edited

hollsk left a comment •

edited

Kolombiken commented Jun 11, 2018 •

edited

hollsk commented Jul 2, 2018 •

edited

thibaudcolas commented Jul 4, 2019 •

edited