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

BUG - Fix a11y tests call #1320

Merged
merged 7 commits into from
May 10, 2023
Merged

BUG - Fix a11y tests call #1320

merged 7 commits into from
May 10, 2023

Conversation

trallard
Copy link
Collaborator

@trallard trallard commented May 9, 2023

Closes #1319

I tested this locally with nox -s a11y, and the accessibility tests are being called correctly.
They are finding several " expected " issues; as we know, there are several accessibility violations here.

If the CI is happy, this should be ready for review and merge.

EDIT:

@trallard trallard requested a review from drammock May 9, 2023 23:20
Copy link
Collaborator

@drammock drammock left a comment

Choose a reason for hiding this comment

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

I pushed commits to fix a comment typo and to add continue-on-error (which I think is a good approach, thanks for suggesting it).

In the long run I'd like to move toward:

  • having some CI jobs required for merge (and the a11y test not required)
  • once we get the a11y test green, making it also required for merge

but for today let's be satisfied that the tests actually run now :)

@trallard
Copy link
Collaborator Author

Fabulous many thanks. That approach for required workflows sounds good.
Been swamped but I should be able to send another PR soon to improve the reporting of accessibility tests and coverage.

@drammock
Copy link
Collaborator

Proof that it worked:
https://github.com/pydata/pydata-sphinx-theme/actions/runs/4938848103/jobs/8829045824?pr=1320#step:11:1411

It turns out that continue-on-error has the unfortunate side-effect of marking the job as successful (!) so we'll need to click through to the CI log to actually see whether there were a11y test failures. Apparently an alternative is to mark any subsequent steps in the job as if: always() --- this lets subsequent steps run while still marking the failed step with a red X. However:

  • the a11y test is currently the last step in its job anyway so we probably don't need either continue-on-error or if: always()
  • the a11y test should probably be its own job rather than a step at the end of the main pytest job.

I'll split this off as a separate issue to remind us to maybe reorg the workflows file in future.

(also I'm ignoring the failed profile job because GitHub is just having a bad day (keeps failing with "internal error"))

@drammock drammock merged commit 99251ff into pydata:main May 10, 2023
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.

a11y tests are not actually being run
2 participants