Skip to content

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

Merged
vstinner merged 10 commits into
python:mainfrom
vstinner:test_capi_import
Jan 17, 2025
Merged

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

Conversation

@vstinner

@vstinnervstinner commented Jan 16, 2025

Copy link
Copy Markdown
Member
  • 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.

@vstinnervstinner changed the title gh-128912: Add tests on the PyImport C APIgh-128911: Add tests on the PyImport C APIJan 16, 2025
* 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

Copy link
Copy Markdown
MemberAuthor

@vstinner

Copy link
Copy Markdown
MemberAuthor

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.
Comment threadModules/_testlimitedcapi/import.c Outdated
Comment threadModules/_testlimitedcapi/import.c
Comment threadModules/_testlimitedcapi/import.c
Comment threadModules/_testlimitedcapi/import.c Outdated
Comment threadModules/_testlimitedcapi/import.c Outdated
Comment threadModules/_testlimitedcapi/import.c Outdated
Comment threadModules/_testlimitedcapi/import.c Outdated
@vstinner

Copy link
Copy Markdown
MemberAuthor

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?

@serhiy-storchakaserhiy-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.

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.

Comment threadModules/_testlimitedcapi/import.c Outdated
Comment threadModules/_testlimitedcapi/import.c Outdated
@vstinner

Copy link
Copy Markdown
MemberAuthor

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

@serhiy-storchaka

Copy link
Copy Markdown
Member

I pushed my changes. Please review them.

It seems that I found bugs in the current code:

  1. PyImport_ExecCodeModuleObject() leaves a non-string name in sys.module after raising AttributeError.
  2. 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.

@vstinner

Copy link
Copy Markdown
MemberAuthor

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.

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

Copy link
Copy Markdown
MemberAuthor

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

I enabled auto-merge.

@vstinner
vstinner merged commit d95ba9f into python:mainJan 17, 2025
@vstinner
vstinner deleted the test_capi_import branch January 17, 2025 17:52
@serhiy-storchakaserhiy-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

Copy link
Copy Markdown

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

@miss-islington-app

Copy link
Copy Markdown

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!

@miss-islington-app

Copy link
Copy Markdown

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

@miss-islington-app

Copy link
Copy Markdown

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

vstinner added a commit to vstinner/cpython that referenced this pull request Jan 17, 2025
* 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)
@bedevere-app

Copy link
Copy Markdown

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

@bedevere-appbedevere-appBot removed the needs backport to 3.13 bugs and security fixes label Jan 17, 2025
@vstinnervstinner removed the needs backport to 3.12 only security fixes label Jan 17, 2025
vstinner added a commit to vstinner/cpython that referenced this pull request Jan 17, 2025
* 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)
vstinner added a commit that referenced this pull request Jan 17, 2025
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)
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Jan 18, 2025
…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)
vstinner added a commit that referenced this pull request Jan 19, 2025
) (#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)
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 21, 2025
* 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>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@vstinner@serhiy-storchaka