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

Run aXe on a few pages during CI #344

Merged
merged 12 commits into from
Jul 12, 2017
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ You will need to have the following installed on your machine before following t
1. Ruby v2.2.2+, [Installation guides](https://www.ruby-lang.org/en/documentation/installation/)
1. Node v4.2.3+, [Installation guides](https://nodejs.org/en/download/)
1. Bundler v1.12.3+, [Installation guides](http://bundler.io/v1.13/guides/using_bundler_in_application.html#getting-started---installing-bundler-and-bundle-init)

1. Chrome v59 or higher (v60 if on Windows)

### Building the documentation with gulp

Expand All @@ -43,7 +43,7 @@ Here are a few other utility commands you may find useful:

- `npm run lint`: Runs `eslint` and `sass-lint` against JavaScript and Sass files.

- `npm test`: Runs `npm run lint` and can also be used to run any tests.
- `npm test`: Runs all tests and linters.

- `npm run watch`: Runs a series of commands that watches for any changes in both the Standards node module and the root level asset folders in this repo.

Expand Down
5 changes: 5 additions & 0 deletions circle.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@ machine:
dependencies:
pre:
- bundle install
# https://discuss.circleci.com/t/using-the-latest-version-of-chrome-on-circleci/871/4
- curl -L -o google-chrome.deb https://dl.google.com/linux/direct/google-chrome-stable_current_amd64.deb
- sudo dpkg -i google-chrome.deb
- sudo sed -i 's|HERE/chrome\"|HERE/chrome\" --disable-setuid-sandbox|g' /opt/google/chrome/google-chrome
- rm google-chrome.deb

test:
pre:
Expand Down
19 changes: 5 additions & 14 deletions config/crawl.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ const express = require('express');
const Crawler = require("simplecrawler");
const chalk = require('chalk');

const app = express();
const runServer = require('./static-server');

// These pages incorporate content from other files in other repos, so
// they should be considered "second class" by the link checker, and
Expand Down Expand Up @@ -33,17 +33,8 @@ function shouldFetch(item, referrerItem) {
return true;
}

if (fs.existsSync(SITE_PATH)) {
app.use(express.static(SITE_PATH));
} else {
console.log(`Please build the site before running me.`);
process.exit(1);
}

const listener = app.listen(() => {
const port = listener.address().port;
const baseURL = `http://127.0.0.1:${port}`;
const crawler = new Crawler(`${baseURL}/`);
runServer().then(server => {
const crawler = new Crawler(`${server.url}/`);
const referrers = {};
const notFound = [];

Expand All @@ -65,7 +56,7 @@ const listener = app.listen(() => {
notFound.push(item);
});
crawler.on("complete", () => {
listener.close(() => {
server.httpServer.close(() => {
let errors = 0;
let warnings = 0;

Expand All @@ -85,7 +76,7 @@ const listener = app.listen(() => {
});

WARNING_PAGES.forEach(path => {
if (!(`${baseURL}${path}` in referrers)) {
if (!(`${server.url}${path}` in referrers)) {
console.log(`${ERROR}: ${path} was not visited!`);
console.log(` If this is not an error, please remove the path ` +
`from WARNING_PAGES.`);
Expand Down
152 changes: 152 additions & 0 deletions config/run-axe.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,152 @@
'use strict';

const fs = require('fs');
const urlParse = require('url').parse;
const chromeLauncher = require('chrome-launcher');
const CDP = require('chrome-remote-interface');
const chalk = require('chalk');
const runServer = require('./static-server');

const REMOTE_CHROME_URL = process.env['REMOTE_CHROME_URL'];
const AXE_JS = fs.readFileSync(__dirname + '/../node_modules/axe-core/axe.js');
const PAGES = [
'/',
'/page-templates/landing/',
'/page-templates/docs/',
];

function launchChromeLocally(headless=true) {
return chromeLauncher.launch({
chromeFlags: [
'--window-size=412,732',
'--disable-gpu',
headless ? '--headless' : ''
]
});
}

function getRemoteChrome() {
const info = urlParse(REMOTE_CHROME_URL);
if (info.protocol !== 'http:')
throw new Error(`Unsupported protocol: ${info.protocol}`);
return new Promise(resolve => {
resolve({
host: info.hostname,
port: info.port,
kill() { return Promise.resolve(); }
});
});
}

// This function is only here so it can be easily .toString()'d
// and run in the context of a web page by Chrome. It will not
// be run in the node context.
function runAxe() {
return new Promise((resolve, reject) => {
window.axe.run((err, results) => {
if (err) return reject(err);
resolve(JSON.stringify(results.violations));
});
});
}

let getChrome = REMOTE_CHROME_URL ? getRemoteChrome : launchChromeLocally;

Promise.all([runServer(), getChrome()]).then(([server, chrome]) => {
const chromeHost = chrome.host || 'localhost';
console.log(`Static file server is listening at ${server.url}.`);
console.log(`Chrome is debuggable on http://${chromeHost}:${chrome.port}.`);
console.log(`Running aXe on:`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Was there more text that was supposed to go here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nah, it's just the heading text that appears before each individual page that's visited. The final output looks like this:

2017-07-12_15-43-34


CDP({
host: chrome.host,
Copy link
Contributor

Choose a reason for hiding this comment

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

Above, it sounds like chrome.host may not exist. Should this be chromeHost? Why wouldn't chrome.host not exist anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, it will just be undefined if it doesn't exit, which CDP just then defaults to localhost--but I agree, I think it'd be less confusing / more explicit if we just passed chromeHost here.

I was hoping that the API for chrome would set host to whatever the ultimate "resolved" value was, whether that was localhost (in the case where Chrome is launched locally) or something explicit (in the case where we connect to a remote Chrome in e.g. a different Docker container). However, that wasn't the case--in the case where Chrome is launched locally, host isn't set at all, so I had to manually default it to localhost via that chromeHost variable. It's a bit annoying because the APIs that connect all these different Chrome packages isn't quite as consistent as I'd hoped, and this is a workaround for that.

port: chrome.port,
}, client => {
const {Page, Network, Runtime} = client;
const pagesLeft = PAGES.slice();
const loadNextPage = () => {
if (pagesLeft.length === 0) {
console.log(`Finished visiting ${PAGES.length} pages with no errors.`);
terminate(0);
} else {
const page = pagesLeft.pop();
const url = `${server.url}${page}`;
process.stdout.write(` ${page} `);
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this doing? Why isn't it a simple console.log?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I wanted it to look cool like all the fancy test runners!

2017-07-12_15-43-34

Page.navigate({url});
}
};
const terminate = exitCode => {
client.close().then(() => {
chrome.kill().then(() => {
// Note that we're not killing the server; this is because
// the remote chrome instance (if we're using one) may be
// keeping some network connections to the server alive, which
// makes it harder to kill, so it's easier to just terminate.
const color = exitCode === 0 ? chalk.green : chalk.red;
console.log(color(`Terminating with exit code ${exitCode}.`));
process.exit(exitCode);
});
});
};

process.on('unhandledRejection', (reason, p) => {
console.log('Unhandled Rejection at:', p, 'reason:', reason);
terminate(1);
});

Promise.all([
Page.enable(),
Network.enable(),
]).then(() => {
Network.responseReceived(({response}) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this when any page resource is received? Or just the main HTML document?

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 believe it's when any page resource is received. So this effectively overlaps a bit with our crawler from #321. It was easier to do it that way than it was to check the URL and see if it matched the page we were requesting, but I could add such a conditional if you think it'd be better that way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems OK to have this extra check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, so I thought about it a bit, and realized that maybe it's actually good that it's counting any sub-resource failure as a 404 too. For example, if it's 404'ing on CSS or JS, then that changes the accessibility profile of the page--calculated color contrast ratios may be incorrect, or certain ARIA attributes may not be present on the evaluated page.

Another thing is that the crawler from #321 isn't actually 100% accurate--the library we're using uses regular expressions to "scrape" resources for URLs, so it's actually possible that it might miss sub-resources that are noticed by actually loading a page in Chrome. So it might still be good to hard fail on sub-resource failures for the sake of redundancy, too.

What do you think? At the very least, I think we should log warnings on sub-resource fails.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea I agree that it's good to consider sub-resource 404s as failures.

if (response.status < 400) return;
console.log(`${response.url} returned HTTP ${response.status}!`);
terminate(1);
});
Network.loadingFailed(details => {
console.log("A network request failed to load.");
console.log(details);
terminate(1);
});
Page.loadEventFired(() => {
Runtime.evaluate({
expression: AXE_JS + ';(' + runAxe.toString() + ')()',
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this possible to write using a template string so it's easier to parse?

awaitPromise: true,
}).then(details => {
let errorFound = false;
if (details.result.type === 'string') {
const viols = JSON.parse(details.result.value);
if (viols.length === 0) {
console.log(chalk.green('OK'));
} else {
console.log(chalk.red(`Found ${viols.length} aXe violations.`));
console.log(viols);
console.log(chalk.cyan(
`\nTo debug these violations, install aXe at:\n\n` +
` https://www.deque.com/products/axe/\n`
));
errorFound = true;
}
} else {
console.log('Unexpected result from CDP!');
Copy link
Contributor

Choose a reason for hiding this comment

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

What's CDP?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, it stands for Chrome DevTools Protocol... I'll expand it to just say that!

console.log(details.result);
errorFound = true;
}
// Note that this means we're terminating on the first error we
// find, rather than reporting the error and moving on to the
// next page. We're doing this because it's possible that the
// error is caused by a layout or include, which may result
// in the same errors being reported across multiple pages, so
// we'd rather avoid the potential spam and just exit early.
if (errorFound) {
terminate(1);
} else {
loadNextPage();
}
});
});

loadNextPage();
});
});
});
30 changes: 30 additions & 0 deletions config/static-server.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
const fs = require('fs');
const os = require('os');
const path = require('path');
const express = require('express');

const app = express();

const SITE_PATH = path.normalize(`${__dirname}/../_site`);

if (fs.existsSync(SITE_PATH)) {
app.use(express.static(SITE_PATH));
} else {
console.log(`Please build the site before running me.`);
process.exit(1);
}

module.exports = () => {
return new Promise((resolve, reject) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need a rejection handler somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good CATCH, will add!

const server = app.listen(() => {
const hostname = os.hostname().toLowerCase();
const port = server.address().port;
resolve({
hostname,
port,
url: `http://${hostname}:${port}`,
httpServer: server,
});
});
});
};
6 changes: 5 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@
"postinstall": "bundle",
"prestart": "gulp build",
"start": "bundle exec jekyll serve --drafts --future",
"test": "npm run lint && npm run crawl && bundle exec rspec",
"test": "npm run axe && npm run lint && npm run crawl && bundle exec rspec",
"axe": "node config/run-axe.js",
"crawl": "node config/crawl.js",
"watch": "nswatch"
},
Expand Down Expand Up @@ -64,8 +65,11 @@
},
"homepage": "https://github.com/18F/web-design-standards-docs#readme",
"dependencies": {
"axe-core": "^2.3.1",
"browserify": "^13.0.0",
"chalk": "^1.1.3",
"chrome-launcher": "^0.3.1",
"chrome-remote-interface": "^0.24.1",
"del": "^2.2.0",
"express": "^4.15.3",
"gulp": "^3.9.0",
Expand Down