-
Notifications
You must be signed in to change notification settings - Fork 9
Fail test suite when GIL is enabled dynamically at runtime #102
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
Fail test suite when GIL is enabled dynamically at runtime #102
Conversation
src/pytest_run_parallel/plugin.py
Outdated
| if self.warn_gil_enabled: | ||
| warnings.warn(GIL_ENABLED_ERROR_TEXT) | ||
| else: | ||
| pytest.exit(GIL_ENABLED_ERROR_TEXT, returncode=1) |
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.
This will work, but I have a slightly different approach I was thinking about today.
What do you think about instead of checking at the end of the session, we check after collection is finished and then check after every individual test. If it fails during collection, we say "GIL re-enabled during collection", if it's re-enabled in a test we say "GIL re-enabled during execution of test_foo". That way people get an indication exactly when the GIL is re-enabled.
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, that makes sense to me as well. I was thinking the warning, that indicates what the module that enabled the GIL was, would be enough but that's lost in the case of a pytest.exit. Let me try to do that in this PR.
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.
This is a bit more complicated than I initially thought. I'll work on it tomorrow and hopefully do a 0.6 release as well.
I think you'd need to create a tiny C extension with e.g. meson and then import it in a test. Seems like a lot of complexity - totally fine with me to punt on testing that for now. |
|
@ngoldbaum This is realy for another review. What do you think of this approach? It fails early during collection or execution phase. Collection: Execution: |
912f8c9 to
c0a5a49
Compare
c0a5a49 to
c63cd26
Compare
ngoldbaum
left a comment
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.
LGTM, let's ship it. I might poke at adding tests with a real extension if we ever have issues with this but your mocking with a warning that you raise yourself in a test is probably more than sufficient.
| ) | ||
|
|
||
| GIL_WARNING_MESSAGE_CONTENT = re.compile( | ||
| r"The global interpreter lock \(GIL\) has been enabled to load module '(?P<module>[^']*)'" |
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.
gemini gave me this explanation for (?P<module>[^']*):
Capture any sequence of zero or more characters that are not a single quote, and name this captured group 'module'.
There might be a better way to do that but this also seems to work... I kind of hate regexes...
@ngoldbaum We can get this into 0.6 as well.
Only problem is I don't really know how to test this. I checked manually with a custom package with a compiled extension and the test suite indeed failed.