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

A11y add axe-puppeteer tests #302

Merged
merged 5 commits into from
Nov 21, 2019

Conversation

microbit-mark
Copy link
Contributor

@@ -15,8 +15,12 @@
"test:coverage": "npm t -- --coverage",
"test:ci": "npm t -- --ci --colors"
},
"dependencies": {},
"dependencies": {
"@babel/runtime": "^7.7.2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this needed? Wouldn't it be a dependency of axe-puppeteer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without it, when I run npm test I get Cannot find module '@babel/runtime/helpers/interopRequireDefault' from 'index.js'

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it tell you which index.js? Sounds like a bug on their end, if they need that dependency it should be included in their pacakge.json.
Could you check the issue trackers for the two new dependencies to see if it has been reported already?
In the meantime, we should move this to dev dependencies, hopefully we can remove it when it get's fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't find an issue for it, but it's from this package https://github.com/WordPress/gutenberg/tree/master/packages/jest-puppeteer-axe which is checking against index.js for @babel/runtime/helpers/interopRequireDefault in the node module when it's built

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see any babel imports in https://github.com/WordPress/gutenberg/blob/master/packages/jest-puppeteer-axe/src/index.js
How did you find out this is the one causing the issue? Is it one of its dependencies?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, so that is only in the packaged code shipped by npm.
Could you open an issue in the repo? Seeing it is a very small project inside a very large project (Gutenberg), it might not be a priority, but we should report it anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this might be the broader issue WordPress/gutenberg#14373 What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, that is definitely related to this issue, but it was raised in March and it hasn't had any activity since then. Would you mind creating a PR like this one on that repo? Maybe that will get merged quickly.
WordPress/gutenberg#14374

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have done so here WordPress/gutenberg#18626

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks Mark!

tests/spec/puppeteer-axe-spec.js Outdated Show resolved Hide resolved
tests/spec/puppeteer-axe-spec.js Outdated Show resolved Hide resolved
tests/spec/puppeteer-axe-spec.js Outdated Show resolved Hide resolved
tests/spec/puppeteer-axe-spec.js Outdated Show resolved Hide resolved
tests/spec/puppeteer-axe-spec.js Outdated Show resolved Hide resolved
tests/spec/puppeteer-axe-spec.js Outdated Show resolved Hide resolved
@microbit-carlos
Copy link
Collaborator

We should also create a new test case for checking the help.html page.

tests/package.json Outdated Show resolved Hide resolved
tests/spec/puppeteer-axe-spec.js Show resolved Hide resolved
Co-Authored-By: Carlos Pereira Atencio <carlos@microbit.org>
Comment on lines +31 to +32
// disable tab-index as we have values greater than 0
// and h1 as we aren't using this heading
Copy link
Collaborator

Choose a reason for hiding this comment

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

Glad to see a comments here and the other tests cases explaining why these rules are ignored, good call 👍

@microbit-carlos
Copy link
Collaborator

PR looks good, good work Mark!
I assume for the tests to pass we need to wait for the other a11y tickets to be implemented and then we'll rebase or merge into this branch?

@microbit-mark
Copy link
Contributor Author

Yes, tests pass if the other tickets are merged into this branch

Copy link
Collaborator

@microbit-carlos microbit-carlos left a comment

Choose a reason for hiding this comment

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

All green now after merging master, thanks Mark!

@microbit-carlos microbit-carlos merged commit b513ef6 into bbcmicrobit:master Nov 21, 2019
microbit-mark added a commit to microbit-mark/PythonEditor that referenced this pull request Nov 25, 2019
microbit-mark added a commit to microbit-mark/PythonEditor that referenced this pull request Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants