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

Closes #8796 - DoctTest line numbers not found due to method wrapping #8861

Merged
merged 5 commits into from Nov 1, 2021

Conversation

denivyruck
Copy link
Contributor

Hey all!

Used the issue's own test as a test case. Looks a bit bloated so let me know if can be improved!

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Hey @denivyruck! 👋

Thanks a lot for this fix. Looks good, could you also please:

  1. Add yourself to AUTHORS.
  2. Add a changelog/8796.bugfix.rst, describing the fix (in user language), something along these lines:
    Fixed internal error when skipping doctests.

@denivyruck
Copy link
Contributor Author

Thanks @nicoddemus! Done

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Thanks!

@nicoddemus
Copy link
Member

Failures in 3.10 are unrelated (#8555 (comment)).

Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

Thanks for looking at this! Some points:

  • We should report it to Python, both so it gets fixed upstream eventually and so we can know if it is the right fix.

  • The docstring of _find_lineno should be updated to also mention the new workaround.

  • ISTM that _find_lineno is allowed to return None in general, so we should really handle it more gracefully. But that's probably not so simple. Shouldn't block this PR anyway.

@denivyruck
Copy link
Contributor Author

Thanks @bluetech. I'll open an issue upstream to discuss this probably today or tomorrow. Will let you know then.

For now updated the docstring as suggested 👍

@nicoddemus
Copy link
Member

@bluetech can you give this another review? Thanks!

@bluetech bluetech merged commit 0191563 into pytest-dev:main Nov 1, 2021
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

Successfully merging this pull request may close these issues.

None yet

3 participants