gh-128911: Add tests on the PyImport C API by vstinner · Pull Request #128915 · python/cpython · GitHub

Add Modules/_testlimitedcapi/import.c
Add Lib/test/test_capi/test_import.py
Remove _testcapi.check_pyimport_addmodule(): tests already covered by newly added tests.

Issue: [C API] Add PyImport_ImportModuleAttr(mod_name, attr_name) helper function #128911

* Add Modules/_testlimitedcapi/import.c * Add Lib/test/test_capi/test_import.py * Remove _testcapi.check_pyimport_addmodule(): tests already covered by newly added tests.

cc @serhiy-storchaka @encukou

Importing asyncio package works if test_capi.test_import is run alone, but fails with the following error when test_capi is run:

ERROR: test_importmodule (test.test_capi.test_import.ImportTests.test_importmodule) (name='asyncio')
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/test_capi/test_import.py", line 42, in check_import_fresh_module
    module = import_module(name)
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/asyncio/__init__.py", line 25, in <module>
    __all__ = (base_events.__all__ +
               ^^^^^^^^^^^
NameError: name 'base_events' is not defined

For now, I modified the tests to not import asyncio.

Avoid the datetime module.

I will merge my PR as it is when the CI passed, to be able to add tests in my following #128912 PR.

I didn't add tests for non-UTF8 strings, nor NULL tests for PyImport_ImportModuleLevel(). For PyImport_ImportModuleLevel(), it's tricky because some arguments are ignored for level=0 and I don't know how to write tests for level > 0.

@serhiy-storchaka: Maybe you would be interested to work on follow-up PR to increase the test coverage / test more cases?

Do you allow me to commit directly in your branch? It is better to have a single clean commit in the logs than several commits that rewrite one other.

@serhiy-storchaka: Ok, you can push changes to my branch.

I pushed my changes. Please review them.

It seems that I found bugs in the current code:

PyImport_ExecCodeModuleObject() leaves a non-string name in sys.module after raising AttributeError.
There is a difference in __spec__.origin between PyImport_ExecCodeModuleWithPathnames() and PyImport_ExecCodeModuleObject() if the first pathname is NULL and the second pathname is not NULL.

I pushed my changes. Please review them.

I pushed a cleanup changes. With this cleanup, the change (additional tests) LGTM.

It seems that I found bugs in the current code

Oh, interesting.

@serhiy-storchaka: Thanks for additional tests and reviews.

I enabled auto-merge.

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

Sorry, @vstinner, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker d95ba9fa1110534b7247fa2ff12b90e930c93256 3.13

Sorry, @vstinner, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker d95ba9fa1110534b7247fa2ff12b90e930c93256 3.12

* Add Modules/_testlimitedcapi/import.c * Add Lib/test/test_capi/test_import.py * Remove _testcapi.check_pyimport_addmodule(): tests already covered by newly added tests. Co-authored-by: Serhiy Storchaka <storchaka@gmail.com> (cherry picked from commit d95ba9f)

GH-128960 is a backport of this pull request to the 3.13 branch.

* Add Modules/_testlimitedcapi/import.c * Add Lib/test/test_capi/test_import.py * Remove _testcapi.check_pyimport_addmodule(): tests already covered by newly added tests. Co-authored-by: Serhiy Storchaka <storchaka@gmail.com> (cherry picked from commit d95ba9f)

gh-128911: Add tests on the PyImport C API (#128915) * Add Modules/_testlimitedcapi/import.c * Add Lib/test/test_capi/test_import.py * Remove _testcapi.check_pyimport_addmodule(): tests already covered by newly added tests. Co-authored-by: Serhiy Storchaka <storchaka@gmail.com> (cherry picked from commit d95ba9f)

…nGH-128915) (pythonGH-128960) pythongh-128911: Add tests on the PyImport C API (pythonGH-128915) * Add Modules/_testlimitedcapi/import.c * Add Lib/test/test_capi/test_import.py * Remove _testcapi.check_pyimport_addmodule(): tests already covered by newly added tests. (cherry picked from commit 34ded1a) Co-authored-by: Victor Stinner <vstinner@python.org> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com> (cherry picked from commit d95ba9f)

) (#128989) * Add Modules/_testlimitedcapi/import.c * Add Lib/test/test_capi/test_import.py * Remove _testcapi.check_pyimport_addmodule(): tests already covered by newly added tests. (cherry picked from commit 34ded1a) Co-authored-by: Victor Stinner <vstinner@python.org> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com> (cherry picked from commit d95ba9f)

* Add Modules/_testlimitedcapi/import.c * Add Lib/test/test_capi/test_import.py * Remove _testcapi.check_pyimport_addmodule(): tests already covered by newly added tests. Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

vstinner added the skip news label Jan 16, 2025

vstinner requested review from brettcannon, ericsnowcurrently, erlend-aasland, ncoghlan and warsaw as code owners January 16, 2025 12:46

bedevere-app Bot added the awaiting core review label Jan 16, 2025

vstinner force-pushed the test_capi_import branch from 3a83056 to 1d8342b Compare January 16, 2025 12:47

vstinner changed the title gh-128912: Add tests on the PyImport C API gh-128911: Add tests on the PyImport C API Jan 16, 2025

bedevere-app Bot mentioned this pull request Jan 16, 2025

[C API] Add PyImport_ImportModuleAttr(mod_name, attr_name) helper function #128911

Closed

pythongh-128911 : Add tests on the PyImport C API

214b751

* Add Modules/_testlimitedcapi/import.c * Add Lib/test/test_capi/test_import.py * Remove _testcapi.check_pyimport_addmodule(): tests already covered by newly added tests.

vstinner force-pushed the test_capi_import branch from 8830cc1 to 214b751 Compare January 16, 2025 12:52

Avoid importing asyncio package

482471f

Update Windows build

fc2f8cd

vstinner requested a review from a team as a code owner January 16, 2025 13:24

vstinner mentioned this pull request Jan 16, 2025

gh-128911: Add PyImport_ImportModuleAttr() function #128912

Merged

Add tests

33b8507

Avoid the datetime module.

vstinner force-pushed the test_capi_import branch from f027ac3 to 33b8507 Compare January 17, 2025 10:02

Fix typo

6a7d1de

serhiy-storchaka reviewed Jan 17, 2025

View reviewed changes

vstinner added 2 commits January 17, 2025 11:37

Address Serhiy's review

08e4810

Document crashes when passing NULL

1599ab6

serhiy-storchaka reviewed Jan 17, 2025

View reviewed changes

Comment thread Modules/_testlimitedcapi/import.c Outdated

Comment thread Modules/_testlimitedcapi/import.c Outdated

PyImport_GetModule() returns KeyError, instead of raising KeyError

b5b8558

Test more corner cases and errors.

e5ad39a

Cleanup

e4c7ff7

vstinner enabled auto-merge (squash) January 17, 2025 17:31

vstinner merged commit d95ba9f into python:main Jan 17, 2025

vstinner deleted the test_capi_import branch January 17, 2025 17:52

bedevere-app Bot removed the awaiting core review label Jan 17, 2025

serhiy-storchaka added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Jan 17, 2025

miss-islington-app Bot assigned vstinner Jan 17, 2025

bedevere-app Bot removed the needs backport to 3.13 bugs and security fixes label Jan 17, 2025

vstinner removed the needs backport to 3.12 only security fixes label Jan 17, 2025

gh-128911: Add tests on the PyImport C API by vstinner · Pull Request #128915 · python/cpython · GitHub

Uh oh!

Uh oh!

gh-128911: Add tests on the PyImport C API - #128915

gh-128911: Add tests on the PyImport C API#128915vstinner merged 10 commits intopython:mainpython/cpython:mainfrom vstinner:test_capi_importvstinner/cpython:test_capi_importCopy head branch name to clipboard

Conversation

vstinner commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Jan 16, 2025

Uh oh!

vstinner commented Jan 16, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vstinner commented Jan 17, 2025

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vstinner commented Jan 17, 2025

Uh oh!

serhiy-storchaka commented Jan 17, 2025

Uh oh!

vstinner commented Jan 17, 2025

Uh oh!

vstinner commented Jan 17, 2025

Uh oh!

miss-islington-app Bot commented Jan 17, 2025

Uh oh!

miss-islington-app Bot commented Jan 17, 2025

Uh oh!

miss-islington-app Bot commented Jan 17, 2025

Uh oh!

miss-islington-app Bot commented Jan 17, 2025

Uh oh!

bedevere-app Bot commented Jan 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gh-128911: Add tests on the PyImport C API#128915
vstinner merged 10 commits into
python:mainfrom
vstinner:test_capi_import

vstinner commented Jan 16, 2025 •
edited
Loading