Pytest: AssertionRewritingHook object returned from pkgutil.get_loader()

[avylove]

I have some code that examines modules and am getting test failures with pkgutil.get_loader() returning
AssertionRewritingHook objects instead of a pkgutil.ImpLoader instance.

This is with user-installed pytest 3.1.2 on Fedora 25 (Python 3.5.3)

Here's an example which demonstrates the problem.
Running pytest against it generates the error:
AttributeError: 'AssertionRewritingHook' object has no attribute 'get_filename'

import os
import pkgutil
import unittest

def get_path(modname):

    path = None
    loader = pkgutil.get_loader(modname)
    if loader:
        path = os.path.dirname(loader.get_filename())
    return path

class TestGetPath(unittest.TestCase):

    def test_load_entry_point_module(self):
        self.assertEqual(get_path(__name__), os.path.dirname(__file__))

[Erotemic]

same issue

[nicoddemus]

Hi @avylove, thanks for reporting this issue.

I took a look at the docs and I gotta be honest: supporting all the bells and whistles of the import machinery across all of 2.6 up to 3.6 is a daunting task, the documentation unfortunately is incomplete in many places and incompatible in others. Between imp, importlib, pkgutil there are tons of confusing parts.

Can you obtain the same behavior using importlib instead perhaps? This is the new way to do things, it seems. pkgutil was introduced in 2.3.

I'm just mentioning that this does not seem a simple task and I'm not sure any maintainer will have time to sort this out.

[nicoddemus]

Actually managed to obtain the same behavior using importlib (in Python 3.6):

import importlib
import os
import unittest


def get_path(modname):
    module = importlib.import_module(modname)
    return os.path.dirname(module.__file__)


class TestGetPath(unittest.TestCase):
    def test_load_entry_point_module(self):
        self.assertEqual(get_path(__name__), os.path.dirname(__file__))

[avylove]

I wouldn't say those are equivalent. In my example the module is not imported, which is why I'm using pkgutil. I actually am using importlib (and it's backport for 2.6) when I want to import the modules, but in this use case, I identify the path before importing so, if there is an error, I can prune the traceback to only parts relevant to the import rather than including the frames for my code and the import machinery. In Python 3, importlib.util.find_spec() can be used, but the module.__spec__ machinery doesn't exist in Python 2.

I think the problem is our monkey patches are running into your monkey patches. :monkey:

I was looking into the AssertionRewritingHook code a bit and it seems like it's maintaining a single importer for all modules and it has the ability to find modules (via imp.find_module()), but it doesn't preserve the information unless the file needs to be rewritten. So it would seem when I call
pkgutil.get_loader(), I'm getting an AssertionRewritingHook object that doesn't know anything about the module I asked it to get a loader for.

What if the AssertionRewritingHook cached data for all the modules it finds, either in two dictionaries or using a flag? You can still pop the module data off as they get loaded. Then a get_filename() method could be added that could return AssertionRewritingHook.modules[MODULENAME][0].co_filename or the similar and sys.modules if the file is and loaded.

An alternative would be to pass this through to the default loading framework. For example, if you determine the file needs to be rewritten, handle get_filename() within AssertionRewritingHook since you already have the data, and if not, pass it to wherever it would normally go.

[nicoddemus]

I wouldn't say those are equivalent. In my example the module is not imported, which is why I'm using pkgutil.

Oh you are right. I assumed pkgutil.get_filename() also imported the module behind the scenes.

Python 3, importlib.util.find_spec() can be used, but the module.__spec__ machinery doesn't exist in Python 2.

Makes sense. I was under the impression you only need Python >=3.5, that's why I suggested importlib.

I was looking into the AssertionRewritingHook code a bit and it seems like it's maintaining a single importer for all modules and it has the ability to find modules (via imp.find_module()), but it doesn't preserve the information unless the file needs to be rewritten. So it would seem when I call
pkgutil.get_loader(), I'm getting an AssertionRewritingHook object that doesn't know anything about the module I asked it to get a loader for.
What if the AssertionRewritingHook cached data for all the modules it finds, either in two dictionaries or using a flag? You can still pop the module data off as they get loaded. Then a get_filename() method could be added that could return AssertionRewritingHook.modules[MODULENAME][0].co_filename or the similar and sys.modules if the file is and loaded.

It is actually always the same object (it is a singleton). I'm under the impression that the design of our AssertionRewritingHook is a bit off: it acts as a finder and a loader at the same time, and the importlib design is that each module should have its own loader. And this is the problem, loader.get_filename() does not receive any parameter (as there's supposed to exist a loader for each module) so it is not possible to implement that signature under the current implementation. 😞

The problem is that it boils down the having to support 2.6 all the way up to 3.6 in the same code. We might have to rethink this design and refactor it with two implementations, the old one for Python 2 and importlib for Python 3.

[avylove]

I can sympathize with supporting 2.6 to 3.6. Most of my code does too.

I'm not sure how others would feel, but for my use case, I already have the module name, so providing it as an argument to get_filename(), while redundant, wouldn't be an issue. pkgutil.ImpLoader.get_loader() takes the module name as an optional argument, so nothing needs to change in a non-test scenario.

Maybe @Erotemic has some input.

[nicoddemus]

pkgutil.ImpLoader.get_loader() takes the module name as an optional argument, so nothing needs to change in a non-test scenario.

Indeed, that might be an interim solution at least. Ideally we would need to refactor AssertionRewritingHook, and perhaps a good time for that is when we drop 2.6 support. Or even depend on importlib, I don't know.

[briancappello]

Any update on this? My tests pass using 4.x but break with this error after upgrading to pytest 5.x

[nicoddemus]

Hi @briancappello,

Thanks for bringing this to attention. We've now dropped py<35 and are using importlib, so it should now be possible to refactor to fix this.

I can confirm the original example still fails.

cc @asottile

[asottile]

it is absolutely correct for a test module's loader to be the assertion rewriter, I think this issue is invalid as written

get_filename is not an API which is required by either spec, I'd suggest importlib's loader retrieval methods and then to inspect the origin there to get the filename in a supported way

[briancappello]

In my case the missing attribute is is_package. Interestingly, this works with only pytest installed:

# tests/__init__.py
# tests/test_it.py

import importlib
import pkgutil


def is_package(module_name):
    module = importlib.import_module(module_name)
    return pkgutil.get_loader(module).is_package(module.__name__)


def test_is_package():
    assert is_package('tests')

But when trying to test my project I am able to reproduce on a clean virtualenv. I'm not familiar with what's going on behind the scenes in pytest but it seems to me like it's mistakenly returning AssertionRewritingHook from pkgutil.get_loader instead of the expected _frozen_importlib_external.SourceFileLoader?

Full traceback:

================================================ test session starts =================================================
platform linux -- Python 3.6.9, pytest-5.1.2, py-1.8.0, pluggy-0.13.0
rootdir: /home/brian/dev/flask-unchained, inifile: setup.cfg, testpaths: tests
plugins: celery-4.2.1, flask-0.15.0, Flask-Unchained-0.7.9
collected 424 items                                                                                                  

tests/test_app_factory.py E

======================================================= ERRORS =======================================================
______________________________ ERROR at setup of TestLoadBundles.test_bundle_in_module _______________________________

request = <SubRequest '_configure_application' for <Function test_bundle_in_module>>
monkeypatch = <_pytest.monkeypatch.MonkeyPatch object at 0x7f1bbed115f8>

    @pytest.fixture(autouse=True)
    def _configure_application(request, monkeypatch):
        """Use `pytest.mark.options` decorator to pass options to your application
        factory::

            @pytest.mark.options(debug=False)
            def test_something(app):
                assert not app.debug, 'the application works not in debug mode!'

        """
        if 'app' not in request.fixturenames:
            return

>       app = getfixturevalue(request, 'app')

../../.virtualenvs/flask-unchained/lib/python3.6/site-packages/pytest_flask/plugin.py:142: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../../.virtualenvs/flask-unchained/lib/python3.6/site-packages/pytest_flask/pytest_compat.py:3: in getfixturevalue
    return request.getfixturevalue(value)
tests/conftest.py:35: in app
    app = AppFactory.create_app(TEST, bundles=bundles, _config_overrides=options)
flask_unchained/app_factory.py:75: in create_app
    unchained.init_app(app, env, bundles, _config_overrides=_config_overrides)
flask_unchained/unchained.py:110: in init_app
    run_hooks_hook.run_hook(app, bundles, _config_overrides=_config_overrides)
flask_unchained/hooks/run_hooks_hook.py:29: in run_hook
    for hook in self.collect_from_bundles(bundles):
flask_unchained/hooks/run_hooks_hook.py:37: in collect_from_bundles
    hooks = self.collect_from_unchained()
flask_unchained/hooks/run_hooks_hook.py:46: in collect_from_unchained
    for Hook in self._collect_from_package(hooks_pkg).values()]
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <flask_unchained.hooks.run_hooks_hook.RunHooksHook object at 0x7f1bbed11240>
module = <module 'flask_unchained.hooks' from '/home/brian/dev/flask-unchained/flask_unchained/hooks/__init__.py'>
type_checker = <bound method RunHooksHook.type_check of <flask_unchained.hooks.run_hooks_hook.RunHooksHook object at 0x7f1bbed11240>>

    def _collect_from_package(self, module,
                              type_checker: Optional[FunctionType] = None,
                              ) -> Dict[str, Any]:
        """
        Discover objects passing :meth:`type_check` by walking through all the
        child modules/packages in the given module (ie, do not require modules
        to import everything into their ``__init__.py`` for it to be discovered)
        """
        type_checker = type_checker or self.type_check
        members = dict(self._get_members(module, type_checker))

>       if pkgutil.get_loader(module).is_package(module.__name__):
E       AttributeError: 'AssertionRewritingHook' object has no attribute 'is_package'

[asottile]

I think you're misunderstanding, for test modules while running tests the loader is the assertion rewriter, that's how pytest assertion magic works.

there are other ways to determine whether something is a package or not or to determine the filename using the importlib.util / importlib.machinery utilities

I can provide code samples when I'm not on my phone

[briancappello]

OK, that would be awesome, thanks!

[asottile]

import importlib.util


def test():
    spec = importlib.util.find_spec(__name__)
    # retrieve source filename
    assert spec.origin == __file__
    # retrive if it is a package
    assert not spec.submodule_search_locations

[asottile]

note that it's not just pytest's import hook that would fail your code above -- it's any PEP 451 compatible hook

[asottile]

also you may be interested in the same adjustments that flask itself needed during the 302 -> 451 loader change: https://github.com/pallets/flask/pull/3278