Fixed the issue of causes KeyError when using the parameter --import-mode=importlib in pytest>=8.2 . (#12592)

[dongfangtianyu]

closes #12592

Reproduction Steps:

1. Create a namespace package in a non-root directory and include a test file.

2. Create a directory with the same name in that location.

Directory Structure:

>tree ./a
a
└── b
    └── c
        ├── c
        └── test_demo.py

Execution Results：

>pytest --import-mode=importlib

==================== test session starts ===================
platform win32 -- Python 3.12.0, pytest-8.3.2, pluggy-1.5.0
rootdir: D:\app
plugins: allure-pytest-2.13.5
collected 0 items / 1 error

========================== ERRORS ==========================
____________ ERROR collecting a/b/c/test_demo.py ____________
<frozen importlib._bootstrap_external>:1533: in find_spec
    ???
<frozen importlib._bootstrap_external>:1334: in __init__
    ???
<frozen importlib._bootstrap_external>:1350: in _get_parent_path
    ???
E   KeyError: 'a.b'

=================== short test summary info =================
ERROR a/b/c/test_demo.py - KeyError: 'a.b'
!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!
========================= 1 error in 0.10s ===================

Through tracing, it was discovered that the error occurs during the second call to pathlib._import_module_using_spec.

_import_module_using_spec  # Import module
 -> _import_module_using_spec # Import parent module
   -> PathFinder.find_spec
     -> _NamespacePath.__init__
       -> NamespacePath._get_parent_path
         -> sys.modules[parent_module_name]  # Retrieve ancestor module from sys.modules
           -> KeyError

---

This is the entire call process when the exception occurs.

In simple terms, when executing PathFinder.find_spec('a.b.c', ['/app/a/b/c']):

1. Python assumes that we are trying to import a module named c (the tail module).

2. At this point, there happens to be a directory c in /app/a/b/c, which is then treated as a namespace.

3. According to convention, its parent module a.b should already exist in sys.modules, and it is accessed directly using the item method.

4. However, when pytest uses the importlib mode, it doesn't follow this convention, leading to a KeyError.

So, from the observable behavior, it seems to be an upstream issue.

However, from a deeper perspective, there might also be a problem with pytest's handling of parent module imports.

I noticed that _import_module_using_spec attempts to follow the convention of importing the parent module, but the error occurs precisely during this process:
https://github.com/pytest-dev/pytest/blob/main/src/_pytest/pathlib.py#L634-L656

I searched for the usage of 'importlib', and the example provided in the Python documentation can import of any module or namespace package in this case:
https://docs.python.org/3/library/importlib.html#approximating-importlib-import-module.

Unlike _import_module_using_spec, the example function in the documentation first ensures that the necessary parent modules are present in sys.modules, and then imports the target module (or target namespace package).

---

Based on this, I attempted to modify the execution process in _import_module_using_spec, moving the import of the parent module to after insert_missing_modules() to avoid a KeyError due to sys.modules not being ready.

From the test results, it works quite well, but I'm not sure if there are potential flaws or better solutions (in fact, I've tried and discarded many solutions).

Welcome any further suggestions.

[nicoddemus]

Thanks @dongfangtianyu, but I'm not sure this approach is correct (although the introduced test seems to indicate it works).

Perhaps it would be helpful to submit a snippet upstream that uses PathFinder.find_spec and causes it to raise KeyError to obtain advice.

[dongfangtianyu]

Thank you for your response @nicoddemus . I’m also unsure if this approach is suitable. The main purpose of this PR is to request a review and discussion from the maintainers.

I’ve tried various fixes, but all have failed. During this process, I’ve often thought that this issue should be fixed upstream.
After tracking the behavior of the example functions from the Python documentation in this use case, for now, I believe a more elegant fix can be applied here in pytest.

Since I’m not yet familiar with the lower-level aspects of Python, I’ve been avoiding creating an issue in the Python repository. However, you’re right. I will prepare a sample code later and seek advice from upstream.

[dongfangtianyu]

Hi @nicoddemus , can you please re-review the code?
This time follow the python documentation example to refactor the function.

Judging from the results, the effect of this modification:

• Avoid KeyError exceptions
• All parent modules are actually imported and executed
• Compatible with the case where the parent module is a namespace package
• Improved the variable naming and value assignment logic during iteration for better clarity.

Welcome any further suggestions.

[nicoddemus]

Hi @dongfangtianyu just to let you know I've been busy, but I will review this ASAP. 👍

[nicoddemus]

Hi @dongfangtianyu, finally managed to find the time to look at this carefully, sorry for the long delay!

Everything makes sense according to the docs and the tests are there to prove it. Took the liberty of tweaking some comments/docs/changelog and simplify the regression test to better reflect the issue, other than that this looks great!

Again, thanks a lot for all the effort put here, we really appreciate it!

[nicoddemus]

I will merge this in the next few days.

[patchback]

Backport to 8.3.x: 💚 backport PR created

✅ Backport PR branch: patchback/backports/8.3.x/6486c3f3a858a0c8043f5c3f7c24297b82a0abe4/pr-12752

Backported as #12843

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.