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.

Sign up for GitHub

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

Jump to bottom

Update to Sphinx 3.3.x branch and fix test mock #597

Merged
merged 2 commits into from
Nov 7, 2020
Merged

Update to Sphinx 3.3.x branch and fix test mock #597

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ab51d70
workflow: update Sphinx branch from 3.2.x to 3.3.x
utzig Nov 5, 2020
1da3aa1
tests: mock lineno when it is None
utzig Nov 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/unit_tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ jobs:
- 3.0.4
- 3.1.2
- 3.2.0
- git+https://github.com/sphinx-doc/sphinx.git@3.2.x
- git+https://github.com/sphinx-doc/sphinx.git@3.3.x
Copy link
Collaborator

Choose a reason for hiding this comment

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

@utzig Seems like 3.3.x is really broken in that case. With the current setup with requirements jobs run for 3.3.x, install 3.3.x from source, then uninstall it and fetch 3.2.x to satisfy the requirements. This results in a misleading test job running for 3.3.x.

I think commenting the 3.3.x branch out with a comment, like the master branch for Sphinx 4, is a good solution for now. Can you push a fixup for that? I will merge after, the rest is good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I was testing a little bit why it is broken and wanted to be sure the issue is not Breathe related. Turns out some of the fails are due to asserts for valid lineno, which when Sphinx calls self.get_source_info()[1] it goes through docutils into get_source_and_line in the MockStateMachine. So I presume it is possible to have a lineno that is None but the C/CPP domain parsers are currently not allowing for it (after a recent change).

Copy link
Collaborator

Choose a reason for hiding this comment

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

For the mock I suggest just returning some arbitrary number.

Copy link
Contributor Author

@utzig utzig Nov 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Done, it passes the tests, but I am not sure that means we should drop the other commit. TBH for doing another release I would rather have it not depending on Sphinx<3.3, because on one of the projects I work on, we already upgraded to 3.3.0! But it is failing for other projects, so maybe for a next release we just wait before 3.3.1? Is there a timeline for that @jakobandersen? Should I drop the requirements @vermeeren?

- git+https://github.com/sphinx-doc/[email protected]
# master (Sphinx 4) will require at least Python 3.6, so disable it for now
#- git+https://github.com/sphinx-doc/sphinx.git@master
Expand Down
2 changes: 2 additions & 0 deletions tests/test_renderer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,8 @@ def __init__(self):
self.reporter = MockReporter()

def get_source_and_line(self, lineno: int):
if lineno is None:
lineno = 42
return 'mock-doc', lineno


Expand Down