-
Notifications
You must be signed in to change notification settings - Fork 317
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
Accessibility test Kitchen Sink with Playwright #1260
Accessibility test Kitchen Sink with Playwright #1260
Conversation
Install pytest-playwright and (npm) axe-core Write test that scans admonitions with Axe How to run test: nox -s compile nox -s docs python -m http.server -d docs/_build/html/ nox -s test -- -k accessibility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done self reviewing!
To whoever reviews my PR, please bear in mind that I am a novice Python programmer, so please do not hesitate to call out any anti-patterns or code smells.
We'll also want to think about how we integrate this code into Github Actions, but rather than hold back this PR until everything is fleshed out, I wanted to get up some working code now to help anchor the conversation about what to do next.
For those who are curious, I have pasted the output from accessibility tests in a gist.
Most of the failures are color contrast issues, but there are also: landmarks need to be unique, scrollable regions need to be keyboard focusable, and links need to be clearly distinguishable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tackling this! here's a first pass with some comments. Please ping when you're ready for a second review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a first pass review - lmk if there is anything need clarification
Co-authored-by: Daniel McCloy <dan@mccloy.info>
Co-authored-by: Tania Allard <taniar.allard@gmail.com>
Co-authored-by: Tania Allard <taniar.allard@gmail.com>
Co-authored-by: Tania Allard <taniar.allard@gmail.com>
Co-authored-by: Tania Allard <taniar.allard@gmail.com>
Co-authored-by: Tania Allard <taniar.allard@gmail.com>
- add pytest-base-url plugin - push more test setup into page fixture - add license header to pretty_axe_results - move pretty_axe_results and check_warnings to utils/
rm package-lock.json rm -r node_modules stb npm install --include=dev
FWIW, locally this command runs for me (there are failures, but they're not due to missing playwright). I've been pushing commits today to get the CIs to run (and then to get them to succeed) and in the process I noticed that pytest-axe supposedly has a built-in pytest mark that you can use to decorate tests; then running As an aside: I'm having trouble wrapping my head around all of this so it's hard to say what is the best approach. I think what we have here is (simplified):
The mix-and-match of Probably this deserves its own issue for discussion, but in the meantime we need to implement the new a11y CI test somehow so if we can quickly converge on what we should be doing everywhere then we can just do that in one place now and then (I or someone else) can propogate that decision to the other CI jobs. Thoughts/advice from anyone are very welcome. |
That's odd. This is how it looks in my terminal |
Not sure what's different. I definitely don't have playwright installed globally (
|
I am really confused about this mix as well. So I'd be +1 on moving this to a separate discussion in the interest of standardisation
@gabalafou and I are meeting today so will make sure to discuss this PR and come up with a "good enough" approach to get this moving |
excellent. I'll open a new issue about untangling the mix for the whole project. |
] | ||
test = ["pytest", "pytest-cov", "pytest-regressions"] | ||
dev = ["pyyaml", "pre-commit", "nox", "pydata-sphinx-theme[doc,test]"] | ||
a11y = ["pytest-playwright"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this to a separate env - since it is not needed for the general tests
process.wait() | ||
|
||
|
||
@pytest.mark.a11y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided register this as a custom mark instead of using the -k
pytest flag for string matching
@drammock and @gabalafou - I just pushed some changes that should fix the issue with missing dependencies.
And all runs as expected. I also made changes to address the concerns re having separate sessions and as long as the CI is happy I suppose we can call this our first iteration and improve on top of this |
if os.environ.get("CI") or os.environ.get("GITPOD_WORKSPACE_ID"): | ||
# CI and other cloud environments are potentially missing system | ||
# dependencies, so we tell Playwright to also install the system | ||
# dependencies | ||
session.run("playwright", "install", "--with-deps") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had initially removed this until I had to update the CI tests and realised I duplicated a lot of code trying to use pytest
directly like in other tests.
So, for now, I am leaving this here and calling tests through nox
.
Note: I am unsure if we should keep the GITPOD_WORKSPACE_ID
bit. I am a Gitpod user, but I wonder if it will confuse the rest of the folks who do not use Gitpod.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hadn't heard of Gitpod, but I wasn't confused :) I figured "probably some containerized thing" (I've since looked it up).
🎉 Tests are running in CI and can be run locally with I will wait for @drammock to give this a last check but I think this can be merged and we can iterate on reporting and the such from here onwards |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one tiny typo that I'll commit through the web interface, then let's merge. Thanks @gabalafou and @trallard !!
@pytest.mark.parametrize( | ||
"url_page,selector", | ||
[ | ||
("admonitions.html", "#admonitions"), | ||
("api.html", "#api-documentation"), | ||
("blocks.html", "#blocks"), | ||
("generic.html", "#generic-items"), | ||
("images.html", "#images-figures"), | ||
("lists.html", "#lists"), | ||
("structure.html", "#structural-elements"), | ||
("structure.html", "#structural-elements-2"), | ||
("tables.html", "#tables"), | ||
("typography.html", "#typography"), | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍
if os.environ.get("CI") or os.environ.get("GITPOD_WORKSPACE_ID"): | ||
# CI and other cloud environments are potentially missing system | ||
# dependencies, so we tell Playwright to also install the system | ||
# dependencies | ||
session.run("playwright", "install", "--with-deps") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hadn't heard of Gitpod, but I wasn't confused :) I figured "probably some containerized thing" (I've since looked it up).
Closes #1168.
This pull request adds accessibility tests for the Kitchen Sink section of the PyData Sphinx Theme docs in order to check the theme for accessibility issues. As such it adds two major dependencies:
pytest-playwright
to the Python test dependenciesaxe-core
to the Node (npm) dev dependenciesIt updates the Nox file accordingly, and adds instructions to the docs.
Though mentioned already in the files added and changed, it bears repeating here: doing an accessibility check with axe-core is only a first-pass at testing for accessibility, since automated accessibility checks can only catch a fraction of web accessibility issues.