feat: incomplete with node on which an error occurred

[WilcoFiers]

Part of #4860

If a check or matches method throws, catch the error and add a node with an error-occurred none check. This way incomplete results are always reported.

This only happens for axe.run / axe.runPartial. rule.runSync() still throws if an occurs in a rule or check.

[WilcoFiers]

Note that I got rid of most of these, and updated how error works:

• error is now serialized so that it can be carried across frame boundaries
• description is clobbered by the reporter. It never even left axe (plus it has a typo)
• message and stack are duplicated on error
• errorNode wasn't getting pulled through nodeSerializer

ErrorNode in particular was a problem. Because it wasn't serialized, it A. doesn't have the frame selector, and B. leaves the _element property on it, which doesn't play well with serialization (axe Extension would completely error out trying to serialize this).

[Copilot]

Pull Request Overview

This PR implements error handling for axe-core checks and rules by catching errors and converting them to incomplete results instead of failing completely. This allows the scanner to report what it could successfully check while noting where errors occurred.

• Introduces a new SupportError class to wrap errors with additional context
• Adds error handling to rule matching and check evaluation phases
• Creates a dedicated "error-occurred" check type for incomplete results

Reviewed Changes

Copilot reviewed 22 out of 23 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
lib/core/utils/support-error.js	New SupportError class for wrapping errors with context
lib/core/utils/serialize-error.js	Utility for serializing errors to JSON
lib/core/base/rule.js	Updated to catch errors and wrap them in SupportError
lib/core/base/audit.js	Added error handling in audit run and after phases
lib/core/utils/merge-results.js	Support for merging error information from frames
lib/checks/shared/error-occurred.json	New check definition for error cases
test/integration/full/error-occurred/*	Integration tests for error handling
test/core/utils/*	Unit tests for new utilities

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[dbjorge]

This ...err is going to mean that any (potentially non-serializable) properties of the original error will stay as-is in the returned object, no? Doesn't this defeat the purpose? I'd have expected that whole point of this function would be to make sure the following test case passes:

  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);
  })

[dbjorge]

(presumably by only passing through properties known to be serializable and specifically handling errorNode by passing it through nodeSerializer.dqElmToSpec)

[WilcoFiers]

Yeah it's not perfect. I'm not sure there's an easy way to ensure this. Considering this is internal, this seemed sufficient to me.

[dbjorge]

if you leave an unserializable property on the "serialized" error, doesn't that defeat the purpose of the PR and still leave you in the same place as you were originally, where you'll get errors when trying to pass an axe result containing the partially-unserializable error across a serialization boundary?

If we're satisfied with not having errorNode and instead relying on the node in the synthetic check result, I think the "easy way to ensure this" is to replace { ...err } with {} here and add the non-errorNode properties of RuleError to the list of props on L12. (maybe also add code to the list, it's common enough in things that could be error causes)

[dbjorge]

The issue suggests that this should also include errorNode here (for compatibility with existing error handling behavior). Note that if we do this, we'd need to pass it through nodeSerializer at frame boundaries.

[WilcoFiers]

Yeah, that's why I chose not to do that. It's a pain to properly serialize, with little benefit. See PR description.

[dbjorge]

I don't think "it's a pain to do" is a good enough reason to break backcompat with existing users' error handlers. The specification in the #4860 (in "What to change" 1) mentioned "In addition to using an error and errorNode property" and "the error and errorNode properties should continue to be set." I think it'd be better to keep it for backcompat.

I'm not sure what you're referring to by "See PR description", I don't see anything relevant there - did you mean this comment? That comment mentions that it's a problem that errorNode isn't going through nodeSerializer like it should, which I agree with, but I don't think it justifies why breaking the existing property is a preferable solution to "update it to go through nodeSerializer". There aren't that many places where we need to do nodeSerializer.mergeSpecs at frame boundaries, I'd expect it to be a reasonably contained change to update those places to make sure they also mergeSpecs on error nodes.

Looking at the current code (before this PR) that creates errorNodes, I'm actually a bit surprised/confused that we see any current scenarios where they contain _element properties that confuse serialization; the only places that appear to do an initial set of errorNode are in check.js where they come from a nodeSerializer.toSpec output. It makes sense that the original values would be missing ancestor frame information, but it surprises me that they'd end up with _element properties.

[WilcoFiers]

I don't agree it's a breaking change:

1. It's already an optional prop: not every method sets it, so not having it can't possibly break things.
2. It's an undocumented property, changing those isn't a breaking change.
3. If this was breaking, than so would removing message and stack be.

errorNode is also much buggier than I realized it was when I wrote the issue. It doesn't work across frames, and it doesn't run through NodeSerializer. Everything errorNode does can now be done in a better way. errorNode is a "feature" that has never worked, and that we keep making mistakes with. Better to drop it IMO.

[dbjorge]

I think 1 is a bit of a sketchy argument - I think if someone had a UI for errors that attempted to display which node an error occurred at, and that UI worked fine for errors in checks in <4.11 but stopped working for errors in checks in >=4.11, I think they could reasonably argue that the change broke them.

But points 2, 3, and that it's already really buggy anyway are all fair enough. I verified that I don't think we have any products with such an error UI already (we do have one product that parses errorNode if it's present, but it doesn't do anything with it). I can live with dropping it since it sounds like you feel strongly about it, it's not a hill I'm going to fight further on.

Done

[WilcoFiers]

Approve Dan's changes

[dbjorge]

Approving + security reviewing everything except my commit (which Wilco approved above)