-
Notifications
You must be signed in to change notification settings - Fork 3
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
MCR-3080: bring back a11y tests in ci #2715
Merged
Merged
Changes from all commits
Commits
Show all changes
29 commits
Select commit
Hold shift + click to select a range
10fe077
ADd and implement cypress-axe
JasonLin0991 36ceb71
Add accessibility commands
JasonLin0991 ca34107
Fix launch darkly intercepts for local deployment.
JasonLin0991 41333e9
Add two accessibility tests.
JasonLin0991 1014ee0
Reverting bad merge
JasonLin0991 eebe3cc
ADd and implement cypress-axe
JasonLin0991 3f9a1e2
Add accessibility commands
JasonLin0991 64f664f
Fix launch darkly intercepts for local deployment.
JasonLin0991 2167a6c
Add two accessibility tests.
JasonLin0991 0bc8e79
Reverting bad merge
JasonLin0991 ce879d7
Merge remote-tracking branch 'origin/jl-mcr-3080-bring-back-a11y-test…
JasonLin0991 7861efd
lock file
JasonLin0991 cfae62b
Reverting bad merge again
JasonLin0991 e2e2df3
Merge remote-tracking branch 'origin/jl-mcr-3080-bring-back-a11y-test…
JasonLin0991 585d269
Fix lock file.
JasonLin0991 f6ff9a9
Fix lock file.
JasonLin0991 58fdd78
Remove pa11y.
JasonLin0991 14c2fe6
Add axe-core and cypress-axe to root package.json
JasonLin0991 edd8960
Cleanup
JasonLin0991 833c0ae
Use WCAG 2.2 Level AA
JasonLin0991 e60db8f
Lock file
JasonLin0991 144ae5f
Merge branch 'main' into jl-mcr-3080-bring-back-a11y-tests-in-ci
JasonLin0991 3402246
Add documentation
JasonLin0991 09dea90
Rename tests
JasonLin0991 75a1d3a
Merge branch 'main' into jl-mcr-3080-bring-back-a11y-tests-in-ci
JasonLin0991 6fe5157
Update ADR
JasonLin0991 ea421f5
Replace depreciated command
JasonLin0991 eb4b34d
Skipping violations and made tickets to address them.
JasonLin0991 321d164
Update doc
JasonLin0991 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
2 changes: 2 additions & 0 deletions
2
docs/architectural-decision-records/005-frontend-a11y-toolset.md
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
60 changes: 60 additions & 0 deletions
60
docs/architectural-decision-records/028-updated-frontend-a11y-toolset.md
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
# Updated Frontend Toolset for Accessibility Standards | ||
|
||
Decide on a toolset to help ensure MCRRS frontend development follows accessibility standards (specifically WCAG 2.2 AA guidelines). | ||
|
||
Tooling is not a replacement for engineers/designers with strong expertise in HTML or manual testing with assistive technology. Thus, the toolset will focus on the types of accessibility issues where tooling is useful. This includes validating HTML attributes, checking basic page structure, ensuring media content has descriptive tags, ensuring that buttons are used appropriately, and checking for basic color contrast. | ||
|
||
### Considerations | ||
|
||
- Must check React component design for accessibility standards | ||
- Must validate raw HTML markup for accessibility standards | ||
- Must include end to end testing for accessibility standards rather than only unit tests | ||
- Preference for easy options to run in CI | ||
|
||
### Decision Changes | ||
|
||
Updated toolset for accessibility testing and supersedes [005-frontend-a11y-toolset](005-frontend-a11y-toolset.md) | ||
|
||
[storybook addon-a11y][storybook-addon-a11y] - Provides integration with Storybook GUI which allows the team to surface a11y concerns to non-developer stakeholders. This helps make a11y issues visible beyond the code editor or command line. | ||
|
||
[eslint jsx-a11y][eslint-a11y] Provides React-friendly code linting which allows developers to find accessibility issues during local development. | ||
|
||
[cypress-axe][cypress-axe] and [axe-core][axe-core] - Accessibility test runner for end to end testing in the command line or Node.js. End to end testing can catch concerns that are not noticeable in linting or unit tests since tests run on browser. Our previous choice [pa11y][pa11y] and [pa11y-ci][pa11y-ci] seems to not be maintained anymore and no longer works with Cypress end to end testing. | ||
|
||
[jest-axe][jest-axe] - We are no longer using this tool and has been removed `package.json` | ||
|
||
### Pro/Cons | ||
|
||
#### [@axe-core/react][axe-core-react], [@axe-core/cli][axe-core-cli] | ||
|
||
- `+` This react plugin outputs accessibility warnings in Chrome Devtools when the app is running. The cli plugin surfaces a command for running in CI. | ||
- `+` Base standard for most accessibility tools. Customizable for different levels of accessibility compliance. | ||
- `-` Documentation. Couldn’t get it set up quickly, they are in progress of moving their docs and consolidating their npm modules. Minimal documentation and example on best use in CI. | ||
- `-` Heard from other teams that have struggled customization - when used with standard flags it is more strict than needed. | ||
|
||
#### [cypress-axe][cypress-axe] and [axe-core][axe-core] | ||
- `+` `axe-core` is a well known and maintained a11y testing engine | ||
- `+` `axe-core` has great documentation | ||
- `+` `cypress-axe` can run tests with specific a11y standards like `WCAG 2.2 AA`. | ||
- `+` `cypress-axe` is a Cypress plugin for axe-core and most of the configuration for this plugin follows `axe-core` which has great documentation. | ||
- `-` `cypress-axe` itself has sparse documentation | ||
- `-` `cypress-axe` is very minimal in features and if we wanted specific things like reporting, we would have to implement that ourselves. | ||
|
||
#### [Cypress Accessibility][cypress-accessibility] | ||
- `+` Built in accessibility testing by Cypress. | ||
- `-` Still in early access and not much is known about implementation or documentation. | ||
- `-` As of now it looks like it would cost money in addition to what we already pay for Cypress. | ||
- `-` To get access we have to sign up for early access as a trial. | ||
|
||
[storybook-addon-a11y]: https://storybook.js.org/addons/@storybook/addon-a11y | ||
[eslint-a11y]: https://github.com/jsx-eslint/eslint-plugin-jsx-a11y | ||
[pa11y]: https://github.com/pa11y/pa11y | ||
[pa11y-ci]: https://github.com/pa11y/pa11y-ci | ||
[jest-axe]: https://github.com/nickcolley/jest-axe | ||
[axe-core-react]: https://www.npmjs.com/package/@axe-core/react | ||
[axe-core-cli]: https://www.npmjs.com/package/@axe-core/cli | ||
[cypress-audit]: https://github.com/mfrachet/cypress-audit | ||
[lighthouse-ci]: https://github.com/GoogleChrome/lighthouse-ci | ||
[cypress-axe]: https://github.com/component-driven/cypress-axe | ||
[axe-core]: https://github.com/dequelabs/axe-core | ||
[cypress-accessibility]: https://www.cypress.io/blog/introducing-cypress-accessibility |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@mojotalantikite I added these two dependencies here too. Correct me if things changed but, I did this because Cypress in CI runs from root and on local is runs from
services/cypress
without this here CI could not find the dependencies. A lot has changed since so is there a better, but simple, way to not have to install in both places?I could do some configuration in each test to point to the location of the install based on deployment, but if installing the deps in both doesn't cost or complicate much would this be ok?
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.
@mojotalantikite @JasonLin0991 imo this is also a good time to consider if cypress should be moved considering the new world we are. Throwing it out there, not trying to suggest moving the entire folder is needed ...just want us to think expansively since we are also doing a lot around modularizing and simplifying our top level architecture right now so maybe we are seeing another opportunity here.
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.
This is totally fine. I personally think it's clearer to declare the dependencies in each service's
package.json
that needs them. Back with Yarn 1 everything got hoisted and when we moved to pnpm it started complaining about missing packages, as it was easy to just ignore the dependency if it was already installed elsewhere with Yarn.Plus with pnpm everything is symlinked around, so it's not actually going to install it in two places, it's just going to throw in a symlink from the global package store to each service's
node_modules
that declares it.