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

Untangle HTML CodeSniffer and investigate supporting different runners #399

Open
rowanmanning opened this Issue Apr 10, 2018 · 25 comments

Comments

Projects
None yet
10 participants
@rowanmanning
Member

rowanmanning commented Apr 10, 2018

I have a plan that's come about after a chat with @cfq. Gonna run it past people here before I steam ahead, but pretty excited about working on this.

  1. I'm going to untangle HTML CodeSniffer from Pa11y core. It'll still live in the repo but it will be abstracted into a standard interface for test runners, which also converts HTMLCS output into Pa11y output. I don't imagine this taking me very long and I think it's worthwhile doing regardless of the next steps.

  2. Me and/or @cfq will investigate plugging aXe core into Pa11y, as it has much more active development than HTMLCS and also covers more error cases. So Pa11y 5.x might end up with the ability to use multiple test runners.

  3. If we do number 2, then we'll also investigate the idea of standardised error codes which work across each of the runners we plug in. One of the larger pieces of work done by GDS was compiling a list of every a11y issue that's possible to automate tests for, their research is really extensive. If we can map the error codes for each test runner we support against this list then we'd be able to dedupe results from multiple test runners at once and cover more errors.

Let me know what you think 🙂

@andrewmee

This comment has been minimized.

Member

andrewmee commented Apr 10, 2018

This sounds very exciting. Mapping error codes though; so we'd maintain that mapping for each of the available test runners that we implement?

@rowanmanning

This comment has been minimized.

Member

rowanmanning commented Apr 10, 2018

Yeah, slightly nervous about how much work that'd be but we won't know until we investigate. The fallback would be fairly graceful though – it could default to the runner-specific error code and there would just be potential duplicates for any new errors the runners have added.

Definitely more something to investigate after we've got a second runner plugged in

@andrewmee

This comment has been minimized.

Member

andrewmee commented Apr 10, 2018

Indeed! Have you looked at aXe or any of the other runners to see if they offer comparable output to what we get from HTMLCS to populate our issues object? Like I wonder if not all runners will provide... context for example, or selector.

Or that's definitely the case and we're just going to run with it?

@rowanmanning

This comment has been minimized.

Member

rowanmanning commented Apr 10, 2018

I think that'll be part of the investigation. We calculate the selector and context based on the actual node that HTMLCS is complaining about, so as long as the other tools include a pointer to a DOM node then we can generate them

@dylanb

This comment has been minimized.

dylanb commented Apr 10, 2018

We might be willing to help you guys with this work. If you want to talk about it, let me know.

@hollsk

This comment has been minimized.

Member

hollsk commented Apr 10, 2018

I don't have much to add other than that this is extremely exciting!

Mapping the error codes to a standard format was also how I imagined us doing this when we talked about it previously. In my head it worked similar to the reporters, where they're broken out of Pa11y core and rely on a standard API. I don't know how feasible that would be here, though.

@cfq

This comment has been minimized.

cfq commented Apr 12, 2018

@hollsk When we were doing the automated testing tool audit at GDS we did something very similar - created a standardised list of problems and tested all tools on it. It's definitely not easy but it is doable. I don't think it'd take long if 3-4 people did it whenever they can.

@rowanmanning

This comment has been minimized.

Member

rowanmanning commented Apr 13, 2018

Still just in a local experimental branch, but step 1 and 2 came together quite quickly:

Command-line output for Pa11y running axe-core

I'm starting to wonder whether we can get this usable without a breaking change. I think we can, but the interface may be a little confusing and untidy 😔

@HugoGiraudel

This comment has been minimized.

HugoGiraudel commented Apr 13, 2018

We run pa11y on hundreds of pages at N26, and use aXe-core (through react-aXe) in development. If you come up with a build we can test, we’d be happy to give it a go.

@mgifford

This comment has been minimized.

mgifford commented May 14, 2018

@rowanmanning this is great. Do you have anything up in a sandbox that folks can look at. Would be great to have some input from the aXe-core folks too. Think they'd be keen.

@WilcoFiers

This comment has been minimized.

WilcoFiers commented May 14, 2018

Axe-core people present :) How can we help?

@mgifford

This comment has been minimized.

mgifford commented May 14, 2018

@WilcoFiers What's the best way to the integrate engine Axe-core into pa11y?

Has something like this been done?

@rowanmanning

This comment has been minimized.

Member

rowanmanning commented May 14, 2018

Hey all, I've got something up-and-running and I just need to find the time to write some more tests and polish :) I'll try and get something released this week that's runnable by other people so that you can feed back on the implementation

@hollsk

This comment has been minimized.

Member

hollsk commented May 14, 2018

So exciting!

@WilcoFiers

This comment has been minimized.

WilcoFiers commented May 14, 2018

@mgifford I don't know enough about Pa11y to say. I'd be happy to set up a call to look at the question, but it sounds like @rowanmanning has things under control. I'll keep an eye on the topic, when there's stuff to review I'll be sure to hare my 2 cents.

@rowanmanning

This comment has been minimized.

Member

rowanmanning commented May 16, 2018

I need to make a decision about whether to bundle the aXe runner with Pa11y. If we do this, then you'll just be able to immediately run pa11y --runner axe https://example.com. Otherwise you'll need to run npm install pa11y-runner-axe first.

Maybe add a 👍 on this comment to bundle aXe, or a 👎 to not bundle.

Pros/cons of bundling:

Pros

  • Ease of use
  • We're more likely to get people testing it
  • We maybe want to switch to aXe by default anyway

Cons

  • We couldn't back-track and remove this without a breaking change
  • The overall dependency size increases
@mgifford

This comment has been minimized.

mgifford commented May 16, 2018

How do the results compare? Not sure what the bias' are for HTML CodeSniffer vs aXe-core. Is there a reason that someone would still prefer to run HTML CodeSniffer? How do the reports change?

If it's about 2 commands vs 1, that's not a big barrier at all. Just depends on what you think the default should be when folks run it.

@rowanmanning

This comment has been minimized.

Member

rowanmanning commented May 16, 2018

I'm not 100% sure how the results compare yet, but aXe is generally considered the better choice; @cfq has done a lot of research and may be better placed to answer this.

Either way the default will have to remain as HTML CodeSniffer until we're ready to ship another major version. If aXe performs better then we'll switch to it as a default in Pa11y 6.0 and make HTML CodeSniffer an installable extra

@WilcoFiers

This comment has been minimized.

WilcoFiers commented May 17, 2018

@rowanmanning Is there a definition of "performs better"? Just curious. I would argue that the best reasons for switching to Axe are the following:

  • False positives are treated as bug, if you report them, they'll be fixed
  • It is widely used, including in Lighthouse and SonarWhal, so you won't get conflicting results if you use multiple tools
  • There are several organisations heavily invested in ensuring it remains a high quality tool (Deque, Google, Microsoft, DAISY)
  • Authors of axe-core are heavily involved in harmonised accessibility testing work in the W3C
@rianrietveld

This comment has been minimized.

rianrietveld commented May 18, 2018

100% agree with what @WilcoFiers says.

@kaelig

This comment has been minimized.

kaelig commented May 18, 2018

Happy to help test a branch, as we're using pa11y on Polaris, Shopify's design system (the site itself for now, and soon on the components).

@rowanmanning

This comment has been minimized.

Member

rowanmanning commented May 21, 2018

OK current status on this:

Pa11y

The branch on Pa11y which uses aXe is nearing completion. I now need to update the command line interface to accept --runner(s) arguments. This shouldn't take me too long. After that I'd like to work out how we can pass specific aXe or HTML CodeSniffer configurations to the runners as you currently can't configure them. Something along the lines of a --runner-config-<runner>-<configuration> flag?

Runners

I've split out the test runners into separate repos/modules. These are ready to be reviewed if anyone fancies looking at the code. Unfortunately you can't actually run them very easily yet, if you'd rather wait until you can run them with a beta release of Pa11y then that's fine.

@kaelig

This comment has been minimized.

kaelig commented May 21, 2018

Any chance pa11y-ci could be configured out of the box to work with the aXe runner?

@rowanmanning

This comment has been minimized.

Member

rowanmanning commented May 21, 2018

Hey Kaelig :) again only with a major version bump. But whenever we bump the Pa11y major version we do that anyway, so if Pa11y v6 uses aXe by default then so will Pa11y CI v3.

In the meantime we'll make sure it's as easy as possible for Pa11y CI to specify an aXe runner

@rowanmanning

This comment has been minimized.

Member

rowanmanning commented May 24, 2018

Ta-da! #408 ready for review and testing, instructions on how to run Pa11y plus aXe in the PR

@pa11y pa11y locked as resolved and limited conversation to collaborators May 24, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.