Skip to content

Conversation

@BPScott
Copy link
Member

@BPScott BPScott commented Sep 21, 2020

Why

A11y tests used to take ~12min. Before this PR they took ~25mins. With this PR they're back down to ~13mins.

Replacing pally with using the axe runner that is already loaded within Storybook's a11y addon serves two purposes:

  • Speeds up our tests
  • Our CI tests now reflect the same errors you see in Storybook's a11y panel, leading to easier debugging. As a bonus it seems to catch more issues than our previous accessibility testing.

What

Replace pa11y with running the axe runner that Storybook provides.

Bonus

This PR previously included several changes that fixed the newly raised issues. They were removed in 9a3d014. They can be addressed in separate follow-up PRs.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 21, 2020

🟢 No significant changes to src/**/*.tsx were detected.

@BPScott BPScott changed the title Disable SB docs as it drastically slows down iframe load Disable SB docs as it drastically slows down iframe load / ally poking Sep 22, 2020
@amrocha
Copy link
Contributor

amrocha commented Sep 22, 2020

Interesting that a lot of the slowdown was due to storybook. I thought there was something weird when I tried running the tests locally and it took over 20 minutes, but was confused by the history since there didn't seem to be a smoking gun, and since the slowdown coincided with a move to Travis's free plan that was my first guess.

I like the idea of using the all components pages to test here. As long as the error reporting is accurate, I think we should go for that.

@amrocha
Copy link
Contributor

amrocha commented Sep 22, 2020

We should also add timeouts to these jobs. If we had a timeout then this would never have happened in the first place.

@BPScott BPScott changed the title Disable SB docs as it drastically slows down iframe load / ally poking Ally testing poking Sep 22, 2020
@BPScott
Copy link
Member Author

BPScott commented Sep 22, 2020

I did some pairing with @dominic earlier around a11y stuff.
The axe stuff is ace and it catches issues that pally didn't and it lining up with what storybook reports already is great, however I don't really want this to bloom into "and now we have to fix these issues" or have to add ignorelists to get green builds.

I'm thinking incremental improvements are still good. I'm gonna open a PR that disables docs (which gets us half-way to our old speed) which we can merge right away. Then have a follow up with a PR to move to axe, @dfmcphee is going to look at fixing the issues that axe raises, and once those are fixed we can merge the axe PR which gets us the rest of the way there.

@BPScott
Copy link
Member Author

BPScott commented Sep 23, 2020

Have rebased this atop master so now it's a lot smaller. Changes have been split out and merged elsewhere:

Already merged:

Pending:

  • Something to fix that Button with role of alert
  • Something to fix whatever is up with autocomplete

@BPScott BPScott force-pushed the allypoke branch 2 times, most recently from 84469e2 to 5f8cc12 Compare October 23, 2020 01:56
@BPScott
Copy link
Member Author

BPScott commented Oct 23, 2020

Had a bit of slack time so I've came back to this.

There is now a basic "here's a list of expected errors" that we can define that won't cause the build to fail.

If there's an error in the expected errors list that is triggered then it won't cause the build to fail.
If there's an error in the expected errors that is never triggered then it will cause the build to fail.

Unexpected errors are logged out. Expected errors are now. Either way it also gives you the story id so you can go look at that page in storybook to diagnose the issue.

@dfmcphee
Copy link
Contributor

I reverted all the component changes here. We can follow up with fixes separately.

@BPScott BPScott requested review from amrocha and dfmcphee October 24, 2020 01:45
@BPScott
Copy link
Member Author

BPScott commented Oct 24, 2020

haha it works!

On the other hand 68 existing errors, awwww maaaaan :(

The code for this is probably a bit fever-dreamy. Thoughts on how we could make the reporting better would be appreciated.

@BPScott BPScott requested a review from alex-page October 26, 2020 18:31
Copy link
Member

@alex-page alex-page left a comment

Choose a reason for hiding this comment

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

This is a great first step, I think we can clean up logging and exceptions in follow ups.

@BPScott BPScott changed the title Ally testing poking Reduce CI a11y test time by using Storybook provided axe runner Oct 26, 2020
@BPScott BPScott merged commit 6824807 into master Oct 26, 2020
@BPScott BPScott deleted the allypoke branch October 26, 2020 19:34
sylvhama pushed a commit that referenced this pull request Mar 26, 2021
Co-authored-by: Dominic McPhee <dominic.mcphee@shopify.com>
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.

4 participants