simplify conditional audit running even further

[fivetanley]

Introduction

In #203, the documentation was updated to recommend using the following code:

if (shouldForceAudit()) {
  await a11yAudit();
}

It's great to have new primitives, but this code is really verbose and it can be easy to wrap await a11yAudit() in an if statement from time to time, leading to flaky or inconsistent builds, which might encourage people to remove or disable (and forget to investigate and re-enable) a11yAudit().

For our app, we use eslint-plugin-ember-a11y-testing and its autofix features to ensure every call to visit/render/etc is followed by await a11yAudit(). This already adds a fair amount of noise to the tests as is:

await visit('/');
await a11yAudit();
await click('.selector');
await a11yAudit();
await fillIn('.text-box', 'name');
await a11yAudit();

With the out-of-the-box ember experience, the tests would now look like:

await visit('/');
if (shouldForceAudit() {
  await a11yAudit();
}
await click('.selector');
if (shouldForceAudit()) {
  await a11yAudit();
}
await fillIn('.text-box', 'name');
if(shouldForceAudit()) {
  await a11yAudit();
}

This makes the tests much larger than they need to be. While we could add a custom function (as noted in the updated README, and the [eslint plugin supports this]), it feels like this will be fairly common for folks. The out-of-the-box experience feels difficult to maintain consistency.

Since Ember is about providing an opinionated default to ensure consistency between apps and a great out of the box experience, I'd like to propose the following.

Remove need for custom if statements by having a11yAudit check shouldForceAudit

Rather than having lots of if statement which adds noise to tests, a11yAudit should call shouldForceAudit internally. That way, all tests will look like:

await a11yAudit();

The implementation would look very similar to the existing implementation, just wrapped in an if statement:

async function a11yAudit(context, options) {
  if (shouldForceAudit()) {
    // ... rest of axe implementation from before
  }
}

What I'd propose is a couple tweaks to the new APIs:

• expose enableAudit() and disableAudit() - modify the module state for a11yAudit() using setEnableA11yAudit under the hood. This will enable conditional disabling/enabling of audits and some cool new linting features to help developers (which are mentioned below)
• Keep shouldForceAudit(), but maybe rename to auditsEnabled() to match enableAudit change above
• Update documentation to mention URL parameter
• Add URL Parameter to QUnit UI by default (if project is using QUnit, see QUnit section below)

enableAudit() and disableAudit() would not modify the urlParameters as the existing setEnableA11yAudit helpers does. It

This enables a few key things:

• API feels more declarative. There's less to read or copy/paste from the README.
• Less verbose tests
• More consistency within a codebase. No need to write a lint rule for the if statement, we can keep the ESLint plugin mentioned above the same
• Enable conditional disabling of audits per test. This can be really useful when adopting an existing test suite or temporarily disabling an audit to unblock the main branch while someone investigates the failure without introducing more test state bugs when someone forgets to re-enable the test
• enableAudit could be called in a beforeEach hook to keep state clean between tests

Enables better editor/linting integrations

This isn't something I expect to be done as part of this issue in this project, rather something I'd volunteer to do with the new APIs in eslint-plugin-ember-a11y-testing.

This would also allow me to improve the experience of eslint-plugin-ember-a11y-testing. We could add some new lint rules:

Today's existing behavior with the lint plugin is this:

// eslint logs lint error since there is no following 
await click('.foo');

The fixed code (which can be autofixed) looks like this:

// eslint logs lint error since there is no following 
await click('.foo');
await a11yAudit();

Some new additions to the rule could take advantage of enableAudit and disableAudit to help minimize state bugs between tests. For example, it could take whether enableAudit or disableAudit was called around the @ember/test-helper when deciding whether or not to log an error:

disableAudit();
// eslint passes here because of disableAudit on the previous line
click('.foo');
// maybe you need to do a waitUntil or similar thing where you don't want aXe to run until you can fix race conditions
// caused by async code that the test suite doesn't control
await waitUntil(() => find('[my-selector]'));
enableAudit();

The ESLint plugin could also require that enableAudit() was called after disableAudit() so that other @ember/test-helper calls aren't accidentally missed by the audit:

disableAudit();
// eslint errors here since there is no enableAudit() call afterward
click('.foo');

Expose UI for developers through QUnit's urlconfig

QUnit (the default for ember apps) provides some functionality in the UI to expose UI for enabling/disabling settings. Ember A11y Testing could (if the app is using QUnit), use QUnit.config.urlconfig to expose a checkbox to work with the query param for enabling/disabling tests. This would be a nicer out of box experience.

Update documentation to include value from config

In CI environments, it's common to use environment variables to determine whether or not do something. Right now it's not clear how to do that other than modifying the ember test command to include the URL query parameter. The documentation could be updated with the following example for tests/test-helper with the new APIs:

// config/environment.js

module.exports = {
  forceAccessibilityAudits: process.env.CI
}

// tests/test-helper.js
import env from 'myapp/config/environment';
import { enableAudit } from 'ember-a11y-testing';

if (env.forceAccessibilityAudits) {
  enableAudits();
}

I'm happy to make PRs as this seems like an easy change to make before a new stable version is declared. Let me know what you think! Thanks.

[scalvert]

Hey @fivetanley, thanks for the detailed write-up! I think it's useful to step back and discuss the direction we're trying to take this addon. Hopefully that will help mitigate some of your concerns WRT the current APIs.

I wrote up an issue that discusses the direction we're trying to move in. In addition, there's a second issue that describes a new API that's been added: setupGlobalA11yHooks. It's the second one that's maybe most relevant to your needs.

The simplification of the qunit APIs in ember-qunit defined in RFC-232 was a good catalyst for cleaning up the testing infrastructure. It made the testing infra more composable, and allowed app authors to extend the functionality with their own hooks. In a similar spirit, the @ember/test-helpers addon has also provided some very nice (and composable) test helpers for use within apps.

This addon mimics those helpers in that the a11yAudit calls follow a similar API. What this addon doesn't do, however, is provide an unobtrusive and non-burdensome way to add accessibility audits to tests - test authors are required to explicitly interleave audit calls in their tests. This creates a few challenges:

1. The test author has to remember to add the a11yAudit calls explicitly, and if one is forgotten you lose coverage (which is somewhat mitigated by your eslint plugin, great!)
2. Large applications that have to retrofit their tests to do a11yAudits place a high cost on their devs
3. Adding explicit a11yAudit calls effectively class these calls as separate, and as such of lesser importance than other forms of testing. This last one is very subjective, but I'm calling it out here based on experience we've had trying to roll out audit calls in our applications.

The idea with the setupGlobalA11yHooks API was that you'd be able to setup your audit calls once, based on an audit strategy of your choosing. In fact, we built the hooks API in @ember/test-helpers with this use case in mind. Once setup, all configured @ember/test-helpers helpers would handle the audit calls for you without having to explicitly make those calls yourself. This would allow test authors to write tests as normal, but take advantage of the audit calls by proxy. If this wasn't appropriate for use in a specific app, the a11yAudit primitive would still exist, allowing you to 'drop down' and use that explicitly.

With the deprecation of the a11yAuditIf, our intent was to try to provide you the primitives you need (as you stated), but without the added burden of supporting another API in this addon. The idea wasn't that you'd add conditionals all over your tests, which I agree would make your tests ugly and unreadable, but that if you needed the conditional functionality, you could compose this yourself in your app by writing your own helper. You'd then instead import and use that helper directly.

Additionally, the reason we didn't built the conditional shouldForceAudit check into a11yAudit was to not force you to have to use the enableA11yAudit query param in order to ensure your audits ran, preserving existing behavior. We wanted to ensure there was a layering effect of control for how your audits ran, providing more control over when and how they'd execute.

Based on the above, my sense is that we should not add the shouldForceAudit conditional to a11yAudit to preserve existing behavior, and allow app/addon authors to compose this functionality in their own repos. We should also try to drive folks to use the new setupGlobalA11yHooks in favor of interleaving the a11yAudit calls in their own tests. This would allow apps/addons to employ a broad strategy for ensuring correct accessibility coverage in their code.

I think the idea of having a CI flag is a reasonable one. I'd love to chat with you more to see how this fits into the broad strategy.

cc/ @rwjblue for anything I maybe have miscommunicated or left out.

[rwjblue]

I think @scalvert addressed most/all of your original points, but I did want to call out one specific thing here:

It's great to have new primitives, but this code is really verbose and it can be easy to wrap await a11yAudit() in an if statement from time to time, leading to flaky or inconsistent builds, which might encourage people to remove or disable (and forget to investigate and re-enable) a11yAudit().

The thing is, we are absolutely trying to encourage folks to remove the explicit a11yAudit() invocations in their tests! Specifically by making them automatic, there is no longer a need to manually call it. This does lets us know for sure that folks aren't forgetting to add proper a11yAudit() calls, ensuring near complete coverage (whereas before, it was very very common for folks to accidentally forget adding them). It also nicely simplifies the tests themselves so that they align very closely with a vanilla guides.emberjs.com style.

For our app, we use eslint-plugin-ember-a11y-testing and its autofix features to ensure every call to visit/render/etc is followed by await a11yAudit().

Wouldn't it be nice to not have to? Your tests can look like:

await visit('/');
await click('.selector');
await fillIn('.text-box', 'name');

And you still have the exact same guarantee as you have today, just less lines of code, less conceptual overhead to developers, etc.

tldr; these changes are geared towards making test authors "fall into the pit of success"...

[fivetanley]

Thanks for the extremely detailed responses @rwjblue @scalvert. I think I'm in agreement in all points. Thanks for walking me through the vision, I am excited every time I interact with any of the Ember projects; it's good to be in an OSS community collaborating and making a lot of progress on bringing the community and their users into a more accessible world. :)

After reading both of these responses, I think this issue can be closed. Perhaps the only carry over issue (to create as a new issue?) would be to have some way to provide dynamic configuration for enabling/disabling audits globally (CI for example, but we could allow people to define this based on any computation that provides a boolean value). What do you think?

[rwjblue]

@fivetanley - The current design is that the app would supply a function that is effectively "should audit" when they call setupGlobalA11yHooks in their tests/test-helpers.js. That function needs to return true or false, and decides if auditing should occur.

So to run on roughly 10% of tests (randomly if you use a QUnit seed, but prints info on deterministically re-running), you would do something like:

// tests/test-helper.js
// ...snip...
let invocationCounter: number = 0;
setupGlobalA11yHooks((_helperName, label) => {
  invocationCounter++;

  return invocationCounter % 10 === 0;
})

Does that do the trick, or are you asking for a slightly different thing?

[fivetanley]

That's great. Thanks!

[scalvert]

Sounds good. Feel free to open an issue related to controlling the execution via a CI env var. Thanks!