-
Notifications
You must be signed in to change notification settings - Fork 341
MAINT - Ensure Playwright tests use test sites and are run in CI #2133
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
Changes from 11 commits
ed82b1f
3d4ca2d
bf18dbf
7447a8a
b4685cf
0e9d265
139ae14
8484e8f
41cbf93
5e8eecb
853ece8
d027352
ddb5997
581a91c
3f9342f
f629966
b2dbcbf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
# Minimal makefile for Sphinx documentation | ||
# | ||
|
||
# You can set these variables from the command line, and also | ||
# from the environment for the first two. | ||
SPHINXOPTS ?= | ||
SPHINXBUILD ?= sphinx-build | ||
SOURCEDIR = . | ||
BUILDDIR = _build | ||
|
||
# Put it first so that "make" without argument is like "make help". | ||
help: | ||
@$(SPHINXBUILD) -M help "$(SOURCEDIR)" "$(BUILDDIR)" $(SPHINXOPTS) $(O) | ||
|
||
.PHONY: help Makefile | ||
|
||
# Catch-all target: route all unknown targets to Sphinx using the new | ||
# "make mode" option. $(O) is meant as a shortcut for $(SPHINXOPTS). | ||
%: Makefile | ||
@$(SPHINXBUILD) -M $@ "$(SOURCEDIR)" "$(BUILDDIR)" $(SPHINXOPTS) $(O) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
[ | ||
drammock marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
"version": "dev", | ||
"url": "https://pydata-sphinx-theme.readthedocs.io/en/latest/" | ||
}, | ||
{ | ||
"name": "0.16.1 (stable)", | ||
"version": "v0.16.1", | ||
"url": "https://pydata-sphinx-theme.readthedocs.io/en/stable/" | ||
} | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
"""Test conf file - basic site with version switcher.""" | ||
|
||
# -- Project information ----------------------------------------------------- | ||
project = "PyData Tests" | ||
copyright = "2020, Pydata community" | ||
author = "Pydata community" | ||
|
||
root_doc = "index" | ||
|
||
# -- General configuration --------------------------------------------------- | ||
html_theme = "pydata_sphinx_theme" | ||
|
||
html_static_path = ["_static"] | ||
|
||
# Add version switcher | ||
html_theme_options = { | ||
"switcher": { | ||
"json_url": "_static/switcher.json", | ||
"version_match": "dev", | ||
}, | ||
"navbar_start": ["navbar-logo", "version-switcher"], | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
Simple site with version switcher | ||
================================= | ||
|
||
.. toctree:: | ||
:caption: Caption 1 | ||
:numbered: | ||
|
||
page1 | ||
page2 | ||
section1/index | ||
|
||
Other content | ||
------------- | ||
|
||
Nothing to see here |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nitpick] having all this link-shortening-related content here is a bit confusing; a future maintainer looking at this site might mistake the test site's purpose. I'm assuming it's a copy-paste job; although it's a bit more work I would tend toward making test sites as bare-bones as possible, with all content reinforcing the test site's purpose. For example, this page might say Page 1
======
Meaningless content; the point of this test site is the version switcher. That said, I'm also not opposed to grouping several tests into a single test site (to reduce the ratio of build time to test time), and have 1 page on the site for each test (e.g., a page for link shortening, a page for code blocks, a page for breadcrumbs, etc). But I wouldn't expect that to happen in a test site with the name There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I basically copied the test site from the base one and made some adjustments. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
Page 1 | ||
====== | ||
|
||
**normal link** | ||
|
||
- https://pydata-sphinx-theme.readthedocs.io/en/latest/ | ||
|
||
**GitHub** | ||
|
||
.. container:: github-container | ||
|
||
https://github.com | ||
https://github.com/pydata | ||
https://github.com/pydata/pydata-sphinx-theme | ||
https://github.com/pydata/pydata-sphinx-theme/pull/1012 | ||
https://github.com/orgs/pydata/projects/2 | ||
|
||
**GitLab** | ||
|
||
.. container:: gitlab-container | ||
|
||
https://gitlab.com | ||
https://gitlab.com/gitlab-org | ||
https://gitlab.com/gitlab-org/gitlab | ||
https://gitlab.com/gitlab-org/gitlab/-/issues/375583 | ||
https://gitlab.com/gitlab-org/gitlab/issues/375583 | ||
https://gitlab.com/gitlab-org/gitlab/-/issues/ | ||
https://gitlab.com/gitlab-org/gitlab/issues/ | ||
https://gitlab.com/gitlab-org/gitlab/-/issues | ||
https://gitlab.com/gitlab-org/gitlab/issues | ||
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/84669 | ||
https://gitlab.com/gitlab-org/gitlab/-/pipelines/511894707 | ||
https://gitlab.com/gitlab-com/gl-infra/production/-/issues/6788 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
:html_theme.sidebar_secondary.remove: true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto the prior [nitpick] comment; a site that only tests the version switcher probably only needs a single page, and if it needs multiple pages, they shouldn't be doing unrelated things like hiding sidebars. |
||
|
||
Page :math:`\beta` | ||
================== | ||
|
||
.. code-block:: python | ||
|
||
import pydata_sphinx_theme as pst | ||
raise RuntimeError('Test of pygments highlighting') |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
Section 1 index | ||
=============== | ||
.. toctree:: | ||
|
||
page1 | ||
https://google.com |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
Section 1 page1 | ||
=============== |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,8 @@ | ||
"""Build minimal test sites with sphinx_build_factory and test them with Playwright.""" | ||
""" | ||
Build minimal test sites with sphinx_build_factory and test them with Playwright. | ||
When adding new tests to this file, remember to also add the corresponding test site | ||
to `tests/sites/` or use an existing one. | ||
""" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is fine for now, but the advice "or use an existing one" might change depending on what others think of my (admittedly opinionated) suggestions to whittle down to a bare-bones one-purpose-per-test-site approach. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TBF I like your proposal of simplifying the tests by reducing the number of test sites. |
||
|
||
from pathlib import Path | ||
from typing import Callable | ||
|
@@ -21,6 +25,7 @@ | |
test_sites_dir = repo_path / "docs" / "_build" / "html" / "playwright_tests" | ||
|
||
|
||
# ------------------------- Helper functions ------------------------- | ||
def _is_overflowing(element): | ||
"""Check if an element is being shortened via CSS due to text-overflow property. | ||
|
||
|
@@ -42,7 +47,8 @@ def _build_test_site(site_name: str, sphinx_build_factory: Callable) -> None: | |
|
||
def _check_test_site(site_name: str, site_path: Path, test_func: Callable): | ||
"""Make the built test site available to Playwright, then run `test_func` on it.""" | ||
test_sites_dir.mkdir(exist_ok=True) | ||
# Need to ensure parent directories exist in CI | ||
test_sites_dir.mkdir(exist_ok=True, parents=True) | ||
symlink_path = test_sites_dir / site_name | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I ran into this as well while debugging; strictly speaking There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Is the reason for keeping it that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought there might be a reason for the symlink 🤷🏽♀️ but I do not see a reason why we would have to keep this as is. I can go and refactor this. |
||
try: | ||
symlink_path.symlink_to(site_path, True) | ||
|
@@ -55,65 +61,39 @@ def _check_test_site(site_name: str, site_path: Path, test_func: Callable): | |
test_sites_dir.rmdir() | ||
|
||
|
||
def test_version_switcher_highlighting(page: Page, url_base: str) -> None: | ||
# ------------------------- Test functions: style ------------------------- | ||
def test_version_switcher_highlighting( | ||
sphinx_build_factory: Callable, page: Page, url_base: str | ||
) -> None: | ||
""" | ||
In sidebar and topbar - version switcher should apply highlight color to currently | ||
selected version. | ||
""" | ||
page.goto(url=url_base) | ||
# no need to include_hidden here ↓↓↓, we just need to get the active version name | ||
button = page.get_by_role("button").filter(has_text="dev") | ||
active_version_name = button.get_attribute("data-active-version-name") | ||
# here we do include_hidden, so sidebar & topbar menus should each have a | ||
# matching entry: | ||
entries = page.get_by_role("option", include_hidden=True).filter( | ||
has_text=active_version_name | ||
) | ||
assert entries.count() == 2 | ||
# make sure they're highlighted | ||
for entry in entries.all(): | ||
light_mode = "rgb(10, 125, 145)" # pst-color-primary | ||
# dark_mode = "rgb(63, 177, 197)" | ||
expect(entry).to_have_css("color", light_mode) | ||
|
||
|
||
def test_breadcrumb_expansion(page: Page, url_base: str) -> None: | ||
"""Test breadcrumb text-overflow.""" | ||
# wide viewport width → no truncation | ||
page.set_viewport_size({"width": 1440, "height": 720}) | ||
page.goto(urljoin(url_base, "community/topics/config.html")) | ||
expect(page.get_by_label("Breadcrumb").get_by_role("list")).to_contain_text( | ||
"Update Sphinx configuration during the build" | ||
) | ||
el = page.get_by_text("Update Sphinx configuration during the build").nth(1) | ||
expect(el).to_have_css("overflow-x", "hidden") | ||
expect(el).to_have_css("text-overflow", "ellipsis") | ||
assert not _is_overflowing(el) | ||
# narrow viewport width → truncation | ||
page.set_viewport_size({"width": 150, "height": 720}) | ||
assert _is_overflowing(el) | ||
|
||
|
||
def test_breadcrumbs_everywhere( | ||
sphinx_build_factory: Callable, page: Page, url_base: str | ||
) -> None: | ||
"""Test breadcrumbs truncate properly when placed in various parts of the layout.""" | ||
site_name = "breadcrumbs" | ||
site_name = "version_switcher" | ||
site_path = _build_test_site(site_name, sphinx_build_factory=sphinx_build_factory) | ||
|
||
def check_breadcrumb_truncation(): | ||
page.goto( | ||
urljoin(url_base, f"playwright_tests/{site_name}/hansel/gretel/house.html") | ||
def check_version_switcher_highlighting(): | ||
page.goto(urljoin(url_base, f"playwright_tests/{site_name}/index.html")) | ||
# no need to include_hidden here ↓↓↓, we just need to get the active | ||
# version name | ||
button = page.get_by_role("button").filter(has_text="dev") | ||
active_version_name = button.get_attribute("data-active-version-name") | ||
drammock marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
active_version_name = button.get_attribute("data-active-version-name") | ||
print(active_version_name) | ||
trallard marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# here we do include_hidden, since we are not adding this in the sidebar | ||
trallard marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# we should only get one entry | ||
entries = page.get_by_role("option", include_hidden=True).filter( | ||
has_text=active_version_name | ||
) | ||
# sidebar should overflow | ||
text = "In the oven with my sister, so hot right now. Soooo. Hotttt." | ||
el = page.locator("#main-content").get_by_text(text).last | ||
assert _is_overflowing(el) | ||
# footer containers never trigger ellipsis overflow because min-width is content | ||
el = page.locator(".footer-items__center > .footer-item") | ||
assert not _is_overflowing(el) | ||
assert entries.count() == 1 | ||
# make sure they're highlighted | ||
for entry in entries.all(): | ||
light_mode = "rgb(10, 125, 145)" # pst-color-primary | ||
# dark_mode = "rgb(63, 177, 197)" | ||
expect(entry).to_have_css("color", light_mode) | ||
|
||
_check_test_site(site_name, site_path, check_breadcrumb_truncation) | ||
_check_test_site(site_name, site_path, check_version_switcher_highlighting) | ||
|
||
|
||
def test_colors(sphinx_build_factory: Callable, page: Page, url_base: str) -> None: | ||
|
@@ -143,3 +123,53 @@ def check_colors(): | |
expect(el).to_have_css("color", hover_color) | ||
|
||
_check_test_site(site_name, site_path, check_colors) | ||
|
||
|
||
# ------------------------- Test functions: layout & components ----------------------- | ||
|
||
|
||
def test_breadcrumb_expansion( | ||
sphinx_build_factory: Callable, page: Page, url_base: str | ||
trallard marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) -> None: | ||
"""Test breadcrumb text-overflow.""" | ||
site_name = "breadcrumbs" | ||
site_path = _build_test_site(site_name, sphinx_build_factory=sphinx_build_factory) | ||
|
||
def check_breadcrumb_expansion(): | ||
# wide viewport width → no truncation | ||
page.set_viewport_size({"width": 1440, "height": 720}) | ||
page.goto(urljoin(url_base, "community/topics/config.html")) | ||
expect(page.get_by_label("Breadcrumb").get_by_role("list")).to_contain_text( | ||
"Update Sphinx configuration during the build" | ||
) | ||
el = page.get_by_text("Update Sphinx configuration during the build").nth(1) | ||
expect(el).to_have_css("overflow-x", "hidden") | ||
expect(el).to_have_css("text-overflow", "ellipsis") | ||
assert not _is_overflowing(el) | ||
# narrow viewport width → truncation | ||
page.set_viewport_size({"width": 150, "height": 720}) | ||
assert _is_overflowing(el) | ||
|
||
_check_test_site(site_name, site_path, check_breadcrumb_expansion) | ||
|
||
|
||
def test_breadcrumbs_everywhere( | ||
sphinx_build_factory: Callable, page: Page, url_base: str | ||
) -> None: | ||
"""Test breadcrumbs truncate properly when placed in various parts of the layout.""" | ||
site_name = "breadcrumbs" | ||
site_path = _build_test_site(site_name, sphinx_build_factory=sphinx_build_factory) | ||
|
||
def check_breadcrumb_truncation(): | ||
page.goto( | ||
urljoin(url_base, f"playwright_tests/{site_name}/hansel/gretel/house.html") | ||
) | ||
# sidebar should overflow | ||
text = "In the oven with my sister, so hot right now. Soooo. Hotttt." | ||
el = page.locator("#main-content").get_by_text(text).last | ||
assert _is_overflowing(el) | ||
# footer containers never trigger ellipsis overflow because min-width is content | ||
el = page.locator(".footer-items__center > .footer-item") | ||
assert not _is_overflowing(el) | ||
|
||
_check_test_site(site_name, site_path, check_breadcrumb_truncation) |
Uh oh!
There was an error while loading. Please reload this page.