Skip to content
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

core(audits): add handling of 'incomplete' results from axe-core #10072

Merged
merged 68 commits into from
Jan 27, 2020
Merged

core(audits): add handling of 'incomplete' results from axe-core #10072

merged 68 commits into from
Jan 27, 2020

Conversation

jayaddison
Copy link
Contributor

@jayaddison jayaddison commented Dec 6, 2019

Summary
As of [email protected], the form-field-multiple-labels audit will no longer indicate a strict failure, but instead returns an incomplete result.

As per the axe-core docs, an incomplete result indicates either an error executing a rule or an accessibility result which requires manual investigation.

This changeset handles the former error cases by transforming the incomplete result into a lighthouse audit error result.

Non-error incomplete results are considered failure cases.

Related Issues/PRs
Additional background and context is available in #10056, particularly this comment.

@jayaddison jayaddison closed this Dec 6, 2019
@jayaddison jayaddison reopened this Dec 6, 2019
@jayaddison
Copy link
Contributor Author

Resolves #10056 (comment) (cc @patrickhulce)

Unit tests pass locally, but Travis seems unreliable for PRs at the moment. I've opened an experimental improvement in #10075.

@jayaddison
Copy link
Contributor Author

@patrickhulce Any chance you could reset the Travis build cache for this PR? Doesn't look like I have permissions to, and I think that's all that's left to get this passing (I was quietly hoping that merging master might invalidate the cache, but no dice so far)

@patrickhulce
Copy link
Collaborator

Just kicked a new one off without the cache, we'll see if it does the trick!

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % types

awesome job @jayaddison!

@patrickhulce
Copy link
Collaborator

@connorjclark any more thoughts?

@jayaddison
Copy link
Contributor Author

@patrickhulce @connorjclark One thing I was wondering about - the audit explanation field isn't present for the incomplete results here, which is reflected in the updated a11y test expectations (ref).

I've opened an upstream ticket in axe-core about it (linked in this PR's history) to see if that can be addressed, but was wondering if it's likely to cause any issues/problems in the meantime?

@patrickhulce
Copy link
Collaborator

if it's likely to cause any issues/problems in the meantime?

It will be a slight regression in the available data, but I don't expect it to be too large of a problem as long as we get this in to 6.0. There's also not much we can do about it other than not upgrade or wait for axe to fix it, so it shouldn't be a blocker IMO.

@jayaddison
Copy link
Contributor Author

Small update: failureSummary should be available for incomplete results from axe-core>=3.5 (ref).

@patrickhulce
Copy link
Collaborator

@paulirish does this require further discussion or does the current state of a review requested from @connorjclark work?

@connorjclark if you'd prefer deferring to my review that's fine too, just wanted to give you the opportunity since you reviewed earlier.

@patrickhulce patrickhulce merged commit c5be90d into GoogleChrome:master Jan 27, 2020
@jayaddison
Copy link
Contributor Author

Thanks a ton @patrickhulce @connorjclark !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants