-
Notifications
You must be signed in to change notification settings - Fork 577
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
[Axe] Run axe across all stories #5592
Conversation
|
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
size-limit report 📦
|
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.
Looks great! Just left a couple of comments 👀
|
||
const {entries} = componentsConfig | ||
|
||
test.describe('@aat', () => { |
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.
test.describe('@aat', () => { | |
test.describe('aat', () => { |
I would just annotate this as aat
, or remove it, since we're using the @aat
tag at the test-level to filter things out currently (although we could totally change this in the future.
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.
My thought was to remove all @aat
tests after this is merged since we don't need to generate them ourselves
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.
@joshblack do you feel like we need the other @Aat tests now?
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.
@kendallgassner I totally bet we could, especially the automated/boilerplate ones.
I think it can still be helpful to have this tag since there are stories where we would like to put the component in a specific state and then run axe, ideally. Like with menus to make sure we test things when it's open and closed or if we want to use the matrix
test helper to make sure we run axe against prop combinations of things.
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.
@joshblack If we keep the '@Aat' in this file, doesn't it break up the tests to run concurrently in aat-report. I assumed aat-runner 1-4 were different threads.
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.
@kendallgassner I think we should definitely keep @aat
in the file for the reasons you mentioned, just was saying that we could add it at the test-level instead of in the describe
(which seems like the case on line 38? let me know if I'm misreading 👀)
If we have that in place then I think we can remove the outer describe
block or replace it with just describe('aat')
Co-authored-by: Josh Black <[email protected]>
A legit color-contrast failure: flash banner with a link in it. I might need to add the |
👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/359958 |
Random question; I see the flow isn't part of the workflow |
@TylerJDev I didn't have to change the CI because @joshblack pointed out that I could use the storybook-static/index.json file to pull storybook ids ✨ . This file is generated during npx storybook build which is already run during the aat report! The aat report will run these specific tests because of the |
In follow up PR:
@aat
testsThis PR aims to run axe ci testing on all Storybook stories. I noticed in a previous PR that we had uncaught axe violations because we have to manually add a Playwright test for each Storybook story. Because of this manual effort many of our stories do not have axe testing setup.
In addition to writing a script to run axe against all stories I made fixes to all pre-existing stories with axe failures.
Changelog
New
Changed
Removed
Rollout strategy
Testing & Reviewing
Merge checklist