Improved error handling

[WilcoFiers]

Currently: When an error occurs inside a rule, axe will set an error and errorNode property. This is initially set to incomplete, but because nodes is empty. Because there are no nodes, finalizeResults sets this to inapplicable. The v1 reporter than restores that to incomplete by detecting the error property, but leaving nodes empty.

Exceptions:

• axe's async callback in evaluate accepts errors, but does not assign errorNode
• matches functions do not assign errorNode
• if an after function throws, the entire run throws

Additionally, if an error occurs inside a frame, the error is bubbled up and will throw the run.

Goal

1. Axe-core should be able to recover from errors as much as possible, so that rules that can complete won't be blocked by those that errored out.

2. Axe should be transparent about what parts of it failed to complete, so that users can understand what was not tested.

3. Recoverable errors should be reported in the same format as other axe results so they can be given to users without special design considerations

UX

In practice, what users should experience is this:

• If an error occurs in a rule, the user is given an incomplete result for that rule, with one node

• If axe fails within a frame (which is almost impossible), we'll report that as a frame-tested incomplete, just like we do if axe failed to inject into that frame.

• We wouldn't show any error messages or anything like that to users. The actual errors should be sent to datadog. We'll just show a friendly message that the page needs to be tested manually for that particular rule, just like how we do for other incompletes.

What to change

1. In addition to using an `error` and `errorNode` property, when an error occurs, the node list is replaced with an array with a single node, where the error occurs. A new `none` check is associated with this with the `error-occurred` id. The `data` object is made the serialized error, so that the `data` property will have `message`, `stack`, and `type`, along with optionally `cause`, which should also be serialized.

   Having a NodeResult for each error ensures that tools that store axe results by node can record display errors. By using axe's existing data format for errors we allow the error to be displayed. By serializing the error's properties we ensure it can be transferred, both across frames, as well as between tools. For the `error` and `errorNode` properties should continue to be set.

2. If we do nothing, the above change will result in error nodes getting passed to after methods. To ensure backward compatibility we'll need to remove error nodes before padding results to after, and add them back in when after is done.

3. When an after method throws, axe-core should catch the error and report that rule as incomplete, instead of failing completely as it does today. To avoid an overly lengthy number of incompletes, only the first node should be kept, the rest should be set as related nodes, up to a maximum of 10 nodes.

4. When a matches function throws, the node that was being matched should be recorded, Currently only the error is.

5. When an Error is passed to an async evaluate's callback, the node that was being evaluated should be recorded. Currently only the error is.

6. When axe inside a frame errors out, currently the error bubbles up and throws out the entire run. While errors inside rules are caught, an error for example in a node serializer would not be. Instead of failing the entire run, we should treat that frame as though it wasn't tested. The frame should be reported as an error node under the frame-tested rule, with the error associated with the data property.

7. The heading-order rule should be updated to understand that if we don't have results from a particular frame, the heading immediately following that frame should be passed. The frame-tested result itself should be sufficient to inform users information was missing.

8. The error property should be documented and typed. An error property on an incomplete result will be the "standard" way axe will expose major failures. Additionally, the frame-tested rule should set a timeout error. All places that set errors should set an errorNode as well.

Future

A nice future addition to this might be that if some a rule errors on some nodes but not others that axe would still show the results it did find. I don't think we'd want to have incompletes for every error. One error can cause another for a future rule, with cache problems and stuff like that. But where we can, showing what we do have would be a good addition.

I'm not sure how common a problem this is. Once we have some of this ticket implemented we'll have logging to be able to see whether this might be worth the effort. In doing this we're going to need to be careful with after methods. Those currently assume they have all relevant content - which they won't if we make this change.

[dbjorge]

I think most of this looks reasonable as an incremental improvement. Some feedback:

What users should experience is this: If an error occurs in a rule, the user is given an incomplete result for that rule, with one node

I think doing this reliably would be a good incremental improvement, but I don't actually think this is a great user experience; I think "axe-core was prevented from scanning a part of the page" is actually something that I as a user would usually want to treat more seriously than a typical "incomplete" result. For example, back in Accessibility Insights, we specifically special cased the frame-tested rule because we wanted issues there to be much more visible than other types of issues results. I think it'd be good for us to provide some sort of consistent mechanism for a user to identify "there was an error that prevented parts of the scan from running" in a way that bubbles up appropriately from all ways that can happen, without needing to separately check "did a frame-tested issue come up? also, did an error-occured check exist in any result? also, did the top-level error property get set?".

You mentioned in section 1 "For backward compatibility, the error and errorNode properties should continue to be set." - I think an option for improving this might be "that, and also we should also start setting them for frame-tested issues, and make sure they get set in the new error detection paths this proposal mentions in 3-6"

1. Instead of using an error and errorNode property, when an error occurs, the node list is replaced with an array with a single node, where the error occurs. A new none check is associated with this with the error-occurred id. The data object is made the serialized error, so that the data property will have message, stack, and type, along with optionally cause, which should also be serialized.

This is more of a nit I could live without for a v1, but: This pattern makes sense to me for cases where an error occurs during rule-level (but not check-level) processing, eg the error reporting for matches this proposes adding. But today the only place we ever write errorNode at all is when an individual check fails. If we know that a failure is occurring inside of a specific check, might it be preferable to instead produce an incomplete result (with serialized error info) for the erroring check, instead of some new synthetic check, to make it more clear what the actual immediate source of the issue was?

I agree with some of the concerns you laid out in the first paragraph of the "Future" section that might motivate stopping a rule early if an error occurs in one node of it, but I think we could treat "stop processing further nodes" separately from "how do we present the error".

[WilcoFiers]

@dbjorge Updated the ticket to keep error as a way of distinguishing these types of results from other incompletes. I'm not sure these actually are more important to users than other incompletes, but they are things tools should probably log.

As for why I'm introducing a new error-occurred check instead of reporting the error on the check that threw. I figured that would be easier to do in a type-safe system. The error-occurred check has its data properties for when something goes wrong, and other checks continue to have their own properties for when things work as expected. We can always include which check threw in the error-occured data properties. We should do that with after too, not just with evaluates.