Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Try to import module before creating dummy modules with 'importmode=importlib' #9681

Merged
merged 1 commit into from
Feb 14, 2022

Conversation

nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Feb 13, 2022

The dummy modules we introduce in insert_missing_modules (due to #7856 and #7859)
would cause problems if the dummy modules end up replacing modules
which could be imported normally because they are available in PYTHONPATH.

Now we attempt to first import the module via normal mechanisms, and only
introduce the dummy modules if the intermediary modules don't actually exist.

Close #9645


Original PR text, when opened as a draft, for reference:

I spent some time investigating this and narrowed it down to this function:

def insert_missing_modules(modules: Dict[str, ModuleType], module_name: str) -> None:
"""
Used by ``import_path`` to create intermediate modules when using mode=importlib.
When we want to import a module as "src.tests.test_foo" for example, we need
to create empty modules "src" and "src.tests" after inserting "src.tests.test_foo",
otherwise "src.tests.test_foo" is not importable by ``__import__``.
"""
module_parts = module_name.split(".")
while module_name:
if module_name not in modules:
module = ModuleType(
module_name,
doc="Empty module created by pytest's importmode=importlib.",
)
modules[module_name] = module
module_parts.pop(-1)
module_name = ".".join(module_parts)

which is called during import_path when importmode=importlib.

The function was introduced because of #7856 and #7859, and it did feel like a hack at the time, but since everything seemed to work fine, it was merged in.

Stepping into the debugger, I can see that the function is called with "tests.conftest", with "test.conftest" already being in sys.modules by then, then it creates this empty/dummy module for "tests".

OK the reported error makes sense: we are telling tests is a plain module, it should be a package according to the message.

I dig around a bit in the import lib resources, looking for the code which generated that error message ("is not a pacakge") to understand what importlib looked for in a module to recognize it as a package, and found this code in the standard library:

# importlib/_bootstrap.py
def _find_and_load_unlocked(name, import_):
    path = None
    parent = name.rpartition('.')[0]
    if parent:
        if parent not in sys.modules:
            _call_with_frames_removed(import_, parent)
        # Crazy side-effects!
        if name in sys.modules:
            return sys.modules[name]
        parent_module = sys.modules[parent]
        try:
            path = parent_module.__path__
        except AttributeError:
            msg = (_ERR_MSG + '; {!r} is not a package').format(name, parent)
            raise ModuleNotFoundError(msg, name=name) from None

So it seems it expects the parent module (tests in our case) to have a __path__ attribute, so I went ahead and quickly hacked this together to see how it behaves:

index def5fa94b..c105c6948 100644
--- a/src/_pytest/pathlib.py
+++ b/src/_pytest/pathlib.py
@@ -601,15 +601,20 @@ def insert_missing_modules(modules: Dict[str, ModuleType], module_name: str) ->
     otherwise "src.tests.test_foo" is not importable by ``__import__``.
     """
     module_parts = module_name.split(".")
+    first=True
     while module_name:
         if module_name not in modules:
             module = ModuleType(
                 module_name,
                 doc="Empty module created by pytest's importmode=importlib.",
             )
+            if not first:
+                module.__package__ = module_name
+                module.__path__ = module_name
             modules[module_name] = module
         module_parts.pop(-1)
         module_name = ".".join(module_parts)
+        first = False

This did not work however, and we get the exact same error.

Then I had another thought, what if we try to use __import__ first, and only if that fails we attempt generate the dummy intermediate modules?

The code in this PR follows that approach and works with the original issue and the rest of the test suite 1, but I'm opening this as a draft to gather the opinion of people more knowledgeable of the import machinery (@asottile).

If this looks good enough, I will add tests and a changelog entry.

Footnotes

  1. Except for one test but that's because we call __import__ with an empty sys.meta_path, which raises a warning, this can easily be fixed by checking sys.meta_path first before calling __import__.

@asottile
Copy link
Member

👍 yeah this seems fine -- and is fairly minimal

@nicoddemus nicoddemus changed the title WIP for #9645 (gather opinions) Try to import module before creating dummy modules with 'importmode=importlib' Feb 14, 2022
@nicoddemus nicoddemus marked this pull request as ready for review February 14, 2022 12:33
…mportlib'

The dummy modules we introduce in `insert_missing_modules` (due to pytest-dev#7856 and pytest-dev#7859)
would cause problems if the dummy modules actually end up replacing modules
which could be imported normally because they are available in `PYTHONPATH`.

Now we attempt to first import the module via normal mechanisms, and only
introduce the dummy modules if the intermediary modules don't actually exist.

Close pytest-dev#9645
Copy link
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bluenote10
Copy link

@nicoddemus Thanks for looking into this and providing a fix!

This hasn't been included into 7.0.1, has it? To weigh up work-around options: Is another bugfix release planned soon, or will it take a bit longer until the next release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants