Skip to content

Conversation

jakkdl
Copy link
Member

@jakkdl jakkdl commented Nov 18, 2024

fixes #12943

As ExceptionInfo still doesn't have proper support for exception groups this continues upon the hacky solution from #10209, modifying the reprcrash as well.

I find this solution ... incredibly hacky and ugly, and am not a fan that n==1 gets handled but not n==2. But conceptually it just gets really tricky to figure out what info to strip, and how to show that. If we're going with stripping structure & group messages we could extend this solution to something like [in ExceptionGroup]: ValueError("foo"), TypeError("bar").

EDIT: less hacky now that I moved the code to ExceptionInfo.exconly

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Nov 18, 2024
@jakkdl jakkdl marked this pull request as ready for review November 19, 2024 16:22
@jakkdl jakkdl requested a review from Zac-HD November 19, 2024 16:28
and isinstance(self.value, BaseExceptionGroup)
and (subexc := _get_single_subexc(self.value)) is not None
):
return f"[in {type(self.value).__name__}] {subexc!r}"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return f"[in {type(self.value).__name__}] {subexc!r}"
return f"{subexc!r} [single exception in {type(self.value).__name__}]"

as discussed in the issue

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed, but noted my disagreement in the issue :)

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.

Thanks @jakkdl!

I really appreciate your writeup in #12943 (comment), but continue to prefer this style for a few reasons:

  • when the nodeid is long and we can get truncation of this short message, I want to keep the leaf-type as the first part of this message. Generally users who are seeing ExceptionGroups know they're possible, so I'm not too worried about truncating that note away in such cases
  • single-leaf groups are qualitatively simpler to think about than multi-leaf groups - the structure of the latter often matters, for example, while the former are basically just extra structure in the traceback - and so discarding the more complex structure feels a bit worse
    • I think we'll probably want to do something better for these cases too at some point, but I don't yet know what and would prefer to wait until we have a proposal that users seem enthusiastic about.

I'm happy with this PR as-is since it seems to me like a clear improvement on the status quo; if nobody else has feedback I'll merge in a few days 🙂

@Zac-HD Zac-HD merged commit acf1303 into pytest-dev:main Nov 25, 2024
29 checks passed
@jakkdl jakkdl deleted the short_info_excgroup branch November 26, 2024 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided (automation) changelog entry is part of PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

=== short test summary info === messages are not useful for ExceptionGroups
2 participants