Skip to content
This repository has been archived by the owner on Aug 26, 2022. It is now read-only.

fix: Bug 1459547, add pa11y for automated local a11y testing #4768

Closed
wants to merge 2 commits into from
Closed

fix: Bug 1459547, add pa11y for automated local a11y testing #4768

wants to merge 2 commits into from

Conversation

schalkneethling
Copy link

No description provided.

@schalkneethling schalkneethling added comp: frontend Component: Frontend meta: a11y Meta: Accessibility issues / improvements labels May 7, 2018
@schalkneethling schalkneethling added this to the Q2 - Sprint 2 milestone May 7, 2018
@schalkneethling schalkneethling requested a review from jwhitlock May 7, 2018 08:08
@jwhitlock
Copy link
Contributor

What sprint 2 work does this support?

@schalkneethling
Copy link
Author

@jwhitlock Not a specific user story but, general improvement of the Kuma front-end. This is the first step in identifying a11y issues, the next would be to start fixing the problems it highlights.

@jwhitlock jwhitlock removed this from the Q2 - Sprint 2 milestone May 8, 2018
Copy link
Contributor

@jwhitlock jwhitlock left a comment

Choose a reason for hiding this comment

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

I get an error when running this command:

$ npm run test:a11y-dev

> kuma@0.1.0 test:a11y-dev /Users/john/src/kuma
> node pa11y-test.js

(node:13963) UnhandledPromiseRejectionWarning: TimeoutError: Pa11y timed out (30000ms)
    at Timeout.setTimeout [as _onTimeout] (/Users/john/src/kuma/node_modules/p-timeout/index.js:27:54)
    at ontimeout (timers.js:427:11)
    at tryOnTimeout (timers.js:289:5)
    at listOnTimeout (timers.js:252:5)
    at Timer.processTimers (timers.js:212:10)
(node:13963) 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:13963) [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.

Does the command work for you?

@schalkneethling
Copy link
Author

@jwhitlock It does indeed work for me. Did you first start up your local dev environment?

@jwhitlock
Copy link
Contributor

Good idea @schalkneethling. Yes, the environment was running, but I was running an alternate configuration for debugging. In my .env file:

COMPOSE_FILE=docker-compose.yml:docker-compose.dev.yml
DEBUG_TOOLBAR=true

The DEBUG_TOOLBAR is for debugging Django, and the alternate docker-compose.dev.yml runs Django in single-threaded mode, to allow for easier debugging.

I logged the web node with docker-compose logs -f web and tried again.

After running $ npm run test:a11y-dev, it logs a request about a second later:

web_1            | 172.18.0.1 - - [09/May/2018 14:59:24] "GET / HTTP/1.1" 302 -

The command then times out at 30 seconds:

(node:81245) UnhandledPromiseRejectionWarning: TimeoutError: Pa11y timed out (30000ms)
    at Timeout.setTimeout [as _onTimeout] (/Users/john/src/kuma/node_modules/p-timeout/index.js:27:54)
    at ontimeout (timers.js:427:11)
    at tryOnTimeout (timers.js:289:5)
    at listOnTimeout (timers.js:252:5)
    at Timer.processTimers (timers.js:212:10)
(node:81245) 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:81245) [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.

Then at about 57 seconds, there are follow-on requests:

web_1            | 172.18.0.1 - - [09/May/2018 15:02:43] "GET /en-US/ HTTP/1.1" 200 -
web_1            | Error on request:
web_1            | Traceback (most recent call last):
web_1            |   File "/usr/local/lib/python2.7/site-packages/werkzeug/serving.py", line 193, in run_wsgi
web_1            |     execute(self.server.app)
web_1            |   File "/usr/local/lib/python2.7/site-packages/werkzeug/serving.py", line 184, in execute
web_1            |     write(data)
web_1            |   File "/usr/local/lib/python2.7/site-packages/werkzeug/serving.py", line 152, in write
web_1            |     self.send_header(key, value)
web_1            |   File "/usr/local/lib/python2.7/BaseHTTPServer.py", line 412, in send_header
web_1            |     self.wfile.write("%s: %s\r\n" % (keyword, value))
web_1            | IOError: [Errno 32] Broken pipe

There are 7 more requests in rapid succession with similar errors.

After turning off DEBUG_TOOLBAR=true, I was able to run the command, and the pa11y-results folder was populated.

When trying to debug my issue running the command, I wiped out my /node_modules folder and tried re-installing, after making sure I was on npm 6.0.0. I wasn't able to install the project because of errors installing fsevents@1.1.1 and node-sass@4.5.2. I wiped out npm-shrinkwrap.json, and then npm install worked for me. I've uploaded my npm-shrinkwrap.json to https://gist.github.com/jwhitlock/afc46df34d917f6df0581cd008bf34e3. I'm on node v10.1.0, if that matters.

I'm not sure if I have recommended changes yet, but it would be nice if npm install worked. What version of node and npm are you using?

@schalkneethling
Copy link
Author

@jwhitlock Thanks for the feedback. Interesting that having the debug toolbar on causes it to timeout. Are you also running on a different port other than 8000?

I will remove my shrinkwrap and node_modules and test again. That version of fsevents sounds very old. Hmmmm

@schalkneethling
Copy link
Author

@jwhitlock using node v9.10.1 and npm 5.6.0

@codecov-io
Copy link

Codecov Report

Merging #4768 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4768   +/-   ##
=======================================
  Coverage   95.82%   95.82%           
=======================================
  Files         270      270           
  Lines       24568    24568           
  Branches     1750     1750           
=======================================
  Hits        23542    23542           
  Misses        814      814           
  Partials      212      212

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a34764...ecd6207. Read the comment docs.

@schalkneethling
Copy link
Author

Updated. I removed node_modules, removed package-lock and npm-shrinkwrap. Ran npm i and then npm run test:a11y-dev and all worked without incident.

@jwhitlock
Copy link
Contributor

I installed node v8, npm 5.6.0, and the install worked for me too. Snyk is showing me an issue, but I can't see it, it appears to be associated with your user, so I'll let you look into that.

No, I'm running on port 8000. The debug toolbar does interesting things, such as keep connections open so that you can debug them. I have to disable it for other PRs as well. I can add some documentation about that in a different PR.

Sorry that the node stuff is complicating this PR. I'm not 100% comfortable with our node.js story.
Python / Django is dockerized, so if you can run Docker with our config (sorry Windows users), then you get the recommended environment. When we want to update, a developer updates the base image, tests out, and then everyone else can pull the latest image and get the exact same config. It took me a couple of years, but I'm a Docker believer now.

We run node tools to compile static assets, so node v6 is in the docker images, used in TravisCI, etc. For local development, we're recommending node v6, to match the version used to build assets in the docker images, but I think this recommendation is out of date. It may be time to update that recommendation. Some candidates:

  • Node v6 - Maintenance LTS release, used by the docker images, TravisCI. We should update this, so we shouldn't make devs use it for tools like pa11y.
  • Node v8 - Active LTS release until April 2019, used by kumascript. I think this is still a good choice for production until October.
  • Node v9 - Standard release, maintenance ends June 2018. It seems like docs, including the npm docs, recommend avoiding the odd versions if you're having trouble.
  • Node v10 - Current release, LTS in October 2018. Another candidate for the development version, but we may have some pain around major versions.

My preference from a few minutes of thinking about it is to update to node v8 in the deployment environments, and leave it up to you to pick the development version. My initial preference is for v8 for development as well (and standardize on npm install and package.json, but that may be a step too far. It's also tricky to install v8 on macOS, since it is an older version:

brew install node@8
# Add to ~/.bash_profile
PATH="/usr/local/opt/node@8/bin:$PATH"

I don't know if there is a recommended npm version to go with each node version. The one I get with node v8 is 5.6.0, and the installation succeeds with 5.6.0.

I haven't tried with the recommended node v6. I think the easiest thing would be to update the docs to the current recommendation, but please try w/ v6 if you don't want to open that bag of monkeys today.

Copy link
Contributor

@jwhitlock jwhitlock left a comment

Choose a reason for hiding this comment

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

Installation works for me with node v8 and npm 5.6.0. It may work with other configurations. I haven't tested with node v6, the current recommendation for front-end development. I'm hoping @schalkneethling will try it, or update the recommendation.

"svgo": "^1.0.5"
},
"dependencies": {
"npm": "6.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

npm 5.6.0 also works for me, and for whatever reason doesn't seem to install a new command-line npm package. Maybe "npm >= 5.6.0"? Or maybe it is for libraries? We're at the limits of my npm knowledge.

@schalkneethling
Copy link
Author

schalkneethling commented May 10, 2018

Snyk is highlighting a problem with lodash, and it is minor so, we could ignore that for now I reckon. It is also not directly in our control but used by one of the other packages.

Looks to be related to npm-audit-report@1.0.9 › cli-table2@0.2.0 › lodash@3.10.1

I have also invited you to the Snyk dashboard.

@jwhitlock
Copy link
Contributor

Thanks, I can see the Snyk report now. It looks like there is a possible solution, updating to lodash@4.17.5.

@schalkneethling
Copy link
Author

@jwhitlock So, we do not have lodash as a direct dependency so, from the report, it seems like cli-table2 needs to update their dependency[https://github.com/jamestalmage/cli-table2/issues/48] and, then whichever dependency we have need to update their cli-table2 dependency ;)

People are trying to get the maintainer to publish a new release:
jamestalmage/cli-table2#43

It seems that the latest version in master does not use lodash anymore.

@schalkneethling
Copy link
Author

Closing this out, and will use the tools locally. Thx for the review @jwhitlock

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
comp: frontend Component: Frontend meta: a11y Meta: Accessibility issues / improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants