-
Notifications
You must be signed in to change notification settings - Fork 840
feat: incomplete with node on which an error occurred #4863
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
base: develop
Are you sure you want to change the base?
Changes from all commits
afd906f
274ebf2
bd65841
fe61589
4665381
9014c5e
75c027b
34e827b
347fb89
b06ffb1
ec3a93c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
{ | ||
"id": "error-occurred", | ||
"evaluate": "exists-evaluate", | ||
"metadata": { | ||
"messages": { | ||
"pass": "", | ||
"incomplete": "Axe encountered an error; test the page for this type of problem manually" | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,8 @@ import { | |
preload, | ||
findBy, | ||
ruleShouldRun, | ||
performanceTimer | ||
performanceTimer, | ||
serializeError | ||
} from '../utils'; | ||
import doT from '@deque/dot'; | ||
import constants from '../constants'; | ||
|
@@ -368,14 +369,24 @@ export default class Audit { | |
after(results, options) { | ||
const rules = this.rules; | ||
return results.map(ruleResult => { | ||
if (ruleResult.error) { | ||
return ruleResult; | ||
} | ||
const rule = findBy(rules, 'id', ruleResult.id); | ||
if (!rule) { | ||
// If you see this, you're probably running the Mocha tests with the axe extension installed | ||
throw new Error( | ||
'Result for unknown rule. You may be running mismatch axe-core versions' | ||
); | ||
} | ||
return rule.after(ruleResult, options); | ||
try { | ||
return rule.after(ruleResult, options); | ||
} catch (err) { | ||
if (options.debug) { | ||
throw err; | ||
} | ||
return createIncompleteErrorResult(rule, err); | ||
} | ||
}); | ||
} | ||
/** | ||
|
@@ -732,36 +743,37 @@ function getDefferedRule(rule, context, options) { | |
rule.run( | ||
context, | ||
options, | ||
// resolve callback for rule `run` | ||
ruleResult => { | ||
// resolve | ||
resolve(ruleResult); | ||
}, | ||
// reject callback for rule `run` | ||
ruleResult => resolve(ruleResult), | ||
err => { | ||
// if debug - construct error details | ||
if (!options.debug) { | ||
const errResult = Object.assign(new RuleResult(rule), { | ||
result: constants.CANTTELL, | ||
description: 'An error occured while running this rule', | ||
message: err.message, | ||
stack: err.stack, | ||
error: err, | ||
// Add a serialized reference to the node the rule failed on for easier debugging. | ||
// See https://github.com/dequelabs/axe-core/issues/1317. | ||
errorNode: err.errorNode | ||
Comment on lines
-746
to
-752
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that I got rid of most of these, and updated how
ErrorNode in particular was a problem. Because it wasn't serialized, it A. doesn't have the frame selector, and B. leaves the |
||
}); | ||
// resolve | ||
resolve(errResult); | ||
} else { | ||
// reject | ||
if (options.debug) { | ||
reject(err); | ||
} else { | ||
resolve(createIncompleteErrorResult(rule, err)); | ||
} | ||
} | ||
); | ||
}; | ||
} | ||
|
||
function createIncompleteErrorResult(rule, error) { | ||
const { errorNode } = error; | ||
const serialError = serializeError(error); | ||
const none = [ | ||
{ | ||
id: 'error-occurred', | ||
result: undefined, | ||
data: serialError, | ||
relatedNodes: [] | ||
} | ||
]; | ||
const node = errorNode || new DqElement(document.documentElement); | ||
return Object.assign(new RuleResult(rule), { | ||
error: serialError, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue suggests that this should also include There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that's why I chose not to do that. It's a pain to properly serialize, with little benefit. See PR description. |
||
result: constants.CANTTELL, | ||
nodes: [{ any: [], all: [], none, node }] | ||
}); | ||
} | ||
|
||
/** | ||
* For all the rules, create the helpUrl and add it to the data for that rule | ||
*/ | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,4 +1,3 @@ | ||||||
/*global SupportError */ | ||||||
import { createExecutionContext } from './check'; | ||||||
import RuleResult from './rule-result'; | ||||||
import { | ||||||
|
@@ -8,7 +7,8 @@ import { | |||||
queue, | ||||||
DqElement, | ||||||
select, | ||||||
assert | ||||||
assert, | ||||||
SupportError | ||||||
} from '../utils'; | ||||||
import { isVisibleToScreenReaders } from '../../commons/dom'; | ||||||
import constants from '../constants'; | ||||||
|
@@ -181,7 +181,16 @@ Rule.prototype.runChecks = function runChecks( | |||||
const check = self._audit.checks[c.id || c]; | ||||||
const option = getCheckOption(check, self.id, options); | ||||||
checkQueue.defer((res, rej) => { | ||||||
check.run(node, option, context, res, rej); | ||||||
check.run(node, option, context, res, error => { | ||||||
rej( | ||||||
new SupportError({ | ||||||
ruleId: self.id, | ||||||
method: `${check.id}#evaluate`, | ||||||
errorNode: new DqElement(node), | ||||||
error | ||||||
}) | ||||||
); | ||||||
}); | ||||||
}); | ||||||
}); | ||||||
|
||||||
|
@@ -235,8 +244,7 @@ Rule.prototype.run = function run(context, options = {}, resolve, reject) { | |||||
// Matches throws an error when it lacks support for document methods | ||||||
nodes = this.gatherAndMatchNodes(context, options); | ||||||
} catch (error) { | ||||||
// Exit the rule execution if matches fails | ||||||
reject(new SupportError({ cause: error, ruleId: this.id })); | ||||||
reject(error); | ||||||
return; | ||||||
} | ||||||
|
||||||
|
@@ -312,15 +320,7 @@ Rule.prototype.runSync = function runSync(context, options = {}) { | |||||
} | ||||||
|
||||||
const ruleResult = new RuleResult(this); | ||||||
let nodes; | ||||||
|
||||||
try { | ||||||
nodes = this.gatherAndMatchNodes(context, options); | ||||||
} catch (error) { | ||||||
// Exit the rule execution if matches fails | ||||||
throw new SupportError({ cause: error, ruleId: this.id }); | ||||||
} | ||||||
|
||||||
const nodes = this.gatherAndMatchNodes(context, options); | ||||||
if (options.performanceTimer) { | ||||||
this._logGatherPerformance(nodes); | ||||||
} | ||||||
|
@@ -451,7 +451,18 @@ Rule.prototype.gatherAndMatchNodes = function gatherAndMatchNodes( | |||||
performanceTimer.mark(markMatchesStart); | ||||||
} | ||||||
|
||||||
nodes = nodes.filter(node => this.matches(node.actualNode, node, context)); | ||||||
nodes = nodes.filter(node => { | ||||||
try { | ||||||
return this.matches(node.actualNode, node, context); | ||||||
} catch (error) { | ||||||
throw new SupportError({ | ||||||
ruleId: this.id, | ||||||
method: `#matches`, | ||||||
errorNode: new DqElement(node.actualNode), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: new DqElement can be passed a virtualNode directly
Suggested change
|
||||||
error | ||||||
}); | ||||||
} | ||||||
}); | ||||||
|
||||||
if (options.performanceTimer) { | ||||||
performanceTimer.mark(markMatchesEnd); | ||||||
|
@@ -542,12 +553,20 @@ function sanitizeNodes(result) { | |||||
*/ | ||||||
Rule.prototype.after = function after(result, options) { | ||||||
const afterChecks = findAfterChecks(this); | ||||||
const ruleID = this.id; | ||||||
afterChecks.forEach(check => { | ||||||
const beforeResults = findCheckResults(result.nodes, check.id); | ||||||
const checkOption = getCheckOption(check, ruleID, options); | ||||||
|
||||||
const afterResults = check.after(beforeResults, checkOption.options); | ||||||
const checkOption = getCheckOption(check, this.id, options); | ||||||
let afterResults; | ||||||
try { | ||||||
afterResults = check.after(beforeResults, checkOption.options); | ||||||
} catch (error) { | ||||||
throw new SupportError({ | ||||||
ruleId: this.id, | ||||||
method: `${check.id}#after`, | ||||||
errorNode: result.nodes?.[0].node, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There should be a test that would have caught this:
Suggested change
|
||||||
error | ||||||
}); | ||||||
} | ||||||
|
||||||
if (this.reviewOnFail) { | ||||||
afterResults.forEach(checkResult => { | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -95,6 +95,10 @@ function mergeResults(frameResults, options) { | |
if (ruleResult.nodes.length) { | ||
spliceNodes(res.nodes, ruleResult.nodes); | ||
} | ||
if (ruleResult.error) { | ||
// TODO: Test me | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like this TODO was done, remove the comment? |
||
res.error ??= ruleResult.error; | ||
} | ||
} | ||
}); | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
/** | ||
* Serializes an error to a JSON object | ||
* @param e - The error to serialize | ||
* @returns A JSON object representing the error | ||
*/ | ||
export default function serializeError(err, iteration = 0) { | ||
if (typeof err !== 'object' || err === null) { | ||
return { message: String(err) }; | ||
} | ||
const serial = { ...err }; // Copy all "own" properties | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This it('should not include unserializable errorNode properties', () => {
const baseError = new Error('test');
const errorNode = new axe.utils.DqElement(document.documentElement);
const supportError = new axe.utils.SupportError({ error: baseError, errorNode })
const serialized = serializeError(supportError);
assert.isUndefined(serialized.errorNode?._element);
}) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (presumably by only passing through properties known to be serializable and specifically handling |
||
// Copy error.message / name / stack, these don't serialize otherwise | ||
for (const prop of ['message', 'stack', 'name']) { | ||
if (typeof err[prop] === 'string') { | ||
serial[prop] = err[prop]; | ||
} | ||
} | ||
// Recursively serialize cause up to 10 levels deep | ||
if (err.cause) { | ||
serial.cause = | ||
iteration < 10 ? serializeError(err.cause, iteration + 1) : '...'; | ||
} | ||
return serial; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
import serializeError from './serialize-error'; | ||
|
||
export default class SupportError extends Error { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The name "SupportError" feels off to me; it implies to me "this indicates that axe-core doesn't support something you/the page tried to do", but really we're using this to generically mean a "an error that represents any unexpected failure while processing a rule". Perhaps |
||
constructor({ error, ruleId, method, errorNode }) { | ||
super(); | ||
this.name = error.name ?? 'SupportError'; | ||
this.message = error.message; | ||
this.stack = error.stack; | ||
if (error.cause) { | ||
this.cause = serializeError(error.cause); | ||
} | ||
if (ruleId) { | ||
this.ruleId = ruleId; | ||
this.message += ` Skipping ${this.ruleId} rule.`; | ||
} | ||
if (method) { | ||
this.method = method; | ||
} | ||
if (errorNode) { | ||
this.errorNode = errorNode; | ||
} | ||
} | ||
} |
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.