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

Conversation

@asottile
Copy link
Member

commented Aug 11, 2018

Resolves #3804

@coveralls

This comment has been minimized.

Copy link

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.

@asottile asottile requested a review from nicoddemus Aug 13, 2018
assert (
out
== """\
:13: in unreraise

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Aug 16, 2018

Member

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!

This comment has been minimized.

Copy link
@asottile

asottile Aug 16, 2018

Author Member

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

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Aug 16, 2018

Member

Thanks! 😁

@asottile asottile force-pushed the asottile:cause_cycles branch from 1865b29 to 17644ff Aug 16, 2018
@@ -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:

This comment has been minimized.

Copy link
@RonnyPfannschmidt

RonnyPfannschmidt Aug 16, 2018

Member

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

This comment has been minimized.

Copy link
@asottile

asottile Aug 16, 2018

Author Member

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).

This comment has been minimized.

Copy link
@RonnyPfannschmidt

RonnyPfannschmidt Aug 16, 2018

Member

thanks for the rebuttal

@RonnyPfannschmidt RonnyPfannschmidt merged commit 7d4c4c6 into pytest-dev:master Aug 16, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@asottile asottile deleted the asottile:cause_cycles branch Aug 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.