Skip to content

gh-104235: Fix doctest loading issues in test_enum #104236

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

Closed
wants to merge 3 commits into from
Closed

gh-104235: Fix doctest loading issues in test_enum #104236

wants to merge 3 commits into from

Conversation

chgnrdv
Copy link
Contributor

@chgnrdv chgnrdv commented May 6, 2023

Fixes problems with test_enum mentioned in #104235.

  • remove invalid doc file existence check
  • mark path to doc as module-relative by using module_relative flag

* remove invalid doc file existence check
* mark path to doc as module-relative by `module_relative` flag
'../../Doc/library/enum.rst',
optionflags=doctest.ELLIPSIS|doctest.NORMALIZE_WHITESPACE,
))
tests.addTests(doctest.DocFileSuite(
Copy link
Member

Choose a reason for hiding this comment

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

the whole test library is shipped to the user, while the docs may not.

Keep the behavior as is please.

Copy link
Contributor Author

@chgnrdv chgnrdv May 6, 2023

Choose a reason for hiding this comment

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

I agree that docs may not be present, but I see that path that is passed to os.path.exists should be changed to "../../Doc/library/enum.rst" in this case. Not sure about module_relative flag though.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we need to figure out the correct path.

@chgnrdv chgnrdv requested a review from sunmy2019 May 6, 2023 16:59
@sunmy2019
Copy link
Member

@ethanfurman

I think this one is a clean change. But we need to fix the new included test first. #104237

I think we may land this once #104237 is merged.

@ethanfurman
Copy link
Member

Fixed in #111180.

Thank you everyone for your efforts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants