bpo-35753: Fix crash in doctest with unwrap-able functions #22981
Conversation
|
@ambv : do you have a few minutes to review this? |
Lib/doctest.py
Outdated
| # Protect against objects that cause unwrap to throw | ||
| isroutine = False | ||
| try: | ||
| isroutine = inspect.isroutine(inspect.unwrap(val)) |
There was a problem hiding this comment.
Without reviewing the whole thing, a nitpick here: if unwrap is the function expected to throw here I'd leave only inspect.unwrap() call inside the try block and move the inspect.isroutine() call outside it. This may make the comment above redundant, which is a win-win.
There was a problem hiding this comment.
If catching the ValueError is the correct approach, I agree with @jstasiak that only the call to unwrap should be inside the block. But I wonder whether this is not, in fact, a bug in either doctest or unwrap, but in unittest.mock.call, and should be fixed there?
There was a problem hiding this comment.
@stevendaprano : unfortunately several other types of things can cause unwrap to fail, here is another bug report similar where they use a similar technique: sphinx-doc/sphinx#7431
I will incorporate your and @jstasiak 's feedbacks on the code shortly and amend.
There was a problem hiding this comment.
It's not clear to me from the sphinx PR that this is "several other types of things" but just the same bug in a different buggy mock object. Dunder attributes are special, and if proxies or mocks misuse them, bad things happen. The ultimate cause of the bug is in the proxy, not unwrap.
Even if we patch doctest to guard against such buggy mocks, the mocks themselves should still be fixed. We shouldn't expect every caller of unwrap to guard against bad mocks, any more than we would expect the caller to guard against code that monkey-patches the built-ins.
There was a problem hiding this comment.
@stevendaprano : I really disagree with the approach of moving the changes to mock.call.
I will list the reasons:
- If doctest is fragile against un-unwrappables then imagine someone who uses doctest, but then imports a third party library or if there are other places to fix, then doctest itself is fragile against such module. Doctest should not be fragile against un-unwrappables.
- Changes to mock.call are far more risky imo and effectively subtly change the behavior of mock.call in ways that could impact downstream.
As such I strongly believe that doctest itself should be made resilient to un-unwrapables, as opposed to fixing a single un-unwrappable type object.
Can we please merge this as-is to make doctest defensive against such broken modules now?
splbio
force-pushed
the
fix-issue-35753-doctest-unwrap-master
branch
from
ec77c87
to
33958e7
Compare
|
Thank you @jstasiak and @stevendaprano . I've taken the review feedback. The function
|
splbio
force-pushed
the
fix-issue-35753-doctest-unwrap-master
branch
2 times, most recently
from
de2e327
to
a878e03
Compare
Summary: Ignore objects that inspect.unwrap throws due to too many wrappers. This is a very rare case, however it can easily be surfaced when a module under doctest imports unitest.mock.call into its namespace. This commit adds a test that should fail prior to the fix to the doctest module.
Summary: Ignore objects that inspect.unwrap throws due to too many wrappers. This is a very rare case, however it can easily be surfaced when a module under doctest imports unitest.mock.call into its namespace. This commit adds the fix to the module. We simply skip any object that throws this exception. This should handle the majority of cases. Future thoughts: 1. Should catching this be optional defaulting to true, but allowing for folks to rethrow the exception to insist on truly clean code? 2. Should we break out _find() into more sub functions sp that we can more easily derive our own versions for each sub function that _find does making it easier to fix future issues? 3. Should we include a way to denylist by `id` some objects so that future objects that cause problems with doctest can be passed in to ignore? Test Plan: Reviewers:
splbio
force-pushed
the
fix-issue-35753-doctest-unwrap-master
branch
from
a878e03
to
81e0d99
Compare
|
@tim-one : do you have some time to review this PR please? |
|
@rhettinger : I heard through the grapevine that tim-one is not very active, do you have time for this review or another reviewer? |
|
@tim-one do you have time to review this? |
|
@stevendaprano : do you have time to respond to this? I'm guessing you want me to fix python mock instead of doing it this way in this patch. I just want to confirm this before trying to write something that it may eventually be included in python as a fix. |
ambv
added
needs backport to 3.9
|
@ambv: Please replace |
…22981) Ignore objects that inspect.unwrap throws due to too many wrappers. This is a very rare case, however it can easily be surfaced when a module under doctest imports unitest.mock.call into its namespace. We simply skip any object that throws this exception. This should handle the majority of cases. (cherry picked from commit 565a318) Co-authored-by: Alfred Perlstein <alfred@fb.com>
|
Sorry, @splbio and @ambv, I could not cleanly backport this to |
|
GH-25926 is a backport of this pull request to the 3.10 branch. |
…#25926) Ignore objects that inspect.unwrap throws due to too many wrappers. This is a very rare case, however it can easily be surfaced when a module under doctest imports unitest.mock.call into its namespace. We simply skip any object that throws this exception. This should handle the majority of cases. (cherry picked from commit 565a318) Co-authored-by: Alfred Perlstein <alfred@fb.com>
Doctest needs to Ignore objects that inspect.unwrap throws ValueError due to "infinite" nesting.
This case is relatively commonly reproduced by importing unittest.mock.call into a module's namespace.
The following code produces a module that will crash doctest:
This is due to the call to unwrap throwing an uncaught ValueError.
For this case we can simply ignore the function and not attempt to extract docstring from this function.
This diff is intentionally done as a branch with two commits to provide failing test case and then a fix to show the problem is contained.
Future thoughts:
idsome objects so that future objects that cause problems with doctest can be passed in to ignore?https://bugs.python.org/issue35753