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

Storyshots: Add beforeAxeTest hook #14563

Merged
merged 2 commits into from
Apr 27, 2021
Merged

Conversation

coreyjv
Copy link
Contributor

@coreyjv coreyjv commented Apr 12, 2021

Issue: #14536

What I did

Per #14536 I added a method named beforeAxeTest that is executed before the axe tests are run.

How to test

I did not see any test suites for imageSnapshot or axeTest. If I missed them please let me know and I'll add the necessary tests.

@coreyjv coreyjv changed the title Before axe test Adds beforeAxeTest per feature request #14536 Apr 12, 2021
@coreyjv
Copy link
Contributor Author

coreyjv commented Apr 26, 2021

Would it be possible to get feedback on this PR? Thanks!

@shilman shilman changed the title Adds beforeAxeTest per feature request #14536 Storyshots: Adds beforeAxeTest hook Apr 27, 2021
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

LGTM!!!

@shilman shilman changed the title Storyshots: Adds beforeAxeTest hook Storyshots: Add beforeAxeTest hook Apr 27, 2021
@shilman shilman merged commit e262a72 into storybookjs:next Apr 27, 2021
export const axeTest = (customConfig: Partial<AxeConfig> = {}) => {
const config = { ...defaultAxeConfig, ...customConfig };
const { beforeAxeTest } = config;

puppeteerTest({
Copy link
Member

Choose a reason for hiding this comment

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

Did it actually work for you @coreyjv? I'm asking because the result of puppeteerTest is no longer returned, so instead of AXE it runs the default snapshot tests.

Ideally, we would catch that on CI, but unfortunately the corresponding GitHub Action was eventually removed:
9be0706

@ndelangen do you remember what exactly wasn't functioning back then?

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'm sorry about that mistake @Hypnosphi! I didn't realize that this wouldn't be caught by CI. I can open another PR to address this if you'd like.

Copy link
Member

Choose a reason for hiding this comment

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

No problem, I'm working on another PR for the same package, so I'll just include the fix there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Hypnosphi -- PR created to address the regression here: #15005

Again my apologies for not catching this the first time.

coreyjv pushed a commit to coreyjv/storybook that referenced this pull request May 21, 2021
The regression prevented Axe tests from being run and instead the default snapshots were being used. This was due to a missing `return` statement that I overlooked in the original source because of the implicit return.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants