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 traceback reporting for exceptions with __cause__ cycles. #3805

Merged
merged 1 commit into from Aug 16, 2018
Merged

Fix traceback reporting for exceptions with __cause__ cycles. #3805

merged 1 commit into from Aug 16, 2018

Conversation

asottile
Copy link
Member

Resolves #3804

@coveralls
Copy link

coveralls commented Aug 11, 2018

Coverage Status

Coverage increased (+0.05%) to 92.555% when pulling 17644ff on asottile:cause_cycles into 64faa41 on pytest-dev:master.

assert (
out
== """\
:13: in unreraise
Copy link
Member

Choose a reason for hiding this comment

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

Ouch this formatting here is messy IMHO. How about this instead:

        out = "\n".join(line for line in tw.lines if isinstance(line, str))
        expected_out = textwrap.dedent(
            """\
            :13: in unreraise
                reraise()
            :10: in reraise
                raise Err() from e
            E   test_exc_chain_repr_cycle0.mod.Err

            During handling of the above exception, another exception occurred:
            :15: in unreraise
                raise e.__cause__
            :8: in reraise
                fail()
            :5: in fail
                return 0 / 0
            E   ZeroDivisionError: division by zero"""
        )
        assert out == expected_out

What do you think?

Otherwise the rest of the patch looks good, thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

ugh, I didn't even notice, black made this look way worse :(

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! 😁

@@ -719,7 +719,9 @@ def repr_excinfo(self, excinfo):
repr_chain = []
e = excinfo.value
descr = None
while e is not None:
seen = set()
while e is not None and id(e) not in seen:
Copy link
Member

Choose a reason for hiding this comment

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

i believe for exceptions its fine to use the direct object banking on the object identity hashing by default,
and that way this wouldn't incur the same cost as it does with id() on pypy

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't really want to make assumptions about exceptions since any user space exception could appear here, even one with an exceptionally bad __hash__ + __eq__. I considered using object.__hash__(e) instead of id(e) but I figure they're probably the same.

The "performance hit" in pypy is almost certainly negligible and this isn't a performance critical path (formatting failed test tracebacks).

Copy link
Member

Choose a reason for hiding this comment

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

thanks for the rebuttal

@RonnyPfannschmidt RonnyPfannschmidt merged commit 7d4c4c6 into pytest-dev:master Aug 16, 2018
@asottile asottile deleted the cause_cycles branch August 16, 2018 05:19
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

4 participants