-
Notifications
You must be signed in to change notification settings - Fork 591
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 some flaky reporting corner cases #4239
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Joachim! This looks great and I'm sure will be increasingly useful over time 😁
if isinstance(e, BaseExceptionGroup) and len(e.exceptions) == 1: | ||
# When a naked exception is implicitly wrapped in an ExceptionGroup | ||
# due to a re-raising "except*", the ExceptionGroup is constructed | ||
# in the caller stack frame (see #4183). | ||
tb = e.exceptions[0].__traceback__ | ||
else: | ||
tb = e.__traceback__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if there are multiple layers, e.g. ExceptionGroup("outer", ExceptionGroup("inner", [ValueError()]))
? Should we use while
to unwrap them all?
(I'd appreciate a code comment about this so we're less baffled when revisiting in a few years for something even more complicated 😅)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep done.
The "missing stack frame" on implicit wrapping of naked exceptions in except*
— I won't call it a cpython bug, as it doesn't contradict any documentation I've seen, but IMO it's surprising behaviour. Do you think it would be worth raising a concern upstream @Zac-HD ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think it's worth opening an issue to say something like "we were surprised by this behavior in $PR, and think that $proposal would be less surprising and more useful. If you agree, up to you whether this treated as a bugfix, or a minor change for 3.14"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too late for a code comment in this PR, but FYI @Zac-HD : Cpython will have this fixed in 3.12+, so the workaround can be removed once 3.11 is EOL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I've stuck a note to that effect onto #4252.
Test failures are at least in part my fault - I'll look into this and come back with a fix.
|
#4240 should address failures, and in the meantime we can retry jobs that fail because of it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again, all!
Thanks, @jobh for patching this quickly! |
Closes #4183
Closes #4228
Two corner cases in reporting of flaky errors.
One of them is a straightforward missing handling of flakiness at
mark_interesting
time.The other deals with a surprising aspect of how a naked exception is wrapped in an
ExceptionGroup
when caught and reraised byexcept*
— the group's traceback points not to where the naked exception is raised, nor to the location of theexcept*
block, but instead to the caller one frame above theexcept*
:I'm not sure if this is consistent between python implementations, so the final commit adds a check. Will revert once CI results are in.