[terra-functional-testing] Add Accessibility Reporter #456

StephenEsser · 2020-10-27T14:17:18Z

Summary

This PR creates a Custom WebdriverIO Reporter that tracks accessibility warnings and writes them to the console at the end of the test run.

Using the axe-core API certain rules are set to reviewOnFail. This configuration option will mark rules to be returned as incomplete upon failure instead of a violation.

reviewOnFail - boolean(option, default false). Override the result of a rule to return "Needs Review" rather than "Violation" if the rule fails.

When running axe if warnings are found the Terra.validates.accessibility() command emits a process message the reporter subscribes to for communication. The reporter stores this information and uses it at the end of the test run to generate an accessibility report that logs warnings for any tests that reported them.

const ruleOverrides = {
  'color-contrast': { reviewOnFail: true }
}

Example Output:

Additional Details

Warnings do not fail the test run.

This approach only works globally when using axe.configure. Warnings cannot be set as options in the Terra.report.accessibility() command which run through axe.run which does not support the reviewOnFail option.

@cerner/terra

StephenEsser · 2020-10-27T14:30:19Z

packages/terra-functional-testing/src/commands/axe/run.js

@@ -43,7 +47,7 @@ const runAxe = (overrides = {}) => {
    axe.run(document, opts, function (error, result) {
      done({ error, result });
    });
-  }, { rules, restoreScroll: true, runOnly: ['wcag2a', 'wcag2aa', 'wcag21aa', 'section508'] });


restoreScroll was removed: dequelabs/axe-core#2388

I didn't know the scrolling that it used to do was only for color-contrast.

emilyrohrbough · 2020-10-28T14:21:46Z

packages/terra-functional-testing/src/commands/expect/toBeAccessible.js

+  // Rules that fail but are marked for review are returned in the incomplete array.
+  // https://github.com/dequelabs/axe-core/blob/develop/doc/API.md#results-object
+  if (incomplete && incomplete.length > 0) {
+    process.emit('terra:report:accessibility', { incomplete });


Should we also add violations to the report's output? I could see two ways:

one report, two sections

two axe reports for incomplete and violations

Violations are printed to the console when the expect fails; adding to the reporter also would print them again. We'd want to change how violations are printed if we wanted to pursue this, keeping in mind the expect would fail the test and print a message and then the reporter would print a message

I don't think we need to add violations to the report. The failed test run should be indication enough. I see the report as valuable on non-failed test runs.

emilyrohrbough · 2020-10-28T14:26:28Z

packages/terra-functional-testing/src/reporters/wdio-accessibility-reporter.js

+   * @param {array} results.incomplete - Incomplete accessibility results.
+   */
+  onReportAccessibility(results) {
+    this.accessibilityResults[this.currentTest] = results;


Is it possible to know the suite info here instead of recursively building it out at the end?

Probably, but I personally think traveling the tree is actually easier than tracking additional test data manually. A suite can be nested infinitely inside of other suites

emilyrohrbough · 2020-10-28T14:26:48Z

packages/terra-functional-testing/src/reporters/wdio-accessibility-reporter.js

+   * @param {RunnerStats} runner - The test runner.
+   */
+  printReport(runner) {
+    const warnings = Object.keys(this.accessibilityResults);


Is there any way to write to the console that an aggregated report can be found in the results dir

We can write to the console, but we don't currently write any files to the results dir

emilyrohrbough · 2020-10-28T14:27:29Z

packages/terra-functional-testing/src/reporters/wdio-accessibility-reporter.js

+    const output = this.travelSuite(rootSuite);
+    const spec = runner.specs[0];
+
+    this.write(`Spec: ${spec}\n${output}`);


Can we output this as a md/json report as well that can be pulled in as an build artifact and as a release site artifact?

We'll probably want something like this, but I think that work will likely happen inside of the WDIO 6 reporter that generates the reports (which may or may not be the same reporter)

emilyrohrbough · 2020-10-28T14:27:41Z

packages/terra-functional-testing/src/reporters/wdio-accessibility-reporter.js

+  }
+
+  /**
+   * Travels the suite tree to generate the accessibility report.


What do you mean by travel?

Travels though each node in the tree

emilyrohrbough · 2020-10-28T14:28:35Z

packages/terra-functional-testing/tests/fixtures/reporters/test-data-1.json

@@ -0,0 +1,75 @@
+{
+  "title": "(root)",


Should this be the spec file that was ran?

No, this was generated directly by the test runner and copy pasted into this file. The root suite is always assigned '(root)':

https://github.com/webdriverio/webdriverio/blob/master/packages/wdio-reporter/src/index.ts#L58

emilyrohrbough · 2020-10-28T14:30:14Z

packages/terra-functional-testing/tests/jest/reporters/wdio-accessibility-reporter.test.js

+  });
+
+  describe('travelSuite', () => {
+    it('should travel the suite tree and generate an accessibility report', () => {


The recursiveness of this func, was that for nested suites?

Can we add a test for this?

This test is using data generated from nested suites and has 100% jest coverage. Is there something specific you'd like to see explicit test coverage for?

it's not obvious from that was what being tested here by reading through the tests. Maybe update the description to indicate that then.

emilyrohrbough · 2020-10-28T14:34:49Z

packages/terra-functional-testing/src/reporters/wdio-accessibility-reporter.js

+  }
+
+  constructor(options) {
+    super(options);


Shall we defined the outputDir and logFile option to pass to super ? It seems there is only one option for output files? maybe, which option 2 from this comment: https://github.com/cerner/terra-toolkit/pull/456/files#r513483163

From the discussion in our WDIO 6 sync I'd say this PR is not going to write out to a file, but it is something we can discuss and circle back on down the road if we think it would be beneficial

emilyrohrbough · 2020-10-28T14:35:08Z

Quick question:

To set rules for review they can be set in the Terra Service axe options

I thought we discussed in the sync last week that we no longer wanted allow axe configure axe via the Service and it would be managed at the command level?

StephenEsser · 2020-10-28T15:30:57Z

I thought we discussed in the sync last week that we no longer wanted allow axe configure axe via the Service and it would be managed at the command level?

My understanding was that we didn't want to expose an additional API for consumers to set the axe options via the service when they are extending our wdio.conf which is still the case here.

The axe API hasn't been removed from the service options, this is just using that existing pass through, this would work today without the changes on this PR.

Do we want to remove the axe option entirely from the service?

emilyrohrbough · 2020-11-02T20:02:19Z

packages/terra-functional-testing/src/commands/axe/run.js

 */
-const runAxe = (overrides = {}) => {
+const runAxe = (options = {}) => {


Can we bind this to the Terra service so we don't need to pull these config options from a global?

I'll experiment with different ways to retrieve the service options in a more direct manner, but I don't think those changes should hold up this PR

The viewport helpers and validate elements also need to look up the service options. Making them classes and initialize with the options was not the easiest to do so I set the service options in the Terra global object. That's what I going with for now unless we have other better ways to do it.

I've created a PR to address these changes, I'd like to move axe configuration discussions onto that PR:

#466

emilyrohrbough · 2020-11-02T20:04:04Z

packages/terra-functional-testing/src/commands/axe/run.js

-    ...axeOptions && axeOptions.rules,
-    ...overrides.rules,
+    ...GLOBAL_RULE_OVERRIDES,
+    ...axeServiceOptions.rules,


I thought we decided to drop this Service Level configuration option in our sync last Wednesday? So no axe configuration when initialized the TerraService or flat on the wdio object, but instead allow per test overrides and then the global be defined like you have in GLOBAL_RULE_OVERRIDES.

I investigated removing the axe configuration from the service options, but we're going to need it to dynamically flex the color contrast rule while in lowlight theme. Either to disable it or adjust the contrast ratio.

Here's how we dynamically flex the rules today:

https://github.com/cerner/terra-toolkit-boneyard/blob/main/config/wdio/wdio.conf.js#L102

Here's an example of what removing the axe option looks like:

https://github.com/cerner/terra-toolkit/compare/remove-terra-service-axe-option

We might be able to flex the option within the service, but until we have a solution we'll need to keep it as a service option. These changes are just cleaning up the file, they're not introducing any new functionality

I assumed we'd be flex that in the service like how we are overriding the scroll rule. It's always be disabled (or warn) for the lowlight theme.

The scroll rule is static and never changes, but the color contrast rule would be dynamic based on the theme.

Removing the axe option from the service is outside the scope of this PR but I'll do additional investigation into removing the axe options while still dynamically flexing rules based on the theme. If I come up with a solution I'll make a separate PR. Removing the axe options from the service doesn't impact the intended functionality of the accessibility reporter

I've created a PR to address these changes, I'd like to move axe configuration discussions onto that PR:

#466

I've removed all changes not directly associated to the accessibility reporter from this PR

benbcai · 2020-11-03T17:53:05Z

packages/terra-functional-testing/src/reporters/wdio-accessibility-reporter.js

+  /**
+   * Formats an accessibility warning summary for a test.
+   * @param {string} uid - The unique identifier of the test.
+   * @param {number} indent - The indentation the report should have,


Did you intend to have more comment at the end or should it be a period?

This should be a period, updated:

51b3981

benbcai · 2020-11-03T18:08:44Z

packages/terra-functional-testing/src/reporters/wdio-accessibility-reporter.js

+    tests.forEach((test) => {
+      const { uid } = test;
+
+      if (this.accessibilityResults[uid]) {


Do we need to ensure that this result specifically has the incomplete type or is it guaranteed that the type is incomplete by the time it gets here?

It's guaranteed, with the current implementation the results are only stored if they contain an incomplete array

benbcai · 2020-11-03T20:16:34Z

packages/terra-functional-testing/tests/jest/commands/axe/inject.test.js

@@ -15,14 +15,14 @@ describe('Inject Axe', () => {

  it('should inject axe into the document with the provided options', () => {
    const mockExecute = jest.fn();
-    const mockRules = { rules: [{ id: 'mock-rule', enabled: true }] };
+    const mockRules = { rules: [{ enabled: true, id: 'mock-rule' }] };


Curious what the reason is for swapping these? It's more natural to me to see the rule name first followed by their attributes.

This is the order the objects will be defined for the real axe inject due to how the map generates the rules array and the order matters for the strict string comparison below. The id is always appended. For this test it doesn't matter, I've reverted the changes to how it was before

StephenEsser added 2 commits October 26, 2020 20:04

Add accessibility reporter

ac782e8

Add jest coverage

8d040a4

StephenEsser added the 📦 terra-functional-testing label Oct 27, 2020

StephenEsser requested review from dkasper-was-taken, emilyrohrbough, mjhenkes, ryanthemanuel and tbiethman as code owners October 27, 2020 14:17

StephenEsser self-assigned this Oct 27, 2020

mjhenkes temporarily deployed to terra-toolki-terra-acce-i76van October 27, 2020 14:17 Inactive

StephenEsser commented Oct 27, 2020

View reviewed changes

emilyrohrbough reviewed Oct 28, 2020

View reviewed changes

Clean up rule overrides and merges

716acc4

mjhenkes temporarily deployed to terra-toolki-terra-acce-i76van October 30, 2020 20:53 Inactive

Merge branch 'main' into terra-accessibility-reporter

763c33d

mjhenkes temporarily deployed to terra-toolki-terra-acce-i76van October 30, 2020 20:55 Inactive

emilyrohrbough reviewed Nov 2, 2020

View reviewed changes

benbcai reviewed Nov 3, 2020

View reviewed changes

benbcai approved these changes Nov 3, 2020

View reviewed changes

Merge branch 'main' into terra-accessibility-reporter

908139c

mjhenkes temporarily deployed to terra-toolki-terra-acce-i76van November 3, 2020 21:08 Inactive

Revert axe related changes

e94194e

mjhenkes temporarily deployed to terra-toolki-terra-acce-i76van November 4, 2020 00:38 Inactive

Update comments and test descriptions

51b3981

mjhenkes temporarily deployed to terra-toolki-terra-acce-i76van November 4, 2020 00:47 Inactive

Merge branch 'main' into terra-accessibility-reporter

5f58ab1

mjhenkes temporarily deployed to terra-toolki-terra-acce-i76van November 5, 2020 15:07 Inactive

mjhenkes approved these changes Nov 9, 2020

View reviewed changes

Merge branch 'main' into terra-accessibility-reporter

35b9085

mjhenkes temporarily deployed to terra-toolki-terra-acce-i76van November 9, 2020 17:28 Inactive

Merge branch 'main' into terra-accessibility-reporter

40c0860

mjhenkes temporarily deployed to terra-toolki-terra-acce-i76van November 9, 2020 19:27 Inactive

StephenEsser merged commit f7f1ce9 into main Nov 9, 2020

StephenEsser deleted the terra-accessibility-reporter branch November 9, 2020 21:58

emilyrohrbough mentioned this pull request Dec 2, 2020

[Epic] Create testing package for WebDriverIO #401

Closed

40 tasks

[terra-functional-testing] Add Accessibility Reporter #456

[terra-functional-testing] Add Accessibility Reporter #456

Conversation

StephenEsser commented Oct 27, 2020 • edited Loading

Summary

Additional Details

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilyrohrbough Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilyrohrbough Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilyrohrbough Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilyrohrbough commented Oct 28, 2020 • edited Loading

StephenEsser commented Oct 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenEsser Nov 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenEsser Nov 4, 2020 • edited Loading

Choose a reason for hiding this comment

StephenEsser commented Oct 27, 2020 •

edited

Loading

emilyrohrbough Oct 28, 2020 •

edited

Loading

emilyrohrbough Oct 28, 2020 •

edited

Loading

emilyrohrbough Oct 28, 2020 •

edited

Loading

emilyrohrbough commented Oct 28, 2020 •

edited

Loading

StephenEsser Nov 2, 2020 •

edited

Loading

StephenEsser Nov 4, 2020 •

edited

Loading