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

Improve internal warning tests #4023

Merged
merged 8 commits into from
Jul 11, 2024

Conversation

jobh
Copy link
Contributor

@jobh jobh commented Jun 27, 2024

Fixes #4021

When a warning is raised (due to -Werror) during test execution, it may cause the test to raise Flaky from StopTest. This makes the error message less informative.

This PR makes HypothesisWarning fatal, not only HypothesisDeprecationWarning. Because the warnings are meant to be seen.

@jobh jobh force-pushed the check-raising-warnings branch from f92ac92 to 102fcae Compare June 27, 2024 06:43
@jobh jobh force-pushed the check-raising-warnings branch from 102fcae to c87ed08 Compare June 27, 2024 06:50
@jobh

This comment was marked as off-topic.

@jobh jobh force-pushed the check-raising-warnings branch from 4b1e688 to 5f8de06 Compare July 9, 2024 12:50
@jobh jobh force-pushed the check-raising-warnings branch from 5f8de06 to f777996 Compare July 9, 2024 13:09
@jobh jobh marked this pull request as ready for review July 9, 2024 13:53
hypothesis-python/tests/common/utils.py Show resolved Hide resolved
Comment on lines 1064 to 1065
except (
HypothesisDeprecationWarning,
HypothesisWarning,
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. I wonder if it would be better to just remove this, so that all HypothesisWarnings would be treated as an ordinary test failure in the next clause down?

If you don't see a substantial downside to that, let's try it out - I'm guessing that we originally treated the deprecation warning as a hard failure just to save on the time spent shrinking and never thought it through again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is (was) that all we saw in that case was a Flaky with a StopTest cause; no trace of the original HypothesisWarning.

The latest revision fixes this, and then the line can be removed.

Side note: Flaky can be caused by one or two different failures; do you think it would make sense to change it into an ExceptionGroup for better reporting?

Copy link
Member

Choose a reason for hiding this comment

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

Ooh, yeah, defining it as class Flaky(BaseExceptionGroup, HypothesisException) would be lovely.

Copy link
Contributor Author

@jobh jobh Jul 11, 2024

Choose a reason for hiding this comment

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

Nice! To clarify, I'm not planning ExceptionGroup work in this PR, but I recorded it in a new issue to remember. This PR is complete, pending review comments and CI.

@jobh jobh force-pushed the check-raising-warnings branch from 67f79d5 to 16155b5 Compare July 10, 2024 12:50
@jobh jobh force-pushed the check-raising-warnings branch from 16155b5 to 7d9b4d4 Compare July 10, 2024 12:53
@jobh jobh force-pushed the check-raising-warnings branch 2 times, most recently from afb4847 to 69de3a3 Compare July 11, 2024 09:37
@jobh jobh force-pushed the check-raising-warnings branch from 69de3a3 to 2a0e48d Compare July 11, 2024 09:39
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 @jobh!

@Zac-HD Zac-HD merged commit 988c8ad into HypothesisWorks:master Jul 11, 2024
58 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warning from tracer causes Flaky
2 participants