-
Notifications
You must be signed in to change notification settings - Fork 63
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
Add support for reporters #129
Conversation
After some inputs from my colleagues, I updated the PR and the description to reflect the changes (c5a67c5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fantastic! Great idea to add IMO.
A slight change to how promises are returned and this is 💯
Co-authored-by: Joey Ciechanowicz <1148637+joeyciechanowicz@users.noreply.github.com>
This is a great proposal, so after taking a look at the details I've been thinking about how I'd adapt
@joeyciechanowicz I thought I had read somewhere here or on Slack that the master plan was to integrate the reporters into pa11y, and integrate the pa11y-ci functionality into pa11y, so just wondering what that means for this PR (and maybe others)? |
@aarongoldenthal thanks for your feedback!
My initial idea was to re-use pa11y reporters interface as much as possible for consistency and to introduce fewer changes, but I understand your point. To preserve the current behavior we can have a default reporter which outputs to the CLI in the same way pa11y-ci does now (if I am correct is what you already proposed in #121). The user can then change the reporters to say, JSON to just output to a file or JSON + HTML + CLI to output both JSON and HTML as file while still have the CLI output. I believe this is how it works in mocha (or maybe jest).
I added the reporter map so that it can be used like a sort of accumulator that is unique for every reporter instance. Since reporters are basically static modules, and supposing that in the future we introduce reporters options, this looked as a good solution to avoid sharing the same accumulator in case the same reporter is used multiple times. Another solution, if we remove the current behavior (ie: if reporters return something log it to the console), might be to use beforeAll as a factory function returning the accumulator. In this way each reporter can set its initial state independently: function beforeAll() {
// do something...
// this is then passed to results, afterAll, ...
return new Map()
}
In some of my experiments I used those URLs as unique key on a
You're right. It makes sense. |
I had some time to throw together a quick example to test and had a few additional comments:
In looking at this I put together a quick example using Configuration file: {
"defaults": {
"reporters": [
"reporters/reporter-test.js"
],
"reporterConfig": {
"reporter-test": {
"savePageReport": true,
"saveSummaryReport": true
}
}
},
"urls": [
"https://weather.com/",
"https://domain.does.not.exist/"
]
}
'use strict';
const filenamifyUrl = require('filenamify-url');
const htmlReporter = require('pa11y-reporter-html');
const fs = require('fs');
const path = require('path');
const report = module.exports = {};
report.results = async (results, _, {config}) => {
// If configuration defined, save report based on setting (default: true)
if (config.reporterConfig &&
config.reporterConfig['reporter-test'] &&
!config.reporterConfig['reporter-test'].savePageReport) {
return;
}
const fileName = path.join('reports', `${filenamifyUrl(results.pageUrl)}.html`);
const htmlReport = await htmlReporter.results(results);
fs.writeFileSync(fileName, htmlReport);
};
report.error = (error, _, {url}) => {
// Process error, but probably not like this, instead log error to summary results
console.error(`Error processing '${url}': ${error.message}`);
}; |
@aarongoldenthal thanks for your feedback. Sorry I wasn't able to reply earlier. I agree with your suggestions and will update the PR in the next few days. I have one question about a topic you shared earlier:
A possible scenario could be that one reporter is used multiple times with different options. Since reporters are module functions how could we manage different instances of the reporter object? Maybe we could add a |
@dwightjack That's a good question, I hadn't considered the case where one reporter was used multiple times. I assume that would only be in the case where there were specific reporter options specific to each instance. My initial thought is that the limitation that we hit is that I ran a quick test with the changes below against a local file and that appeared to work, although I'll admit it probably deservers a little more thought. module.exports = function resolveReporters(reporters) {
return [].concat(reporters).map(reporter => {
if (typeof reporter !== 'string') {
return undefined;
}
try {
delete require.cache[require.resolve(reporter)];
return require(reporter);
} catch (_) {
const localModule = path.isAbsolute(reporter) ?
reporter : path.resolve(process.cwd(), reporter);
if (fs.existsSync(localModule)) {
delete require.cache[require.resolve(localModule)];
return require(localModule);
}
console.error(`Unable to load reporter "${reporter}"`);
return undefined;
}
}).filter(Boolean).map(reporterModule => {
return buildReporter(reporterModule);
});
}; |
@aarongoldenthal I didn't think about that solution. We could use import-fresh for the task, which should be a little bit more optimized. |
Co-authored-by: Aaron Goldenthal <aarongoldenthal@users.noreply.github.com>
Co-authored-by: Aaron Goldenthal <aarongoldenthal@users.noreply.github.com>
A reporter option would be awseome! |
@aarongoldenthal @joeyciechanowicz Sorry for the delay in updating this PR, I've pushed the changes we discussed earlier (hope I didn't miss any point):
For the scenario with the same reporter used multiple times (and with custom options), I thought that maybe exporting as default export a factory function would have been the more intuitive option (instead of using module.exports = (options) => {
const filename = options.filename
return {
afterAll(report) {
fs.writeFile(filename, ...);
}
}
} we could configure it to write two files: // configuration file
{
"defaults": {
"reporters": [
"pa11y-ci/libs/reporters/cli", // preserve the CLI output
["file-reporter", { "fileName": "./my-report.json" }],
["file-reporter", { "fileName": "./my-other-report.json" }]
]
},
"urls": [
...
]
} The array syntax is the same used by Jest, so it should be enough familiar to developers. |
@aarongoldenthal Apologies for the slow reply. Yes, the aim is to merge the reporters into
That gets my vote! Regarding the issue of console output, I think that |
@dwightjack This is great. After using it for a week and throwing together a couple of example reporters it's really straightforward, and I agree the reporter configuration option is very familiar and easy to work with. I did have some other observations (which really are intended not to be critical but to show how useful this is). I hoped to have some code for some of these, but got sidetracked working #132 for GitHub Actions. JSON With the reporter interface it seems like the JSON option could/should be converted to a reporter. At the moment you can get yourself into some messy output with reporters and The other questions with that is what to do with the Built-In Modules Should there be an abbreviated name syntax for the included reporters (e.g. Tests I didn't see an issue running multiple reporters (and at one point I was running 3, two of which were instances of the same reporter with different configuration), but I didn't see a test to check that all reporters are called if multiple are specified, which seemed valuable. Reporter Interface The Default Configuration In digging through a lot of the |
I add this comment for reference. @aarongoldenthal implemented most of the ideas from the previous comment ( #129 (comment)) in dwightjack#1. To reply to some of the remaining points:
I'm going to implement those tests in the next few days. I am not sure if they would fit better as integration of unit tests. I will investigate that.
Adding a new property is not a problem. I am wondering if it is really useful since we can use
Agree. Navigating the defaults and how they are merged and used got me confused at first. I will review that now that I am a bit more confident with the codebase. |
@aarongoldenthal I added both unit and integration tests for reporters. (1d7010b 460c008) I also updated the In the end, I didn't refactor the configuration resolution. Maybe it's not so readable but I cannot see a better pattern to make it work with the two usage scenarios (CLI and programmatic) of the library 🤔 . |
@dwightjack I made a couple of comments, but overall it looks really good to me. I looked again at the configuration and agree it seems like the best way to handle both scenarios (although it did lead to me comment in the README). I'm also a supporter of deprecating old Node versions, so I wouldn't be worried about Node 8 support (and assuming The only question that still lingers in my mind is potentially dropping the |
Co-authored-by: Aaron Goldenthal <aarongoldenthal@users.noreply.github.com>
Co-authored-by: Aaron Goldenthal <aarongoldenthal@users.noreply.github.com>
@aarongoldenthal thanks! I've merged your suggestions. About the
|
If it is not too much hassle it would be nice to deprecate the flag at first and remove it only in a major version number change (ie 3.0) as it is a breaking change. |
@dwightjack It looks good to me. And I think it makes sense to look at deprecating |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great addition and brings it line with the reporter merging that's recently been completed on the pa11y
repo.
Really lovely documentation as well ❤️
@joeyciechanowicz thanks to you and @aarongoldenthal for the help and great feedback. |
This has now been released as part of v3.0.0. Thanks everyone for all the work in this PR! |
This PR adds support for Pa11y compatible reporters into Pa11y CI.
Why
Pa11y CI reports omit the violations for URLs below the error threshold.
In my project, I want to log those violations as well. Instead of modifying the core behavior, I think it's a good idea to add reporters support so that users can use their own strategies to transform, store and analyze the results.
Reporters use the same interface as pa11y reporters with some additions:
report
argument. This object is an instance of a JavaScript Map that can be used to initialize and collect data across each tested URL.beforeAll(urls, report)
andafterAll(urls, report)
optional methods called respectively at the beginning and at the very end of the process with the following arguments:urls
: the URLs array defined in your configreport
: the report objectresults()
method receives a thirdoption
argument with the following properties:config
: the current URL configuration objecturl
: the current URL under testurls
: the URLs array defined in your configChanges
--reporter
CLI option to define a single reporterdefaults.reporters
configuration array to define multiple reporterspa11y-ci ... > my-file.json
)