Bug: Reporter error handling

[aarongoldenthal]

There is no specific handling for errors thrown by a reporter. This can lead to varying and undesirable results depending on the reporter event.

• If an error is thrown in reporter.beforeAll or reporter.afterAll, it's not caught and pa11y-ci fails
• If an error is thrown in reporter.begin, the async/queue task fails for that URL, results/error are not called, and the results are not saved in the report passed to afterAll. Execution continues with the remaining URLs.
  • Results are similar if an error is thrown in reporter.error - results not saved, continue execution.
• If an error thrown in reporter.results, it's within the same try block as pa11y execution, so it's caught and the catch block runs, which then calls error for all reporters (with the reporter error).

Propose updating cycleReporters with error handling to cover all events and return false on reporter failure as it does when there is no reporter function (although it could log something to stderr as well). If pa11y-ci had more exhaustive debug logging as an option like pa11y, this seems like a good candidate.

In addition, the Promise.all() in cycleReporters could be replaced with Promise.allSettled() since Promise.all() will reject on the first rejection, versus allowing all to execute with Promise.allSettled(). This is supported as of Node 12.9.0, which would require a breaking engine update.

[joeyciechanowicz]

Propose updating cycleReporters with error handling to cover all events and return false on reporter failure

Good idea. Definitely want to log to stderr when a reporter method fails IMO.

Regarding updating the minimum engine to 12.9, I think that would be fine. We support all the Node LTS releases, however when Node states that 12 is an LTS, that doesn't mean 12.0 will always be working, rather the latest 12.x release will be kept in a fixed state.

[aarongoldenthal]

@joeyciechanowicz I agree on the Node 12 philosophy, more a note that currently "engines": { "node": ">=12" }, so I assume we'd want to update that to be clear on the dependency.