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

display single contained exception in excgroups in test summary #12975

Merged
merged 7 commits into from
Nov 25, 2024

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
src/_pytest/_code/code.py Show resolved Hide resolved
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