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

create additional test without respecting restrictions on sphinx version of dependant packages #1099

Closed
nicoa opened this issue Jan 3, 2023 · 6 comments

Comments

@nicoa
Copy link
Contributor

nicoa commented Jan 3, 2023

The error in #1094 wasn't catched by the tests as they are relying on myst-nb (executablebooks/MyST-NB#466) and sphinx-design (executablebooks/sphinx-design#118), which are both restricting sphinx<6.

Happy to hear your opinion on which of the below options would be preferred (or is there a third?).

Option 1

Add new optional dependency sections in pyproject.toml (doc_newest_sphinx and test_newest_sphinx) and add additional nox routine for testing without installing the above mentioned packages, therefore using the latest version.

In addition, add a second testing to .github/workflows/tests.yml using this optional dependency section (would currently fail on main).

Optional: somehow doublecheck that no restriction pins sphinx - maybe directly use sphinx from github upstream?

Option 2

add another test routine directly to test.yml that simply does pip install --upgrade sphinx ( maybe --pre?) before running the tests to ignore the constrained imposed by the packages. Easier but hackier, I'd prefer the first one.

@drammock
Copy link
Collaborator

drammock commented Jan 6, 2023

What about this for pyproject.toml changes:

doc_base = [everything `doc` currently has except myst-nb and sphinx-design] 
doc = ["myst-nb", "sphinx-design", "pydata-sphinx-theme[doc_base]"]
test = ["pytest", "pydata-sphinx-theme[doc_base]"]
coverage = ["pytest", "pydata-sphinx-theme[doc]", plus whatever `coverage` has now]

I think this would allow our test installs to grab sphinx 6.0 immediately (without needing to set up new github workflows). Then you can run the tests and see what fails. In test_build.py will probably will need to adjust:

  • test_logo_no_image
  • test_logo_two_images
  • test_logo_external_link
  • test_logo_external_image

Then if necessary we can add any additional tests (though I think once we're running sphinx 6, at least one of the above four will fail on main and pass on #1097).

It would probably be good to still add a separate GitHub workflow (or matrix) so that we expressly test against sphinx 5.3 and 6.0, but I'd wait to do that until the above steps are done.

@nicoa
Copy link
Contributor Author

nicoa commented Jan 7, 2023

sounds good, I'll start on providing a PR on this. Thanks for hinting me on the adjustments necessary in the testsuite.

nicoa added a commit to nicoa/pydata-sphinx-theme that referenced this issue Jan 9, 2023
@nicoa
Copy link
Contributor Author

nicoa commented Jan 9, 2023

Hey, looked into it and can't determine what needs to be changed with the tests - could you elaborate? It seems that the tests are running through anyways with old sphinx and failing with newest without those adjustments.

@drammock
Copy link
Collaborator

I created a new environment like this:

$ mamba create -n pst_sphinx6 accessible-pygments numpydoc linkify-it-py pytest pytest-regressions rich sphinxext-rediraffe sphinx-sitemap ablog jupyter_sphinx pandas plotly matplotlib numpy xarray sphinx-copybutton sphinx-togglebutton ipyleaflet nbsphinx pytest pytest-cov codecov colorama pyyaml pre-commit nox sphinx==6
$ conda activate pst_sphinx6
$ pip install -e .  # assuming already in root of cloned repo
$ nox -s test --no-venv  # lots of failures

then I updated and theme.conf and confoverrides in 3 of the tests to use html_logo instead of logo, and started messing with src/pydata_sphinx_theme/theme/pydata_sphinx_theme/components/navbar_logo.html to fix all the string has no .get() method errors. I didn't have time to finish debugging yet but FYI that's how I'm approaching it.

@nicoa
Copy link
Contributor Author

nicoa commented Jan 13, 2023

I think they don't have to be changed, compare #1097 (comment) .

Update, now that I had a coffee I can write a more detailed and focused response: This issue and the associated PR is about allowing tests to use the newest sphinx version. It will obviously fail, but that is intended - as you suggested here, this would introduce failing tests on main where #1097 would then have passing tests on that branch. Therefore, no fixing of any logo issues should happen here.

Side-Note: we would need a pre-release to have the doc-base optional set published once, otherwise tests would fail because they don't know which packages to install.

drammock pushed a commit to drammock/pydata-sphinx-theme that referenced this issue Jan 24, 2023
@drammock
Copy link
Collaborator

closing, superseded by #1136

@drammock drammock closed this as not planned Won't fix, can't repro, duplicate, stale Feb 24, 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

No branches or pull requests

2 participants