-
Notifications
You must be signed in to change notification settings - Fork 22
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
feat(jest): add jest accessibility matcher #9
Conversation
as --fix option wasn't being passed to the eslint cmd
modify root jest dep version to match
and add links to existing docs in root readme
add type def, tests
to prevent failed commit process due to failed checks in staged files
Coverage after merging feat/add_jest_integration into master
Coverage Report
|
Fixes Error when using "await" with "toBeAccessible": "Unexpected await of a non-Promise (non-"Thenable") value"
that supports typescript among other things
to enable markdown eslint plugin to check them
Coverage after merging feat/add_jest_integration into master
Coverage Report
|
Coverage after merging feat/add_jest_integration into master
Coverage Report
|
Add integration test package to test setup of a11y matcher
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.
Thanks for the review @trevor-bliss
I have taken care of your comments. Can you review please.
packages/jest/src/matcher.ts
Outdated
* @param config - A11yConfig to be used to test for accessibility. Defaults to extended. | ||
*/ | ||
export async function toBeAccessible( | ||
receivedDom: Document = document, |
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.
axe supports element references, node lists and css selectors - defaulting to the entire DOM.
Which of those would make sense? Or could we just make the first parameter a passthrough?
https://github.com/dequelabs/axe-core/blob/develop/doc/API.md#context-parameter
packages/jest/src/matcher.ts
Outdated
receivedDom: Document = document, | ||
config: A11yConfig | ||
): Promise<jest.CustomMatcherResult> { | ||
return toBeAccessible(receivedDom, config); |
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.
Yeah was wondering the same after adding it. The marginal readability achieved with the With(Config)
matcher is probably not worth the overhead. Removed.
packages/assert/src/assert.ts
Outdated
import { A11yConfig } from '@sa11y/preset-rules'; | ||
import { Formatter, a11yResultsFormatter } from '@sa11y/format'; | ||
import * as axe from 'axe-core'; | ||
import { ElementContext, RunOptions } from 'axe-core'; |
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.
Agree - felt weird for me too, fixed.
packages/jest/src/index.ts
Outdated
*/ | ||
|
||
export { toBeAccessible, toBeAccessibleWith } from './matcher'; | ||
export { adaptA11yConfig, registerA11yMatchers } from './setup'; |
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.
Right now the tests are using it - so it needs to be exported I guess?
But I have been thinking about its usefulness to consumers as well.. Right now with its limited scope, its probably not useful. But if we allowed consumers to register their own adapters that could be useful. But there isn't a clear use case for such a workflow right now.
packages/jest/README.md
Outdated
In the `jest.config.js` at the root of your project, add | ||
|
||
```javascript | ||
const { jestConfig } = require('@sa11y/jest'); |
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.
@trevor-bliss In 2b18c4b I am trying to automate setup of the a11y matchers so that they can be setup once in a project instead of every test module. But I am not able to get it to work. Can you please take a look at see what I am missing.
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.
I would simplify the jest config in the integration test package and work backwards from a working solution.
In packages/test-integ/jest.config.js
, can you have the setupFilesAfterEnv
option directly that points to a local file that calls expect.extend
to add a trivial custom matcher?
Once you have that then start using the imports/exports until you get back to where you wanted to be.
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.
Good suggestion, let me try that out - thanks.
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.
@trevor-bliss Ran into some issues trying the config jest
I have opened an issue with info #11
If there are no other issues with this PR can we merge this while I continue working on the jest config? I can revert 2b18c4b from this PR and add it to another branch?
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.
Yeah works for me. Approved the PR.
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.
Thanks @trevor-bliss
Modified jest setup as per your suggestions in #11
Can you please review
change name of CI action to Lint, Test
as it is used only internally currently
@mohanraj-r , I'm working on testing the work in this Pull Request in existing jest tests from another repo, but I'm having a difficult time referencing |
Thanks a lot @spelak-salesforce cd /tmp
git clone --single-branch --branch feat/add_jest_integration [email protected]:salesforce/sa11y.git
cd /tmp/sa11y && yarn install && yarn build
cd <back to project dir where we want to install sa11y jest>
yarn add /tmp/sa11y/packages/jest/ Please let me know if that works for you. |
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.
with a11y matcher api Fixes #11
for uniformity
@cordeliadillon I refactored the docs since your last review. Can you please look a quick look at them to make sure they make sense. |
Fix incorrect integration test, add doc about async usage
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.
🚢
Primary addition starts at the jest package
jest
Accessibility matcher for Jest
Setup
The accessibility matcher helper APIs need to be registered with Jest before they can be used in tests.
Project level
You can set up the a11y matchers once at the project level to make it available to all the Jest tests in the project.
For an example look at the Integration tests.
jest-setup.js
) and add the following code that registers the a11y matchersIn the
jest.config.js
at the root of your project, add:Test module level
Invoke
registerA11yMatchers
before using the accessibility matchers in the tests e.g.Usage