-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
gh-110529: Guard _testcapi
imports in tests
#110530
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
base: main
Are you sure you want to change the base?
Conversation
import _testcapi | ||
from test.support import import_helper | ||
|
||
_testcapi = import_helper.import_module('_testcapi') |
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 think that such check should be done in Lib/test/test_capi/__init__.py
. I don't think that it's worth to attempt to run any of sub-tests if _testcapi cannot be imported.
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 don't agree that it should only be done there, because people can run only a single test file. In case of ./python.exe Lib/test/test_capi/test_WHATEVER.py
, this test must also be skipped correctly.
However, I think that we can also check this in test_capi/__init__.py
just to speed up test runs without _testcapi
There is a test in |
cpython/Lib/test/test_capi/test_unicode.py Lines 6 to 26 in 0df772f
But, not in all tests, here's the result without
Only two tests are executed:
|
Running all Shows that only a single test is executed:
So, I guess we can move it to Btw, without this patch tests fail with:
|
With my latest commit and without
|
This was unexpected:
|
This happens because |
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.
Please return test_from_format where it was. It is better to keep all C API tests in one place.
The rest LGTM. It is a small change and it is easier to go this way.
Currently, these modules are required for building 3.13-dev. See python/cpython/pull/110530
Currently, these modules are required for building 3.13-dev. See python/cpython/pull/110530
Currently, these modules are required for building 3.13-dev. See python/cpython/pull/110530
Currently, these modules are required for building 3.13-dev. See python/cpython/pull/110530
It's suspicious that this test is implemented with ctypes, and not _testcapi. I think that I wrote it long time ago, it should be reimplemented in _testcapi (the thin wrapper to the C API) :-) The problem is how to encode variadic arguments :-( ctypes make it easy for such quick test. Maybe ctypes is just fine. I don't think that it's worth it to still allow running test_capi if _testcapi is missing, just for this test.
One day, I wanted to make test_capi empty, move all tests to their test_xxx companion test, like move PyUnicode C API tests to test_str. But the ship has sailed, it's now too late :-) |
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.
@vstinner do we have any actionable items here? Or can we proceed? :)
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.
If @vstinner is fine with skipping a test for PyUnicode_FromFormat()
on platforms without _testcapi
, then LGTM.
I propose to only check if _testcapi module is available in On CPython, the |
I tried to do that, but it didn't work: because there's |
Oh that's strange. It should be moved to test.support if it's used by other tests. |
Here's the offending test I am talking about: cpython/Lib/test/test_capi/test_misc.py Lines 1972 to 1977 in 8f07b6e
So, when making all How about (either):
|
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.
Changes in test_dict and test_abstract LGTM. Changes in test_unicode are more controversial and not actually needed.
I don't get how it's an issue that cpython/Lib/test/test_capi/test_misc.py runs a subprocess which imports test_capi. The test should not be run if _testcapi extension is not available, since I propose to skip the whole test_capi package. No? |
Here's how I understand this:
|
I suppose that _testcapi lacks Py_MOD_MULTIPLE_INTERPRETERS_SUPPORTED or Py_MOD_PER_INTERPRETER_GIL_SUPPORTED. See own_gil and gil options of It seems like it's a real issue in the test, and this work just made it more obvious, no? |
|
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.
The situation is way more complicated than what I thought. I have doubts on test_abstract.py and test_dict.py changes, but test_unicode.py changes LGTM.
Maybe restrict your PR to test_unicode.py?
See also #111067 which also has tests that depend on |
test_dict
andtest_abstract
oftest_capi
unconditionally import_testcapi
module #110529