Skip to content

gh-110529: Guard _testcapi imports in tests#110530

Closed
sobolevn wants to merge 7 commits into
python:mainfrom
sobolevn:issue-110529
Closed

gh-110529: Guard _testcapi imports in tests#110530
sobolevn wants to merge 7 commits into
python:mainfrom
sobolevn:issue-110529

Conversation

@sobolevn

@sobolevn sobolevn commented Oct 8, 2023

Copy link
Copy Markdown
Member

import _testcapi
from test.support import import_helper

_testcapi = import_helper.import_module('_testcapi')

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Member Author

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

@serhiy-storchaka

Copy link
Copy Markdown
Member

There is a test in test_capi.test_unicode that do not use _testcapi, but uses ctypes. There may be other such tests. I do not know how important is the ability to run these tests when _testcapi is not available.

@sobolevn

sobolevn commented Oct 9, 2023

Copy link
Copy Markdown
Member Author

test_unicode does use _testcapi:

try:
import _testcapi
except ImportError:
_testcapi = None
try:
import _testinternalcapi
except ImportError:
_testinternalcapi = None
NULL = None
class Str(str):
pass
class CAPITest(unittest.TestCase):
@support.cpython_only
@unittest.skipIf(_testcapi is None, 'need _testcapi module')
def test_new(self):

But, not in all tests, here's the result without _testcapi module:

Total duration: 53 ms
Total tests: run=2 skipped=45
Total test files: run=1/1
Result: SUCCESS

Only two tests are executed:

  1. @support.cpython_only
    @unittest.skipIf(_testinternalcapi is None, 'need _testinternalcapi module')
    def test_transform_decimal_and_space(self):
    """Test _PyUnicode_TransformDecimalAndSpaceToASCII()"""
    from _testinternalcapi import _PyUnicode_TransformDecimalAndSpaceToASCII as transform_decimal
    (which requires _testinternalcapi, unrealistic to have it without _testcapi)
  2. def test_from_format(self):
    """Test PyUnicode_FromFormat()"""
    # Length modifiers "j" and "t" are not tested here because ctypes does
    # not expose types for intmax_t and ptrdiff_t.
    # _testcapi.test_string_from_format() has a wider coverage of all
    # formats.
    import_helper.import_module('ctypes')
    from ctypes import (
    c_char_p,
    pythonapi, py_object, sizeof,
    c_int, c_long, c_longlong, c_ssize_t,
    c_uint, c_ulong, c_ulonglong, c_size_t, c_void_p,
    c_wchar, c_wchar_p)
    name = "PyUnicode_FromFormat"
    _PyUnicode_FromFormat = getattr(pythonapi, name)
    _PyUnicode_FromFormat.argtypes = (c_char_p,)
    _PyUnicode_FromFormat.restype = py_object
    which uses ctypes as was mentioned in gh-110529: Guard _testcapi imports in tests #110530 (comment)

@sobolevn

sobolevn commented Oct 9, 2023

Copy link
Copy Markdown
Member Author

Running all test_capi tests without _testcapi: ./configure --with-pydebug --disable-test-modules

Shows that only a single test is executed:

...

Test PyUnicode_FromFormat() ... ok

...

Total duration: 62 ms
Total tests: run=1 skipped=59
Total test files: run=1/1
Result: SUCCESS

So, I guess we can move it to test_str or to test_ctypes, keep cpython_only mark and additionally skip the whole directory if _testcapi is not available.

Btw, without this patch tests fail with:

Traceback (most recent call last):
  File "/Users/sobolev/Desktop/cpython/Lib/unittest/loader.py", line 394, in _find_test_path
    module = self._get_module_from_name(name)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/sobolev/Desktop/cpython/Lib/unittest/loader.py", line 337, in _get_module_from_name
    __import__(name)
  File "/Users/sobolev/Desktop/cpython/Lib/test/test_capi/test_abstract.py", line 3, in <module>
    import _testcapi
ModuleNotFoundError: No module named '_testcapi'

Failed to import test module: test.test_capi.test_dict
Traceback (most recent call last):
  File "/Users/sobolev/Desktop/cpython/Lib/unittest/loader.py", line 394, in _find_test_path
    module = self._get_module_from_name(name)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/sobolev/Desktop/cpython/Lib/unittest/loader.py", line 337, in _get_module_from_name
    __import__(name)
  File "/Users/sobolev/Desktop/cpython/Lib/test/test_capi/test_dict.py", line 4, in <module>
    import _testcapi
ModuleNotFoundError: No module named '_testcapi'

test test_capi crashed -- Traceback (most recent call last):
  File "/Users/sobolev/Desktop/cpython/Lib/test/libregrtest/single.py", line 138, in _runtest_env_changed_exc
    _load_run_test(result, runtests)
  File "/Users/sobolev/Desktop/cpython/Lib/test/libregrtest/single.py", line 95, in _load_run_test
    regrtest_runner(result, test_func, runtests)
  File "/Users/sobolev/Desktop/cpython/Lib/test/libregrtest/single.py", line 48, in regrtest_runner
    test_result = test_func()
                  ^^^^^^^^^^^
  File "/Users/sobolev/Desktop/cpython/Lib/test/libregrtest/single.py", line 92, in test_func
    return run_unittest(test_mod)
           ^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/sobolev/Desktop/cpython/Lib/test/libregrtest/single.py", line 35, in run_unittest
    raise Exception("errors while loading tests")
Exception: errors while loading tests

test_capi failed (uncaught exception)

== Tests result: FAILURE ==

1 test failed:
    test_capi

@sobolevn

sobolevn commented Oct 9, 2023

Copy link
Copy Markdown
Member Author

With my latest commit and without _testcapi module:

» ./python.exe -m test test_capi -v                
== CPython 3.13.0a0 (heads/issue-110525:9feda284557, Oct 9 2023, 10:15:28) [Clang 15.0.0 (clang-1500.0.40.1)]
== macOS-14.0-arm64-arm-64bit little-endian
== Python build: debug
== cwd: /Users/sobolev/Desktop/cpython/build/test_python_worker_65250æ
== CPU count: 12
== encodings: locale=UTF-8 FS=utf-8
== resources: all test resources are disabled, use -u option to unskip tests

Using random seed 2143053226
0:00:00 load avg: 1.56 Run 1 test sequentially
0:00:00 load avg: 1.56 [1/1] test_capi
test_capi skipped -- No module named '_testcapi'
test_capi skipped

== Tests result: SUCCESS ==

1 test skipped:
    test_capi

Total duration: 27 ms
Total tests: run=0
Total test files: run=1/1 skipped=1
Result: SUCCESS

@sobolevn

sobolevn commented Oct 9, 2023

Copy link
Copy Markdown
Member Author

This was unexpected:

0:09:10 load avg: 2.96 Re-running 1 failed tests in verbose mode in subprocesses
0:09:10 load avg: 2.96 Run 1 test in parallel using 1 worker process (timeout: 10 min, worker timeout: 15 min)
0:09:11 load avg: 2.96 [1/1/1] test_capi failed (3 failures)
Re-running test_capi in verbose mode (matching: test_overridden_setting_extensions_subinterp_check, test_overridden_setting_extensions_subinterp_check, test_overridden_setting_extensions_subinterp_check)
test_overridden_setting_extensions_subinterp_check (test.test_capi.test_misc.SubinterpreterTest.test_overridden_setting_extensions_subinterp_check)
PyInterpreterConfig.check_multi_interp_extensions can be overridden ... Traceback (most recent call last):
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/support/import_helper.py", line 78, in import_module
    return importlib.import_module(name)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/importlib/__init__.py", line 88, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen importlib._bootstrap>", line 1381, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1354, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1325, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 915, in _load_unlocked
  File "<frozen importlib._bootstrap>", line 813, in module_from_spec
  File "<frozen importlib._bootstrap_external>", line 1302, in create_module
  File "<frozen importlib._bootstrap>", line 488, in _call_with_frames_removed
ImportError: module _testcapi does not support loading in subinterpreters

@sobolevn

sobolevn commented Oct 9, 2023

Copy link
Copy Markdown
Member Author

This happens because test_overridden_setting_extensions_subinterp_check imports test.test_capi.check_config, which cannot be imported now, since we raise an exception in test_capi/__init__.py 🤔

Comment thread Lib/test/test_capi/__init__.py Outdated

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

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.

ChrisLovering added a commit to python-discord/snekbox that referenced this pull request Oct 10, 2023
Currently, these modules are required for building 3.13-dev. See python/cpython/pull/110530
ChrisLovering added a commit to python-discord/snekbox that referenced this pull request Oct 10, 2023
Currently, these modules are required for building 3.13-dev. See python/cpython/pull/110530
ChrisLovering added a commit to python-discord/snekbox that referenced this pull request Oct 10, 2023
Currently, these modules are required for building 3.13-dev. See python/cpython/pull/110530
ChrisLovering added a commit to python-discord/snekbox that referenced this pull request Oct 10, 2023
Currently, these modules are required for building 3.13-dev. See python/cpython/pull/110530
@vstinner

Copy link
Copy Markdown
Member

@sobolevn:

Test PyUnicode_FromFormat() ... ok

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.

@serhiy-storchaka:

Please return test_from_format where it was. It is better to keep all C API tests in one place.

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

@sobolevn sobolevn left a comment

Copy link
Copy Markdown
Member Author

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? :)

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

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.

@vstinner

Copy link
Copy Markdown
Member

@vstinner do we have any actionable items here? Or can we proceed? :)

I propose to only check if _testcapi module is available in Lib/test/test_capi/__init__.py, and then use regular import _testcapi. Is there a disagreement on that?

On CPython, the _testcapi module must always be present when tests are run. Maybe it should even be an error, but you can skip the test on other Python implementations. Usually, we tolerate missing ctypes, but that's different.

@sobolevn

Copy link
Copy Markdown
Member Author

Is there a disagreement on that?

I tried to do that, but it didn't work: because there's check_config.py that is used in some other tests. If we do this skip in test_capi/__init__.py, these tests will fail.

@vstinner

Copy link
Copy Markdown
Member

there's check_config.py that is used in some other tests

Oh that's strange. It should be moved to test.support if it's used by other tests.

@sobolevn

Copy link
Copy Markdown
Member Author

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:

script = textwrap.dedent(f'''
from test.test_capi.check_config import run_singlephase_check
run_singlephase_check({override}, {w})
''')
with os.fdopen(r) as stdout:
ret = support.run_in_subinterp_with_config(script, **kwargs)

So, when making all test_capi folder unimportable without _testcapi module we kill the possibility to have some helper things that are only suitable for capi itself.

How about (either):

  1. Creating support/capi/*.py (others locations are welcome) helpers? (the biggest downside is that capi stuff will not be contained in a single folder)
  2. Or changing the structure as test_capi/{utils,check_config,etc}.py and test_capi/tests where the last directory will be ignored, while test_capi.check_config will be importable
  3. Or Keeping this PR as-is (the biggest downside is that we have to remember to skip all modules inside)

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

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.

@vstinner

Copy link
Copy Markdown
Member

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?

@sobolevn

Copy link
Copy Markdown
Member Author

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

Here's how I understand this:

  1. It runs test_capi in the main process / interpreter. _testcapi is there
  2. It starts support.run_in_subinterp_with_config, which does not have _testcapi (because subiterpreters cannot use it for some reason)
  3. The test fails, because the subinterpreter can no longer import from test_capi

@vstinner

Copy link
Copy Markdown
Member

It starts support.run_in_subinterp_with_config, which does not have _testcapi (because subiterpreters cannot use it for some reason)

I suppose that _testcapi lacks Py_MOD_MULTIPLE_INTERPRETERS_SUPPORTED or Py_MOD_PER_INTERPRETER_GIL_SUPPORTED.

See own_gil and gil options of support.run_in_subinterp_with_config() and PEP 684.

It seems like it's a real issue in the test, and this work just made it more obvious, no?

@sobolevn

Copy link
Copy Markdown
Member Author

_testcapi also lacks https://peps.python.org/pep-0489/

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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?

@serhiy-storchaka

Copy link
Copy Markdown
Member

See also #111067 which also has tests that depend on _testcapi and tests that depend on ctypes. I think that the absence of one of these modules should not disable other tests that do not need it.

@github-actions

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Apr 14, 2026
@vstinner

Copy link
Copy Markdown
Member

@sobolevn: Are you still working on this old PR? If yes, you should update it, there are now conflicts. If not, I suggest closing it with its issue.

@sobolevn sobolevn closed this Apr 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting review skip news stale Stale PR or inactive for long period of time. tests Tests in the Lib/test dir

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants