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

Fix hidden traceback entries of chained exceptions getting shown #10921

Merged
merged 5 commits into from
May 30, 2023

Conversation

bluetech
Copy link
Member

@bluetech bluetech commented Apr 16, 2023

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 a TerminalRepr. Two parts of it are relevant here:

pytest/src/_pytest/nodes.py

Lines 452 to 457 in 4eca606

if self.config.getoption("fulltrace", False):
style = "long"
else:
self._prunetraceback(excinfo)
if style == "auto":
style = "long"

This calls the node's _prunetraceback method with the excinfo, which then proceeds to filter out internal entries, hide __tracebackhide__ = True frames, etc. It modifies the excinfo in-place by updating its excinfo.traceback field.

pytest/src/_pytest/nodes.py

Lines 481 to 488 in 4eca606

return excinfo.getrepr(
funcargs=True,
abspath=abspath,
showlocals=self.config.getoption("showlocals", False),
style=style,
tbfilter=False, # pruned already, or in --fulltrace mode.
truncate_locals=truncate_locals,
)

This calls into the main FormattedExcinfo machinery to do the bulk of the work, but passes tbfilter=False because it already did the filtering itself, in a Node-customizable manner, so doesn't want the generic filtering that FormattedExcinfo does.

Next, need to look at how FormattedExcinfo handles exception chaining.

def repr_excinfo(

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 an ExceptionInfo for 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_traceback before it was even passed to FormattedExcinfo. But the chained ExceptionInfo's are created inside FormattedExcinfo, and we've passed tbfilter=False so they don't even get the basic filtering.

Proposed solution

I think it would have been best if the chained ExceptionInfo were represented in the ExceptionInfo itself, like how base exceptions work. Then the _prunetraceback stuff would also prune them before passing to FormattedExcinfo and all is well. But this is somewhat difficult to achieve, so I went with something easier but less clean.

Instead of doing the _prunetraceback before FormattedExcinfo, I allow tbfilter to also be a callback in addition to the existing False/True. Then we pass tbfilter=self._prunetraceback instead of calling it ourselves and passing tbfilter=False. This then causes the chained exceptions to be filtered exactly like the main exception.

Technical notes

As mentioned above, the _prunetraceback mutates the excinfo in-place. I really didn't like this behavior for the tbfilter callback. So instead, I changed it to be (ExceptionInfo) -> Traceback. This breaks the _prunetraceback interface, so I renamed it to _traceback_filter.

There is also some internal refactoring to make this possible, namely removing the ghastly WeakReference cycle which makes TracebackEntry know about its ExceptionInfo, and also making TracebackEntry immutable. These are separate commits.

@bluetech bluetech force-pushed the tb-simplify-2 branch 2 times, most recently from e0703be to 35d23d0 Compare April 22, 2023 10:47
…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.
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.

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.

@bluetech
Copy link
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.

@bluetech bluetech merged commit 99c78aa into pytest-dev:main May 30, 2023
@bluetech bluetech deleted the tb-simplify-2 branch May 30, 2023 17:09
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.

2 participants