Fix hidden traceback entries of chained exceptions getting shown#10921
Merged
bluetech merged 5 commits intopytest-dev:mainfrom May 30, 2023
Merged
Fix hidden traceback entries of chained exceptions getting shown#10921bluetech merged 5 commits intopytest-dev:mainfrom
bluetech merged 5 commits intopytest-dev:mainfrom
Conversation
e0703be to
35d23d0
Compare
…kEntry TracebackEntry needs the excinfo for the `__tracebackhide__ = callback` functionality, where `callback` accepts the excinfo. Currently it achieves this by storing a weakref to the excinfo which created it. I think this is not great, mixing layers and bloating the objects. Instead, have `ishidden` (and transitively, `Traceback.filter()`) take the excinfo as a parameter.
…ash` Since `Traceback.getcrashentry` takes the `ExceptionInfo`, it is not really independent of it and is in the wrong layer. Prevent nonsensical mistakes by inlining it.
TracebackEntry being mutable caught me by surprise and makes reasoning about the exception formatting code harder. Make it a proper value.
…modifying excinfo This makes it usable as a general function, and just more understandable in general.
nicoddemus
approved these changes
May 30, 2023
Member
There was a problem hiding this comment.
Awesome work @bluetech!
The PR description giving excellent guidance and context, is definitely chef's kiss. 👌
Up to you if you want to backport this to 7.3.x, given the number of changes done here.
Member
Author
|
Thanks @nicoddemus! I think this change is a bit too big for a backport, so let's keep it for the next release only. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is my proposal for fixing #1904 (previous attempt 431ec6d was reverted).
Root cause analysis
The main entry for rendering the traceback for a test exception is
Node._repr_failure_py. It takes the excinfo and returns aTerminalRepr. Two parts of it are relevant here:pytest/src/_pytest/nodes.py
Lines 452 to 457 in 4eca606
This calls the node's
_prunetracebackmethod with the excinfo, which then proceeds to filter out internal entries, hide__tracebackhide__ = Trueframes, etc. It modifies theexcinfoin-place by updating itsexcinfo.tracebackfield.pytest/src/_pytest/nodes.py
Lines 481 to 488 in 4eca606
This calls into the main
FormattedExcinfomachinery to do the bulk of the work, but passestbfilter=Falsebecause it already did the filtering itself, in aNode-customizable manner, so doesn't want the generic filtering thatFormattedExcinfodoes.Next, need to look at how
FormattedExcinfohandles exception chaining.pytest/src/_pytest/_code/code.py
Line 951 in 4eca606
It starts with the excinfo it was passed, which is the main exception. It renders it, then checks if it has any chained exceptions (either
__cause__or__context__), and if so, creates anExceptionInfofor them, renders them, and does the same in a loop.If you've followed closely you see the issue - the main exception got the filtering treatment from the
Node's_prune_tracebackbefore it was even passed toFormattedExcinfo. But the chainedExceptionInfo's are created insideFormattedExcinfo, and we've passedtbfilter=Falseso they don't even get the basic filtering.Proposed solution
I think it would have been best if the chained
ExceptionInfowere represented in theExceptionInfoitself, like how base exceptions work. Then the_prunetracebackstuff would also prune them before passing toFormattedExcinfoand all is well. But this is somewhat difficult to achieve, so I went with something easier but less clean.Instead of doing the
_prunetracebackbeforeFormattedExcinfo, I allowtbfilterto also be a callback in addition to the existing False/True. Then we passtbfilter=self._prunetracebackinstead of calling it ourselves and passingtbfilter=False. This then causes the chained exceptions to be filtered exactly like the main exception.Technical notes
As mentioned above, the
_prunetracebackmutates theexcinfoin-place. I really didn't like this behavior for thetbfiltercallback. So instead, I changed it to be(ExceptionInfo) -> Traceback. This breaks the_prunetracebackinterface, so I renamed it to_traceback_filter.There is also some internal refactoring to make this possible, namely removing the ghastly
WeakReferencecycle which makesTracebackEntryknow about itsExceptionInfo, and also makingTracebackEntryimmutable. These are separate commits.