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

Make helpers configurable in setupGlobalA11yHooks mode #319

Merged
merged 18 commits into from
Nov 11, 2021

Conversation

jgwhite
Copy link
Contributor

@jgwhite jgwhite commented Nov 9, 2021

This adds an additional argument, options, to setupGlobalA11yHooks which allows us to specify additional helpers to hook into.

import {
  setupGlobalA11yHooks,
  DEFAULT_A11Y_TEST_HELPER_NAMES,
} from 'ember-a11y-testing/test-support';

setupGlobalA11yHooks(() => true, {
  helpers: [...DEFAULT_A11Y_TEST_HELPER_NAMES, 'render', 'tab'],
});

The motivating use case is to automatically perform an audit when the render helper is used, thereby improving automated coverage for component tests.

This allows ember-a11y-testing to automatically perform an audit when
the `render` helper is used, thereby improving automated coverage for
component tests.

We’ve added in a test component test to ensure this is plumbed-in
correctly. Note the comment about the surprising additional audit in
this context.

Co-authored-by: Melanie Sumner <[email protected]>
@drewlee
Copy link
Contributor

drewlee commented Nov 9, 2021

This would be a breaking change IMO, in that we'd be introducing a new point for a test to fail where one didn't exist before. There are also perf considerations, in that the total volume of a11y audits would substantially increase.

I'm wondering if instead we could make this an opt-in setting, or expose an API to make global hook registration configurable, as this project isn't quite ready for a v5 release. I'm curious to get @scalvert's take, as he authored the hooks implementation, and I'm sure there was a good reason why we didn't leverage render.

@MelSumner
Copy link
Member

@drewlee I think even if we consider this a breaking change AND decide that it should be an option, the option should be to turn it off (having it on should be the default state). WDYT?

@scalvert
Copy link
Contributor

scalvert commented Nov 9, 2021

I really like this idea for a few reasons:

  1. It fills the gap nicely for rendering tests so we have a complement to the application tests that invoke visit
  2. It provides a tiered set of APIs that cover more test use cases

I also like what you suggested, @MelSumner, WRT to making this an option. What if we, in fact, made all of the hook bindings optional, with specific defaults. This would allow us to add this change, keep the current set of defaults, and give folks a path to using render in addition to those defaults. The next major we could then fold render into that default set, which would be a natural time for it to graduate.

Thoughts?

@jgwhite
Copy link
Contributor Author

jgwhite commented Nov 10, 2021

Thanks for the splendid feedback and ideas y'all. I'll continue working on it this week.

@jgwhite
Copy link
Contributor Author

jgwhite commented Nov 10, 2021

@gregone and I addressed all the feedback and added the new configuration option in the most pragmatic way possible. Looking forward to hearing what y’all think.

@jgwhite jgwhite requested a review from scalvert November 10, 2021 11:21
@jgwhite jgwhite changed the title Automatically audit on render Make set of helpers configurable in setupGlobalA11yHooks mode Nov 10, 2021
@jgwhite jgwhite changed the title Make set of helpers configurable in setupGlobalA11yHooks mode Make helpers configurable in setupGlobalA11yHooks mode Nov 10, 2021
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@jgwhite jgwhite requested a review from scalvert November 10, 2021 22:44
.eslintrc.js Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
addon-test-support/setup-global-a11y-hooks.ts Outdated Show resolved Hide resolved
jgwhite and others added 4 commits November 11, 2021 07:26
@scalvert I went back to the original implementation in the end, as it
wasn’t an issue of variable redeclaration. I think this solution is
pretty clear (or as clear as overloaded function signatures can ever
be).
@jgwhite jgwhite requested a review from scalvert November 11, 2021 10:51
@scalvert scalvert merged commit 395a2b9 into master Nov 11, 2021
@scalvert scalvert deleted the audit-on-render branch November 11, 2021 16:05
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.

4 participants