-
Notifications
You must be signed in to change notification settings - Fork 595
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
Warnings for side effects during import #3837
Merged
Merged
Changes from 4 commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
ade525a
Warnings for side effects during import
jobh 6b7c33e
Fix for python <3.11
jobh 2c861a9
Extend coverage to pytest plugins, rename and reword to fit.
jobh 6289589
Fix some tests
jobh 53b037b
Remove ignore of sideeffects warning, fix some failures
jobh 9a81b72
Format fix
jobh 61d800e
Test for pytest plugin sideeffect detector
jobh f3ea914
Improve and complete, with parts from Zac-HD/hypothesis/plugin-checks
jobh 263c044
Add the _hypothesis_globals module in setup.py, tweaks
jobh c3c77e6
Formatting and cache busting
jobh 6e0d364
Skip one test with older pytest
jobh beb143f
Fix test when pytest or other plugins emit warnings, add a no-cover
jobh cf7d3d5
Update hypothesis-python/src/hypothesis/errors.py
jobh dbd7ae9
Delay repr computation
Zac-HD File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Why would we want to always warn?
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.
That's a bit of a long story, so I'll explain. Pardon the verbosity! I agree there's nothing pytest-specific about the check for plugins that we load ourselves, but pytest plugins (and conftest) are an instance of a wider check: Side effects during initialization, where initialization is not just the initial import.
We can't warn directly about side effects during post-import initialization, but we can detect them. Even at no additional cost, since we can check whether the patching has been done - which happens on the first side-effect-inducing operation after import. And then the pytest plugin (or other test runners) can quite easily check if this is the case at start of session.
Now, it can warn that it has happened, but it can't see who did it or where, at least not easily [*]. That's where this setting comes in:
...which will cause the warning to happen, even after import is finished, and show the relevant stack trace.
[*] Yeah, we could record the stack of the first such call, but that's more effort than it's worth IMO
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.
Hm, or maybe. I guess it's easy enough to stash the stacktrace and message. Would simplify the wording.
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.
Right, there are basically two ways to go about this:
I generally prefer (2), because the stack trace from
-Werror
makes it really easy to tell when this is happening.I guess the problem is how to tell whether we're in post-import initialization, but I think there we could set a global from the earliest possible pytest setup hook, and then unset it at the end? We might still miss a few cases due to the ordering of hooks between our and other pytest plugins, but I think that applies equally to (1) for side effects triggered by pytest plugins.
At that point I suppose there's still a case for (1) to warn if we detect that there was a side-effect between finishing
import hypothesis
and delivery of the first pytest hook... but that seems concerningly prone to false alarms (c.f.testdir.runpytest_inprocess()
test changes!). Let's just skip detection in this case then?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 agree (2) would be best, and we could do that as soon as possible (I was thinking even plugin-module execution, not wait for the import hook). I haven't done this because I am concerned that there is a significant chance of something happening during that hole in coverage. But hey, maybe it's not that important compared to having a simpler best-effort variant.
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... if there is a hole, we must check and deal with that anyway to revert the patching, so we may as well warn in that case, too. I'll follow this path then.