feat(jest): add custom results processor for a11y failures #62

mohanraj-r · 2021-05-21T20:54:57Z

Features ✨

Jest automatic checks 🤖

jest: add a custom test results processor
- create a new test suite to hold a11y errors
- add WCAG metadata to results output
- transform a11y error details into suite, test names

Results

Before - with default results processor (a11y error is embedded within the test failure)

JSON result:

"assertionResults": [
  {
    "ancestorTitles": [
      "integration test @sa11y/jest"
    ],
    "failureMessages": [
      "A11yError: 1 Accessibility issues found\n * (link-name) Links must have discernible text: a\n\t- Help URL: https://dequeuniversity.com/rules/axe/4.1/link-name\n    at Function.checkAndThrow (packages/format/src/format.ts:67:19)\n    at automaticCheck (packages/jest/src/automatic.ts:54:19)\n    at Object.<anonymous> (packages/jest/src/automatic.ts:69:13)"
    ],
    "fullName": "integration test @sa11y/jest should throw error for inaccessible dom",
    "location": null,
    "status": "failed",
    "title": "should throw error for inaccessible dom"
  }
]

After - with new sa11y results processor added in this PR (and automatic checks)

Original JSON test result (failure with embedded a11y error) is now disabled

"assertionResults": [
        {
          "ancestorTitles": [
            "integration test @sa11y/jest"
          ],
          "failureMessages": [
            "A11yError: 1 Accessibility issues found\n * (link-name) Links must have discernible text: a\n\t- Help URL: https://dequeuniversity.com/rules/axe/4.1/link-name\n    at Function.checkAndThrow (packages/format/src/format.ts:67:19)\n    at automaticCheck (packages/jest/src/automatic.ts:54:19)\n    at Object.<anonymous> (packages/jest/src/automatic.ts:69:13)"
          ],
          "fullName": "integration test @sa11y/jest should throw error for inaccessible dom",
          "location": null,
          "status": "disabled",
          "title": "should throw error for inaccessible dom"
        },
]

New JSON test result created to represent the a11y failure bringing a11y metadata to forefront

"assertionResults": [
  {
    "ancestorTitles": [
      "integration test @sa11y/jest",
      "integration test @sa11y/jest should throw error for inaccessible dom"
    ],
    "failureMessages": [
      "Accessibility issues found: Links must have discernible text\nCSS Selectors: a\nHTML element: <a href=\"#\"></a>\nHelp: https://dequeuniversity.com/rules/axe/4.1/link-name\nTests: \"integration test @sa11y/jest should throw error for inaccessible dom\"\nSummary: Fix all of the following:\n  Element is in tab order and does not have accessible text\n\nFix any of the following:\n  Element does not have text that is visible to screen readers\n  aria-label attribute does not exist or is empty\n  aria-labelledby attribute does not exist, references elements that do not exist or references elements that are empty\n  Element has no title attribute"
    ],
    "fullName": "Links must have discernible text: a",
    "location": null,
    "status": "failed",
    "title": "should throw error for inaccessible dom"
  }
],

TODO

TODOs from last PR (feat(jest): de-duplicate a11y failures for automatic checks #61)
Refactor to unify consolidated results processing logic from last PR
Address missing code cov that has been suppressed in this PR (using /* istanbul ignore next */) as much as possible esp the non-edge cases

…esult

add jest/circus as test runner due to jestjs/jest#11405

codecov · 2021-05-26T02:21:36Z

Codecov Report

Merging #62 (d24cea3) into master (64c35af) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master       #62   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           18        19    +1     
  Lines          208       234   +26     
  Branches        27        31    +4     
=========================================
+ Hits           208       234   +26

Impacted Files	Coverage Δ
packages/common/src/format.ts	`100.00% <100.00%> (ø)`
packages/common/src/helpers.ts	`100.00% <100.00%> (ø)`
packages/common/src/index.ts	`100.00% <100.00%> (ø)`
packages/format/src/format.ts	`100.00% <100.00%> (ø)`
packages/format/src/result.ts	`100.00% <100.00%> (ø)`
packages/format/src/wcag.ts	`100.00% <100.00%> (ø)`
packages/jest/src/automatic.ts	`100.00% <100.00%> (ø)`
packages/jest/src/matcher.ts	`100.00% <100.00%> (ø)`
packages/jest/src/resultsProcessor.ts	`100.00% <100.00%> (ø)`
packages/test-utils/src/test-data.ts	`100.00% <100.00%> (ø)`
... and 10 more

…tests

…g suite

trevor-bliss

Overall looks good. Couple questions.

packages/jest/src/resultsProcessor.ts

trevor-bliss · 2021-06-17T18:26:30Z

packages/jest/src/resultsProcessor.ts

+ * Modify existing test suites, test results containing a11y errors after a11y errors
+ *  are extracted out into their own test suites and results.
+ */
+function modifyTestSuiteResults(results: AggregatedResult, testSuite: TestResult, testResult: AssertionResult) {


I don't really understand what the purpose of this function is. Can you explain more?

Since the custom reporter is transforming the a11y errors into their own test failures contained within a new suite based on the a11y rule (const suiteKey = [Sa11y ${a11yResult.wcag} ${a11yResult.id} ${suiteName}]), the original test in which the a11y error occurred is being disabled and test failure counts adjusted accordingly. Guess if the main logic is refactored to use map/filter without using any intermediates, it might be possible to just filter these out instead of disabling (the test counts will still need to be adjusted I think).

I have updated the PR desc to include docs/screenshots to illustrate this. Please check them out and let me know if it makes sense and if we can include them as part of jest README.

Ok. I just want to make sure we don't mess up the overall test and failure counts. E.g., If we have a functional failure and then that is fixed but replaced by an a11y failure, then the total tests and failure counts should remain the same.

It sounds like you're accounting for that. Just wanted to make sure.

That is a good point. The failure count and the total test count could increase with the sa11y results processor - because each a11y failure from a test would be extracted to be its own test failure. When there is only 1 a11y failure in a test, the count would stay the same. But when there are multiple unique a11y failures in a test, the count could increase. But the results processor doesn't affect default console reporter or other reporters - it affects just the JSON output. Added a limitations section to the doc to reflect this and other points - can you please review.

Also created #66 as it is important and deserves its own issue.

… file

…dability

mohanraj-r

Thanks for the review @trevor-bliss 👍
I have addressed/responded to your comments. Please take a look.

packages/jest/src/resultsProcessor.ts

mohanraj-r · 2021-06-17T22:20:57Z

packages/jest/src/resultsProcessor.ts

+ * Modify existing test suites, test results containing a11y errors after a11y errors
+ *  are extracted out into their own test suites and results.
+ */
+function modifyTestSuiteResults(results: AggregatedResult, testSuite: TestResult, testResult: AssertionResult) {


Since the custom reporter is transforming the a11y errors into their own test failures contained within a new suite based on the a11y rule (const suiteKey = [Sa11y ${a11yResult.wcag} ${a11yResult.id} ${suiteName}]), the original test in which the a11y error occurred is being disabled and test failure counts adjusted accordingly. Guess if the main logic is refactored to use map/filter without using any intermediates, it might be possible to just filter these out instead of disabling (the test counts will still need to be adjusted I think).

I have updated the PR desc to include docs/screenshots to illustrate this. Please check them out and let me know if it makes sense and if we can include them as part of jest README.

…efault

…formation examples

trevor-bliss · 2021-06-22T03:24:48Z

packages/jest/README.md

+-   Automatic check is triggered regardless of the test status which would result in the original test failure if any getting overwritten by a11y failures if any from automatic checks ([#66](https://github.com/salesforce/sa11y/issues/66))
+-   Tests using the sa11y jest api would get tested twice with automatic checks - once as part of the sa11y API in the test and again as part of the automatic check
+    -   a11y issues from automatic checks would overwrite the a11y issues found by the API
+    -   If the sa11y API has been added to the test to check specific intermediate states of the DOM, enabling automatic checks could result in missed a11y issues


I think this point is also covered by the next one below.

Yes it is related, but there is also a fine difference - Enabling automatic checks without using the API vs while also using the API. Please check if the rewording now makes sense.

I guess to me since you say the automatic check is only at the end of the test it was clear to me that checking intermediate states of the DOM, whether you called the API or not, would not be caught. I'm fine to keep it worded like this though if you think it's more clear.

Thanks Trevor. I will try to revisit and reword/remove it in a subsequent PR.

trevor-bliss · 2021-06-22T03:31:10Z

packages/jest/README.md

+    -   Workaround: Remove the DOM cleanup code from the test and opt-in to using sa11y to clean-up the DOM using the options as described above (`cleanupAfterEach: true` or `SA11Y_CLEANUP=1`)
+-   With the sa11y results processor
+    -   Though the originating test from which the a11y failures are extracted is disabled, and test counts adjusted accordingly - the original test suite failure message still contains the a11y failures.
+        -   The test suite failure message is typically not displayed or used in testing workflows. But if your testing workflow uses the test suite failure message, this might cause confusion.


The double indentation here feels a little confusing to me. Can we combine these 3 bullets into a single bullet and a couple sentences maybe?

Agree, modified - please take a look.

…tedResults

mohanraj-r

@trevor-bliss modified the docs (and also did some last bits of cleanup/refactoring). Can you please take a look.

mohanraj-r · 2021-06-22T20:53:19Z

packages/jest/README.md

+-   Automatic check is triggered regardless of the test status which would result in the original test failure if any getting overwritten by a11y failures if any from automatic checks ([#66](https://github.com/salesforce/sa11y/issues/66))
+-   Tests using the sa11y jest api would get tested twice with automatic checks - once as part of the sa11y API in the test and again as part of the automatic check
+    -   a11y issues from automatic checks would overwrite the a11y issues found by the API
+    -   If the sa11y API has been added to the test to check specific intermediate states of the DOM, enabling automatic checks could result in missed a11y issues


Yes it is related, but there is also a fine difference - Enabling automatic checks without using the API vs while also using the API. Please check if the rewording now makes sense.

mohanraj-r · 2021-06-22T20:53:37Z

packages/jest/README.md

+    -   Workaround: Remove the DOM cleanup code from the test and opt-in to using sa11y to clean-up the DOM using the options as described above (`cleanupAfterEach: true` or `SA11Y_CLEANUP=1`)
+-   With the sa11y results processor
+    -   Though the originating test from which the a11y failures are extracted is disabled, and test counts adjusted accordingly - the original test suite failure message still contains the a11y failures.
+        -   The test suite failure message is typically not displayed or used in testing workflows. But if your testing workflow uses the test suite failure message, this might cause confusion.


Agree, modified - please take a look.

mohanraj-r added 9 commits May 12, 2021 18:59

refactor: use map to consolidate results; add a util to convert axe r…

77b7277

…esult

feat: add minimal custom jest reporter

5d17571

refactor: move custom jest reporter to typescript inside jest pkg

76aa29f

add jest/circus as test runner due to jestjs/jest#11405

build: move jest reporter pkg dependency from root to jest pkg

a6d04be

feat: access and store a11y violations from reporter

443b4e1

refactor: revert to using default test runner with on run complete hook

228b384

build: remove obsolete jest circus pkg, add jest test-result pkg

483cdf7

refactor: revert conditional gates to reduce indented blocks

aed493c

refactor: serialize a11y error into failure message and parse it back

43f1f5c

mohanraj-r self-assigned this May 21, 2021

mohanraj-r changed the title ~~refactor: use map to consolidate results; add a util to convert axe result~~ feat(jest): Output consolidated a11y failures after test run May 21, 2021

mohanraj-r added 3 commits May 25, 2021 14:20

refactor: revert serialize a11y error into failure msg

ca71b30

test(eslint): add ignore pattern for unused vars rule

84c8546

refactor: consolidate a11y results from reporter keyed on test path

e183191

mohanraj-r changed the title ~~feat(jest): Output consolidated a11y failures after test run~~ feat(jest): output consolidated a11y failures after test run May 26, 2021

mohanraj-r added 8 commits May 26, 2021 14:37

refactor: replace hardcoded output file path in reporter with config

be9661f

build: add xml pkg dependency

8cf387f

feat: add xml output support for reporter

7f0bbda

feat: add a WIP test results processor that currently filters failed …

dba9a81

…tests

feat: create a new test suite for a11y errors duplicated from existin…

fc68b02

…g suite

refactor: remove err msg from original suite for now

b0f480d

refactor: extract test result processing into its own function

0695605

feat: transform a11y error details into suite, test names

57fa2be

mohanraj-r force-pushed the feat_output_failures_json branch from 937720b to 57fa2be Compare May 29, 2021 02:51

mohanraj-r added 5 commits May 31, 2021 18:35

test: add data and first set of basic tests for test results processor

097b2a1

refactor: split up results processing logic into smaller functions

0457cd2

refactor: update failure msgs format

97b8cf0

feat: add wcag metadata to results output

6f19075

refactor: move wcag metadata processing into format pkg, add tests

54bf098

mohanraj-r added 3 commits June 16, 2021 14:16

test(utils): fix missing code cov on conditional branch

34dd7e4

test: exclude statement from code cov that is causing issue in CI

7470dab

test: add code cov exceptions to edge cases

aedf3c1

mohanraj-r marked this pull request as ready for review June 17, 2021 03:14

mohanraj-r requested review from cordeliadillon and a team as code owners June 17, 2021 03:14

mohanraj-r requested a review from trevor-bliss June 17, 2021 03:14

trevor-bliss reviewed Jun 17, 2021

View reviewed changes

mohanraj-r added 2 commits June 17, 2021 14:47

test(jest): add script cmd to invoke automatic checks and output json…

15decaa

… file

refactor(resultsProcessor): rename functions and improve docs for rea…

fcc2646

…dability

mohanraj-r commented Jun 17, 2021

View reviewed changes

trevor-bliss previously approved these changes Jun 21, 2021

View reviewed changes

mohanraj-r added 2 commits June 21, 2021 12:12

refactor: add a debug flag to enable console log msg and silence by d…

88c30a6

…efault

docs(jest): add limitations of automatic checks and json result trans…

acb0ab1

…formation examples

mohanraj-r dismissed trevor-bliss’s stale review via acb0ab1 June 22, 2021 01:30

mohanraj-r added 4 commits June 21, 2021 19:56

refactor: remove duplicated consolidation logic based on axe results

7717c3d

refactor: unify consolidation based on a11y results

6582930

refactor: rename a11y results consolidate function to add for brevity

39c80ef

test: fix visual a11y issues count for wdio test

b424e86

trevor-bliss reviewed Jun 22, 2021

View reviewed changes

mohanraj-r added 2 commits June 22, 2021 13:35

refactor: create a convenience A11yResults class and remove Consolida…

6e0fc7e

…tedResults

docs(jest): reword limitations of automatic checks for readability

3052554

mohanraj-r commented Jun 22, 2021

View reviewed changes

mohanraj-r mentioned this pull request Jun 22, 2021

build(deps): bump ws from 7.4.5 to 7.4.6 #63

Closed

docs(jest): fix indentation in json results example

d24cea3

trevor-bliss approved these changes Jun 22, 2021

View reviewed changes

mohanraj-r merged commit 120fe16 into salesforce:master Jun 22, 2021

mohanraj-r deleted the feat_output_failures_json branch June 22, 2021 23:13

mohanraj-r restored the feat_output_failures_json branch June 22, 2021 23:13

mohanraj-r deleted the feat_output_failures_json branch June 23, 2021 01:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(jest): add custom results processor for a11y failures #62

feat(jest): add custom results processor for a11y failures #62

mohanraj-r commented May 21, 2021 •

edited

Loading

codecov bot commented May 26, 2021 •

edited

Loading

trevor-bliss left a comment

trevor-bliss Jun 17, 2021

mohanraj-r Jun 17, 2021 •

edited

Loading

trevor-bliss Jun 21, 2021

mohanraj-r Jun 22, 2021

mohanraj-r left a comment

mohanraj-r Jun 17, 2021 •

edited

Loading

trevor-bliss Jun 22, 2021

mohanraj-r Jun 22, 2021

trevor-bliss Jun 22, 2021

mohanraj-r Jun 22, 2021

trevor-bliss Jun 22, 2021

mohanraj-r Jun 22, 2021

mohanraj-r left a comment

mohanraj-r Jun 22, 2021

mohanraj-r Jun 22, 2021

feat(jest): add custom results processor for a11y failures #62

feat(jest): add custom results processor for a11y failures #62

Conversation

mohanraj-r commented May 21, 2021 • edited Loading

Features ✨

Jest automatic checks 🤖

Results

Before - with default results processor (a11y error is embedded within the test failure)

After - with new sa11y results processor added in this PR (and automatic checks)

TODO

codecov bot commented May 26, 2021 • edited Loading

Codecov Report

trevor-bliss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohanraj-r Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohanraj-r left a comment

Choose a reason for hiding this comment

mohanraj-r Jun 17, 2021 • 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

mohanraj-r left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohanraj-r commented May 21, 2021 •

edited

Loading

codecov bot commented May 26, 2021 •

edited

Loading

mohanraj-r Jun 17, 2021 •

edited

Loading

mohanraj-r Jun 17, 2021 •

edited

Loading