-
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
Make Flaky into an ExceptionGroup #4043
Conversation
fec4b0d
to
342c677
Compare
342c677
to
522274d
Compare
Looks pretty rare, with the majority being schemathesis: https://github.com/search?q=%22except+Flaky%3A%22+OR+%22except+Flaky+as%22+language%3Apython&type=code That said, I definitely have some local research code that catches Flaky...but I knew I was flying close to the sun with that one. |
Seems rare, but while I share your preference for the flat version I think we should continue to support |
So be it 😇 Ready for review then! No rush - I'm starting my summer vacation today, and will be on and off depending on weather. Hopefully mostly off 😁 |
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.
I want to play around and compare the before-and-after reporting for some examples before we merge, but this looks great so far 😁
self._interesting_origins = interesting_origins | ||
|
||
|
||
class FlakyData(Flaky): |
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.
I'm concerned that users will think we're reporting an internal error rather than something they need to fix, but I can't think of any name that wouldn't 😕
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.
Naming is hard! I went through FlakyGeneration plus some variants of InconsistentX and IndetermisticX, and not at all sure I settled for the best.
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.
FlakyStrategy? And rename the other one to FlakyExecution, for symmetry?
Edit: maybe it's bad form to like my own comment, but I think these names would better. FlakyExecution and FlakyStrategy.
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.
I do like FlakyReplay
as a concise and evocative name; for the other I've gone with FlakyStrategyDefinition
which is more verbose but does seem to point to the thing-which-is-flaky.
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.
self._interesting_origins = interesting_origins | ||
|
||
|
||
class FlakyData(Flaky): |
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.
I do like FlakyReplay
as a concise and evocative name; for the other I've gone with FlakyStrategyDefinition
which is more verbose but does seem to point to the thing-which-is-flaky.
This PR makes
Flaky
into anExceptionGroup
to improve presentation of flaky failures. Closes #4040.As usual, There Were Complications. Basically because an
ExceptionGroup
is not allowed to have an empty group; so a straightforward replacement is not possible.On the plus side, I think the reworked
Flaky
is more expressive by differentiating two different kinds of flakiness (replay inconsistencies vs. inconsistent data generation), and it does improve reporting of flaky errors detected by the engine by adding full exceptions on the way out.A simpler flat alternative is this, with the only drawback being that
InconsistentData
(renamed from FlakyData) is no longerFlaky
. I think I actually prefer this version, but I don't know how you feel about this drawback — is it even acceptable for a minor revision, do users ever catchFlaky
? — so the implemented version is the one above.