refactor(format): make the error messages more readable #13

mohanraj-r · 2020-05-29T02:31:23Z

Refactor formatter to

add number of total issues to output
sort a11y issues by impact
rearrange to bring rule-id to front
add unicode chars to mark the issue boundary and help url clearly
remove ?application=axeAPI referral from Help URLs
based on custom error object

Primary changes in

Other changes:

rename registerA11yMatchers to registerSa11yMatcher
add .circleci/config.yml

After this PR is merged, I will publish the packages to npm.

Before

  expect(received)toBeAccessible: expected document to have no accessibility violations but found
 following issues:

     Error: Page must have means to bypass repeated blocks (bypass): html
      - More info: https://dequeuniversity.com/rules/axe/3.5/bypass?application=axeAPI

    Documents must have <title> element to aid in navigation (document-title): html
      - More info: https://dequeuniversity.com/rules/axe/3.5/document-title?application=axeAPI

    Document must have one main landmark (landmark-one-main): html
      - More info: https://dequeuniversity.com/rules/axe/3.5/landmark-one-main?application=axeAPI

    Links must have discernible text (link-name): a
      - More info: https://dequeuniversity.com/rules/axe/3.5/link-name?application=axeAPI

    Page must contain a level-one heading (page-has-heading-one): html
      - More info: https://dequeuniversity.com/rules/axe/3.5/page-has-heading-one?application=axeAPI(expected)

After

 expect(5 issues)toBeAccessible: expected document to have no accessibility violations but found 5 issues:

     "⭕ (bypass) Page must have means to bypass repeated blocks: html"
        🔗 Help URL: https://dequeuniversity.com/rules/axe/3.5/bypass

    "⭕ (document-title) Documents must have <title> element to aid in navigation: html"
        🔗 Help URL: https://dequeuniversity.com/rules/axe/3.5/document-title

    "⭕ (link-name) Links must have discernible text: a"
        🔗 Help URL: https://dequeuniversity.com/rules/axe/3.5/link-name

    "⭕ (landmark-one-main) Document must have one main landmark: html"
        🔗 Help URL: https://dequeuniversity.com/rules/axe/3.5/landmark-one-main

    "⭕ (page-has-heading-one) Page must contain a level-one heading: html"
        🔗 Help URL: https://dequeuniversity.com/rules/axe/3.5/page-has-heading-one(0 issues)

Screenshot:

https://eslint.org/docs/user-guide/migrating-to-7.0.0#default-ignore-patterns

to make the badge in readme follow common convention

as it doesn't seem to be affecting the tests??

as rimraf is generic and reusable

add corresponding tests

jsdom as direct dependency doesn't seem to be needed anymore

expect(null).toBeDefined(); // success

Reformat a11y error msg to include total num of issues, unicode chars to represent a11y issue & help url, rearrange error msg fields, add jest matcher helper to include more info about received vs actual

docs(rules): add readme information with details about the rules included in each pre-config ruleset

mohanraj-r · 2020-05-29T02:43:33Z

Looks like I missed some lint errors. Although the pre-commit hook ran the linter on changes, looks the whole picture got missed. Working on fixing them.
Also added lint check to pre-push git hook to avoid this in future.

mohanraj-r · 2020-05-29T03:26:57Z

Looks like I missed some lint errors. Although the pre-commit hook ran the linter on changes, looks the whole picture got missed. Working on fixing them.
Also added lint check to pre-push git hook to avoid this in future.

Looks like the issue was a last minute update to dependencies I did after I created the PR 🤦 - which caused bunch of eslint packages to be updated - which seems to have added/changed some checks. Reverted those updates for now which fixed those errors - will investigate the eslint updates in next PR.

But now the build is failing with Jest snapshot errors saying they are not up-to-date - but the updated snapshots have been checked in and the tests pass locally after a clean build. Even checked out the branch to a diff clean location in my machine and tried out the CI commands - no issues 😕

@trevor-bliss Have you encountered this issue with Jest snapshots in CI ?

trevor-bliss · 2020-05-29T20:47:39Z

@mohanraj-r I'm not exactly sure what's going on here... My only guess is it's something to do with the unicode characters. Maybe Jest is getting confused with the ⭕symbol.

trevor-bliss · 2020-05-29T20:49:48Z

To test maybe try stripping the ⭕symbol and any other special characters before taking the snapshot?

Only other thing I can think of right now is the CI might be doing some weird caching? I'm not too familiar with github CI.

trevor-bliss · 2020-05-29T21:29:33Z

packages/format/src/format.ts

+                    const helpURL = violation.helpUrl.split('?')[0];
+                    return (
+                        // TODO: Create a formatter specifically for Jest using printReceived etc?
+                        printReceived(`⭕ (${violation.id}) ${violation.help}: ${selectors}`) +


Feels a little weird to me to call this Jest matcher API here. I would think the Jest specific APIs would only be called in the matcher package.

Agree, was thinking similar. Will try to refactor.

@trevor-bliss refactored to remove the jest specific matchers to be passed from jest package

#14 Workaround for #14

This reverts commit 6e73cd6.

move func to get num of issues from matcher to format

to check if it fixes #14

check if this fixes #14

This reverts commit dfc39c1. # Conflicts: # packages/assert/__tests__/__snapshots__/assert.test.ts.snap # packages/format/__tests__/__snapshots__/format.test.ts.snap # packages/jest/__tests__/__snapshots__/matcher.test.ts.snap

This reverts commit 72c5e28. # Conflicts: # packages/jest/__tests__/__snapshots__/matcher.test.ts.snap

… hint" This reverts commit 44fbfc0.

revert workarounds tested for #14

for consistency with build step

check if the snapshot errors could be related to OS #14

…into refactor_formatter

This reverts commit fc5c181.

to use docker image tag and new syntax

Due to #14

as there is only 1 matcher and adding sa11y name to make it easily identifiable

trevor-bliss

Mohan Raj Rajamanickam and others added 16 commits May 13, 2020 20:14

chore: update deps

3cf9f86

chore: fix eslint config for changes in 7.0

8c69649

https://eslint.org/docs/user-guide/migrating-to-7.0.0#default-ignore-patterns

ci: rename test lint phase to build

255fd76

to make the badge in readme follow common convention

test(format): reuse test data, setup from test utils

2b755d6

build: remove global-jsdom dep and registration steps

e3c869e

as it doesn't seem to be affecting the tests??

build: replace ts-cleaner with rimraf to clean dist files

61573aa

as rimraf is generic and reusable

refactor(test-utils): move check a11y error from assert to test-utils

6f81850

add corresponding tests

build: remove jsdom, add jest matcher utils to format deps

2b1fb4b

jsdom as direct dependency doesn't seem to be needed anymore

refactor(common): add common package

2028f91

refactor(common): reuse const from common in assert, test-utils

fb2aafa

test: change toBeDefined matcher to toBeTruthy as null is defined!

90babaf

expect(null).toBeDefined(); // success

feat(format): add num of issues, unicode chars, jest matcher helper

757020a

Reformat a11y error msg to include total num of issues, unicode chars to represent a11y issue & help url, rearrange error msg fields, add jest matcher helper to include more info about received vs actual

build: update dependencies, npm scripts

2c92e5d

chore(release): update changelog

b21570d

chore(release): bump versions

5726a98

docs(rules): update readme to include rule descriptions (#12)

3fd7e55

docs(rules): add readme information with details about the rules included in each pre-config ruleset

mohanraj-r requested a review from a team May 29, 2020 02:31

mohanraj-r self-assigned this May 29, 2020

Mohan Raj Rajamanickam added 4 commits May 28, 2020 19:52

build: add lint check to pre-push git hook

303dfe5

revert: revert update of deps due to eslint issues after upgrade

838387d

build: rename npm scripts for consistency

20c19ba

build: add ci option to test ci npm script

0076a7c

mohanraj-r changed the title ~~Refactor formatter to make the error messages more readable~~ refactor(format): make the error messages more readable May 29, 2020

trevor-bliss reviewed May 29, 2020

View reviewed changes

mohanraj-r mentioned this pull request May 29, 2020

Build failure with Jest snapshot errors #14

Closed

build(ci): workaround jest snapshot errors in github CI

6e73cd6

#14 Workaround for #14

Mohan Raj Rajamanickam and others added 27 commits May 29, 2020 15:36

Revert "build(ci): workaround jest snapshot errors in github CI"

3050c60

This reverts commit 6e73cd6.

refactor(format): extract unicode chars used in a11y err msg

58f0459

move func to get num of issues from matcher to format

style(format): remove unicode chars in a11y err msg

dfc39c1

to check if it fixes #14

revert(format): remove print received jest helper

72c5e28

to check if it fixes #14

revert(matcher): remove expected, received num of issues from hint

44fbfc0

check if this fixes #14

Revert "revert(format): remove print received jest helper"

a6f41b3

This reverts commit 72c5e28. # Conflicts: # packages/jest/__tests__/__snapshots__/matcher.test.ts.snap

Revert "revert(matcher): remove expected, received num of issues from…

b4ff445

… hint" This reverts commit 44fbfc0.

revert(format): revert changes to snapshots

552332b

revert workarounds tested for #14

build(ci): use test:ci in code cov step

666b2a0

for consistency with build step

build(ci): switch to macos for builds to test #14

fc5c181

check if the snapshot errors could be related to OS #14

Add .circleci/config.yml

9ea7cf6

Merge branch 'circleci-project-setup' of github.com:salesforce/sa11y …

2691a00

…into refactor_formatter

Revert "build(ci): switch to macos for builds to test #14"

47996ce

This reverts commit fc5c181.

ci: switch to node 12

06177cb

ci: fix circle ci config

db350eb

to use docker image tag and new syntax

ci: fix node js tag version in circleci config

6989d6f

ci: disable build and test in github ci

f804b0d

Due to #14

ci: add codecov artifacts to circleci, fix status badges in readme

376ff12

refactor(format): add custom error type for a11y errors

c53ff32

refactor(format): fix default parameters destructuring of format options

c416cfb

refactor(matcher): rename registerA11yMatchers to registerSa11yMatcher

3908974

as there is only 1 matcher and adding sa11y name to make it easily identifiable

docs(contributing): add info about local testing with circle ci

b262833

feat(format): sort a11y issues by impact

6ce6620

test(integration): add test to check error thrown for inaccessible dom

d3025b1

build(format): remove obsolete jest matcher utils dep

fbe7628

docs: update changelog, readme to add common pkg

179c1fb

trevor-bliss approved these changes Jun 2, 2020

View reviewed changes

mohanraj-r merged commit 80a5d05 into master Jun 2, 2020

mohanraj-r deleted the refactor_formatter branch June 2, 2020 23:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(format): make the error messages more readable #13

refactor(format): make the error messages more readable #13

mohanraj-r commented May 29, 2020 •

edited

Loading

mohanraj-r commented May 29, 2020

mohanraj-r commented May 29, 2020

trevor-bliss commented May 29, 2020

trevor-bliss commented May 29, 2020

trevor-bliss May 29, 2020

mohanraj-r May 29, 2020

mohanraj-r Jun 2, 2020

trevor-bliss left a comment

refactor(format): make the error messages more readable #13

refactor(format): make the error messages more readable #13

Conversation

mohanraj-r commented May 29, 2020 • edited Loading

Before

After

mohanraj-r commented May 29, 2020

mohanraj-r commented May 29, 2020

trevor-bliss commented May 29, 2020

trevor-bliss commented May 29, 2020

trevor-bliss May 29, 2020

Choose a reason for hiding this comment

mohanraj-r May 29, 2020

Choose a reason for hiding this comment

mohanraj-r Jun 2, 2020

Choose a reason for hiding this comment

trevor-bliss left a comment

Choose a reason for hiding this comment

mohanraj-r commented May 29, 2020 •

edited

Loading