Skip to content

Fix some flaky reporting corner cases#4239

Merged
jobh merged 3 commits intoHypothesisWorks:masterfrom
jobh:exception-fixes
Jan 13, 2025
Merged

Fix some flaky reporting corner cases#4239
jobh merged 3 commits intoHypothesisWorks:masterfrom
jobh:exception-fixes

Conversation

@jobh
Copy link
Copy Markdown
Contributor

@jobh jobh commented Jan 12, 2025

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 by except* — the group's traceback points not to where the naked exception is raised, nor to the location of the except* block, but instead to the caller one frame above the except*:

def wrapped():
    try:
        raise Exception  # innermost frame of EceptionGroup.exceptions[0]
    except* Exception:
        raise

def naked():
    raise Exception  # innermost frame of Exception

if __name__ == "__main__":
    import traceback
    try:
        naked()
    except Exception as e:
        print("naked:", traceback.format_tb(e.__traceback__)[-1])
    try:
        wrapped()  # innermost frame of ExceptionGroup
    except Exception as e:
        print("wrapper:", traceback.format_tb(e.__traceback__)[-1])
        print("wrapped:", traceback.format_tb(e.exceptions[0].__traceback__)[-1])

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.

Zac-HD
Zac-HD reviewed Jan 12, 2025
View reviewed changes
Copy link
Copy Markdown
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 Joachim! This looks great and I'm sure will be increasingly useful over time 😁

hypothesis-python/src/hypothesis/core.py
Comment on lines +1126 to +1137
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__
Copy link
Copy Markdown
Member

@Zac-HD Zac-HD Jan 12, 2025

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 😅)

Copy link
Copy Markdown
Contributor Author

@jobh jobh Jan 12, 2025

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 ?

Copy link
Copy Markdown
Member

@Zac-HD Zac-HD Jan 13, 2025

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"

Copy link
Copy Markdown
Contributor Author

@jobh jobh Jan 25, 2025

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.

Copy link
Copy Markdown
Member

@Liam-DeVoe Liam-DeVoe Jan 25, 2025

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.

@Liam-DeVoe
Copy link
Copy Markdown
Member

Liam-DeVoe commented Jan 12, 2025

Test failures are at least in part my fault - I'll look into this and come back with a fix.

FAILED tests/conjecture/test_shrinker.py::test_redistribute_numeric_pairs - KeyError: 'stopped-because'

@Liam-DeVoe Liam-DeVoe mentioned this pull request Jan 12, 2025
Merged
@Liam-DeVoe
Copy link
Copy Markdown
Member

Liam-DeVoe commented Jan 12, 2025

#4240 should address failures, and in the meantime we can retry jobs that fail because of it.

Zac-HD
Zac-HD approved these changes Jan 13, 2025
View reviewed changes
Copy link
Copy Markdown
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 again, all!

@jobh jobh merged commit b16be2e into HypothesisWorks:master Jan 13, 2025
@jobh jobh mentioned this pull request Jan 13, 2025
Closed
@carterbox
Copy link
Copy Markdown

carterbox commented Jan 13, 2025

Thanks, @jobh for patching this quickly!

@jobh jobh deleted the exception-fixes branch January 19, 2025 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Liam-DeVoe Liam-DeVoe Liam-DeVoe left review comments

@Zac-HD Zac-HD Zac-HD approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Avoid raising FlakyReplay to users when multiple bugs are found Bizarre quirk with how Hypothesis formats the error message for falsifying examples

4 participants

@jobh @Liam-DeVoe @carterbox @Zac-HD