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

MAINT: Use different lighthouse action #548

Closed
wants to merge 1 commit into from

Conversation

choldgraf
Copy link
Collaborator

@choldgraf choldgraf commented Jan 10, 2022

This uses a dedicated GitHub Action for lighthouse to analyze and upload reports for our HTML artifacts automatically. It simplifies the configuration of our pages, and also provides reports that are a bit more understandable than our current setup.

It removes the pa11y as well - my reasoning for this is the following:

  • We already have auditing with Lighthouse, and it's not clear what pa11y provides on top of lighthouse
  • There is a lot of custom configuration for running the pa11y server. I worry that this is not easy to maintain or inspect, and given that we have an alternative with much less configuration, I think that's a better option.

Question: remove pa11y?

There's a lot of other scripting stuff that we could remove if we want to stop relying on pa11y for our accessibility. I think there are pros and cons, but wanted to show off this alternative as a start. What do others think?
closes #546

@damianavila
Copy link
Collaborator

It seems a lot of accessibility stuff was introduced by @bollwyvl here: #294.

Nick, WDYT about the py11y removal proposed by @choldgraf here?

@damianavila
Copy link
Collaborator

Btw, @choldgraf, it seems there is some branch conflict.

@choldgraf
Copy link
Collaborator Author

choldgraf commented Jan 11, 2022

Yes I would love to hear @bollwyvl's thoughts re: maintainability+complexity vs. doing a reasonable job of addressing accessibility issues.

I think the main question I have is: what are the benefits to using pa11y + lighthouse, vs just lighthouse alone

@bollwyvl
Copy link
Collaborator

i think it's a regression in development and testing robustness if there is no local capability for a developer to identify (and therefore fix) the ~30% of WCAG issues that pa11y's rules can actually say anything about. But then i haven't been doing much beyond getting that set up, as the roadmap hasn't been changed since we added it. The fact that pa11y finds those issues, and lighthouse doesn't, already suggests that it covers more than lighthouse.

To that end, I don't even know what claims lighthouse (or the wrapper action) are making, as it's more of a general-purpose tool, and was useful for putting number to things like asset pre-loading that helped us improve the perceptual speed of theme with actual metrics, and helping keep it there (provided we haven't just been bumping the thresholds down). The fact that it handles some accessibility concerns seems almost incidental, aside from google's goal of helping you convert more eyeballs (or screen readers) so they can sell more ads and personal data.

Longer term, I'd prefer that concerns like accessbility aren't delegated to a secondary test suite: i'm also looking into working accessibility testing into more real browser environments, with e.g. robotframework/selenium, but don't yet have anything concrete to demonstrate these. The advantage there is one could hijack things like Capture Page Screenshot, which get run in happy day and error situations, which would help on highly dynamic tools... which some documentation sites get closer to with e.g. thebe, jupyterlite, etc.

But same deal applies: if a tool can't be run locally and in CI worker, I feel like it's bad for open source.

@choldgraf
Copy link
Collaborator Author

Just a note that you can generate lighthouse reports directly from Google Chrome. So at least that's one option for local running, but as you say the things that lighthouse vs. pa11y test for are slightly different.

I agree with all of your points, I just worry that right now our usage of pa11y is not super actionable. As a first step, I've added some more documentation of what the pa11y error messages mean in the contributing docs: https://pydata-sphinx-theme.readthedocs.io/en/latest/contribute/topics.html#errors-in-ci-cd-and-what-to-do

Maybe this will make it easier for people to understand what the pa11y test does and how to interpret the results?

@damianavila damianavila self-requested a review January 13, 2022 19:51
@choldgraf
Copy link
Collaborator Author

choldgraf commented Jan 15, 2022

Two more updates on the pa11y stuff:

  1. Our current accessibility audit tests are broken - There's something wrong with puppetteer: https://github.com/pydata/pydata-sphinx-theme/runs/4822444059?check_suite_focus=true

    Run python docs/scripts/a11y.py --no-serve
    ##[debug]/usr/bin/bash -e /home/runner/work/_temp/7fe03fc5-38b4-4220-903d-0bf8c0a9da19.sh
    /home/runner/work/pydata-sphinx-theme/pydata-sphinx-theme/node_modules/puppeteer/lib/Page.js:215
    this.emit('error', new Error('Page crashed!'));
    
  2. Our manual audit testing uses yarn but we no longer use it for the theme. I was investigating re-enabling the a11y.py check here, but realized that it is using yarn in that script, and we don't use yarn anymore in this package. So that script is broken now, I think.

    I do not understand the script or the yarn ecosystem well enough to debug it, just highlighting it here.

@choldgraf
Copy link
Collaborator Author

I think that the accessibility audits with pa11y was just flaky, not broken. I also opened up a new PR to reduce the scope to just lighthouse, and not touching pa11y. closing this one!

@choldgraf choldgraf closed this Jan 18, 2022
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.

Use the GitHub Action for Lighthouse rather than manually running it
3 participants