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

fallback to native traceback when handling ExceptionGroup (take 2) [SQUASH] #10209

Merged
merged 7 commits into from
Aug 17, 2022

Conversation

jakkdl
Copy link
Member

@jakkdl jakkdl commented Aug 12, 2022

Picking up work on and therefore closes #10071 to temporarily address #9159

Improvements over the previous PR:

  • Handles BaseExceptionGroup
  • Handles exception chains properly, using excinfo_ instead of excinfo, and sets reprcrash
  • fixed tox.ini
  • rewrote tests extending them, previous ones also had regex bugs

I'm not entirely sure how to handle the envlist in tox.ini, since hypothesis currently depends on exceptiongroup, making an extra environment for it redundant. I also wanted a test for py311 without exceptiongroup installed, but it got messy setting up a separate testenv without extras=testing. An alternative is a 3.11-only test that modifies _pytest._code.code.ExceptionGroups removing exceptiongroup.BaseExceptionGroup from it.

The test is still somewhat WIP, I want some extra matching lines to differentiate the parametrized variables, but thought I'd push my current work for initial review.

@Zac-HD

commit 41d339c46763bbe26123e1e6504b6e32290e33e1
Author: Cheukting <cheukting.ho@gmail.com>
Date:   Thu Jun 23 17:01:04 2022 +0800

    test in all py versions

commit b3572a5a12672228c3276fc8c8e05980dfb7888a
Author: Cheukting <cheukting.ho@gmail.com>
Date:   Thu Jun 23 16:41:06 2022 +0800

    add test

commit 7166a2a51e4f99046b028b663c193d8b558c7fd4
Author: Cheukting <cheukting.ho@gmail.com>
Date:   Thu Jun 23 16:00:07 2022 +0800

    update changelog

commit b958c73d489157f0c0d4e46425083a5e2e2bc851
Author: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Date:   Thu Jun 23 07:50:52 2022 +0000

    [pre-commit.ci] auto fixes from pre-commit.com hooks

    for more information, see https://pre-commit.ci

commit ea7f376c6ca37c40c83df0e4a1cfaaedb34bae91
Author: Cheukting <cheukting.ho@gmail.com>
Date:   Thu Jun 23 15:48:21 2022 +0800

    Fix MyPy

commit 97469beb1da40257e9a061a5e19548546c9312c4
Author: Cheukting <cheukting.ho@gmail.com>
Date:   Thu Jun 23 15:03:48 2022 +0800

    fix if ExceptionGroup not exist

commit 84e553642cd69b4d499231d733df91ebfa84c7ad
Author: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Date:   Thu Jun 23 03:43:27 2022 +0000

    [pre-commit.ci] auto fixes from pre-commit.com hooks

    for more information, see https://pre-commit.ci

commit 76bbef449b88bbd74fb5cca3b5293337a624ef03
Author: Cheukting <cheukting.ho@gmail.com>
Date:   Thu Jun 23 11:40:41 2022 +0800

    adding changelog

commit db82bebc5a4969e2083adcd97bdfd2a63bb17d98
Author: Cheukting <cheukting.ho@gmail.com>
Date:   Thu Jun 23 11:33:10 2022 +0800

    fall back to native when handeling to exception groups
…ceptionchain (excinfo->excinfo_, set reprcrash. Extended tests, though they're wip.
@jakkdl
Copy link
Member Author

jakkdl commented Aug 12, 2022

failing code coverage is partly for the 3.11-only test, and partly for the code written for when exceptiongroup is not installed.

@jakkdl
Copy link
Member Author

jakkdl commented Aug 12, 2022

also shoutout to @Cheukting for giving a good base to work off of. I'll add you to AUTHORS if you want :)

@Cheukting
Copy link
Contributor

Thank you @jakkdl 😊

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Two comments below; otherwise both tests and implementation look good to me. I've also confirmed that it works locally 👍

Definitely add Cheuk to the authors list though, it's important to give OSS credit (and copyright acknowledgement!) where it's due.

src/_pytest/_code/code.py Outdated Show resolved Hide resolved
src/_pytest/_code/code.py Outdated Show resolved Hide resolved
@Zac-HD
Copy link
Member

Zac-HD commented Aug 12, 2022

Tagging @RonnyPfannschmidt for review too because I'm pretty close to the whole ExceptionGroup project, between PEP-678 and implementing support in Hypothesis.

…irectly defining BaseExceptionGroup, added block comment, added match line for inner exception, changked mark.skipif to importorskip to not need top-level import, changed tox.ini a bit - only uncovered should now be py37 without exceptiongroup, due to hypothesis
@jakkdl
Copy link
Member Author

jakkdl commented Aug 13, 2022

Fixed comments, although Cheukting is already in AUTHORS 😅 Also changed the tests a little bit.
Adding exceptiongroup to the pre-commit-config is just for line 64, from exceptiongroup import BaseExceptionGroup, so maybe simpler to just #type: ignore that line instead.

I managed to figure out how to add a tox testenv to test py37 without exceptiongroup installed, but it's quite a hassle and doesn't seem worth it just to test ~2 lines of code.

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Looks great to me - thanks @jakkdl!

If I don't hear any objections (@nicoddemus @RonnyPfannschmidt), I'll merge this on Friday.

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.

Left a few comments, please take a look.

tox.ini Outdated Show resolved Hide resolved
src/_pytest/_code/code.py Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
src/_pytest/_code/code.py Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
@bluetech
Copy link
Member

bluetech commented Aug 16, 2022

Since exceptiongroup hasn't had a stable release yet and has had some breaking changes in the recent releases, let's ask exceptiongroup's developer:

@agronholm - pytest is considering adding a dependency on exceptiongroup for Python versions before 3.11. pytest is a popular project so the dependency will add a lot of installs of exceptiongroup. Is this OK with you? If so, do you think it's OK to depend on version exceptiongroup>=1.0.0rc8 or should we wait for a final release? Thanks.

@agronholm
Copy link

The only reason I've held back the final release is to ensure that it matches the behavior of Python 3.11 in its final release.
That said, I would like to resolve the last two open issues (agronholm/exceptiongroup#18 and agronholm/exceptiongroup#14) before making a final release.

And yes, I'm okay with pytest depending on exceptiongroup. But the latter of the open issues would need to be resolved for pytest which, IIRC, installs its own exception hook, yes?

@bluetech
Copy link
Member

But the latter of the open issues would need to be resolved for pytest which, IIRC, installs its own exception hook, yes?

pytest installs threading.excepthook and sys.unraisablehook, but not sys.excepthook. Is that OK?

@agronholm
Copy link

If it overrides those hooks, then it needs some way to manually render exception groups – something the exceptiongroup package doesn't provide yet. I'm not sure how to even provide such functionality in a way that doesn't involve patching TracebackException. It's something that needs to be researched. I would welcome any help with this.

@Zac-HD
Copy link
Member

Zac-HD commented Aug 16, 2022

If it overrides those hooks, then it needs some way to manually render exception groups – something the exceptiongroup package doesn't provide yet. I'm not sure how to even provide such functionality in a way that doesn't involve patching TracebackException.

I'd have thought that calling the traceback.format_exception() in (without loss of generality) pytest's hook function would work, since that ultimately delegates to the patched-by-exceptiongroup TracebackException methods?

And that's what Pytest's sys.unraisablehook and threading.excepthook currently do, without going through the fancier formatting that this PR works around, so I think it already works.

@agronholm
Copy link

I'd have thought that calling the traceback.format_exception() in (without loss of generality) pytest's hook function would work, since that ultimately delegates to the patched-by-exceptiongroup TracebackException methods?

You're probably right – I am no so knowledgeable about the inner workings of the traceback module. I just adapted trio's MultiError patching to exceptiongroup. To be honest, I don't even know why pytest patches those two specific hooks but NOT sys.excepthook.

@Zac-HD
Copy link
Member

Zac-HD commented Aug 16, 2022

I don't even know why pytest patches those two specific hooks but NOT sys.excepthook.

(I'm reconstructing rather than recalling, but) we don't need to register a sys.excepthook because we'll always catch the exception coming out of the user's test function. We do want to register a simple hook for the others; they just convert from the default "print to stderr" to "emit a warning containing the same text" - I guess someone had a reason to prefer that at some point, maybe warnings capture is more reliable around threads? 🤷‍♂️

@Zac-HD
Copy link
Member

Zac-HD commented Aug 16, 2022

As a high-level comment, I'd also observe that this PR seems like a strict improvement on the status quo, so I propose that we merge it (without the 3.11-exceptiongroup CI task) and move any remaining discussion to a new issue 🙂

@Zac-HD Zac-HD requested a review from nicoddemus August 16, 2022 21:35
@jakkdl
Copy link
Member Author

jakkdl commented Aug 17, 2022

Yeah not much meaningful has changed after reviews, though the code is cleaner now ✨
Removed the added toxenv from CI, and added comments to tox and pre-commit configs to avoid future confusion and make continued work with exceptiongroups easier.

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.

Looks great, thanks everyone for chiming in.

Let's please use squash when merging this. 👍 (I've added [SQUASH] to the title as a reminder).

@nicoddemus nicoddemus changed the title fallback to native traceback when handling ExceptionGroup (take 2) fallback to native traceback when handling ExceptionGroup (take 2) [SQUASH] Aug 17, 2022
@Zac-HD Zac-HD merged commit 69f2855 into pytest-dev:main Aug 17, 2022
@Zac-HD
Copy link
Member

Zac-HD commented Aug 17, 2022

🎉🎉

Thanks to everyone involved! I'm looking forward to using this 😁

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

6 participants