Skip to content

Conversation

@vstinner
Copy link
Member

@vstinnervstinner commented Jan 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.

@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
MemberAuthor

@vstinner
Copy link
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.
@vstinner
Copy link
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?

Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

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.

@vstinner
Copy link
MemberAuthor

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

@serhiy-storchaka
Copy link
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
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.

@vstinnervstinner enabled auto-merge (squash) January 17, 2025 17:31
@vstinner
Copy link
MemberAuthor

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

I enabled auto-merge.

@vstinnervstinner merged commit d95ba9f into python:mainJan 17, 2025
39 checks passed
@vstinnervstinner 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

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

@miss-islington-app
Copy link

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

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

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

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