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

Replace pa11y with underlying HTML_CodeSniffer #285

Merged
merged 4 commits into from
Jan 25, 2022

Conversation

aduth
Copy link
Member

@aduth aduth commented Jan 25, 2022

Why:

  • It currently duplicates and sometimes conflicts with our integration testing setup
    • Currently, we run two separate versions of Puppeteer, which each download separate copies of Chromium
    • Also helps me to avoid debugging a vague error occurring when upgrading dependencies to latest version
  • Consolidates testing tools to Jest
  • A step toward alignment of accessibility integration testing, as it could be reasonably substituted with aXe (our runner of choice in 18F/identity-site and 18F/identity-idp).
  • Easier to run individual tests, which was difficult before since Jest setup occurred through scripts/jest.sh and did not support additional arguments. Now, test commands can go directly through the Jest CLI.
  • Fewer dependencies overall (net 1636 lines removed from package-lock.json)

Note that the intended result of these changes is an identical testing result to what existed previously, since pa11y-ci was already running HTML_CodeSniffer on our behalf. For additional context on HTML_CodeSniffer vs. aXe, see #203.

**Why**:

- It currently duplicates and sometimes conflicts with our integration testing setup
   - Currently, we run two separate versions of Puppeteer, which each download separate copies of Chromium
   - Also helps me to avoid debugging a vague error occurring when upgrading dependencies to latest version
- Consolidates testing tools to Jest
- A step toward alignment of accessibility integration testing, as it could be reasonably substituted with aXe
- Easier to run individual tests, which was difficult before since Jest setup occurred through scripts/jest.sh and did not support additional arguments. Now, test commands can go directly through the Jest CLI.
- Fewer dependencies overall
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines 36 to 37
"test": "make test",
"test-pa11y": "make test-runner-pa11y",
"test-jest": "make test-runner-jest",
"build": "make build",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I've never looked inside this file, but it seems very weird to me that the npm tasks call out to the Makefile... seems like most of the pipeline happens in JS except the jekyll build... make in a separate/future PR we can invert this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I've never looked inside this file, but it seems very weird to me that the npm tasks call out to the Makefile... seems like most of the pipeline happens in JS except the jekyll build... make in a separate/future PR we can invert this?

Yeah, it's a bit nonconventional as things stand. Also means there is no make run like there is in other projects.

I feel like Makefile and npm scripts serve many of the same goals, so maybe we could just get rid of one or the other. 🤷

@aduth aduth merged commit a9b3275 into main Jan 25, 2022
@aduth aduth deleted the aduth-pa11y-to-html-codesniffer branch January 25, 2022 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants