Fix assertion rewriting module detection for egg dists #6313

nicoddemus · 2019-12-03T22:53:32Z

bluetech

Some comments:

I wonder if it is not possible to unify the two cases by using importlib_metadata a bit more? Maybe by getting the file name using file.locate() instead of str(file) in _mark_plugins_for_rewrite(), or maybe some other way - I'm not really familiar with that library.

Is there a reason not to just strip to at most the two last components (so it catches foo.py and foo/__init__.py) instead of stripping one component at a time and trying that? So source/python/bar/__init__.py -> bar/__init__.py instead of source/python/bar/__init__.py -> python/bar/__init__.py -> bar/__init__.py.

Does the function need to handle subpackages, e.g. pytest_mock.foo._version? If such a file existed, wouldn't it conflict with pytest_mock._version? Or maybe subpackages are considered different "distributions"?

src/_pytest/config/__init__.py

nicoddemus · 2019-12-04T12:10:28Z

I wonder if it is not possible to unify the two cases by using importlib_metadata a bit more? Maybe by getting the file name using file.locate() instead of str(file) in _mark_plugins_for_rewrite(), or maybe some other way - I'm not really familiar with that library.

Good point, that's something that looks like worth pursuing. @asottile (which knows a lot more about importlib_metadata than I) any input here?

Is there a reason not to just strip to at most the two last components (so it catches foo.py and foo/init.py) instead of stripping one component at a time and trying that? So source/python/bar/init.py -> bar/init.py instead of source/python/bar/init.py -> python/bar/init.py -> bar/init.py.

Does the function need to handle subpackages, e.g. pytest_mock.foo._version? If such a file existed, wouldn't it conflict with pytest_mock._version? Or maybe subpackages are considered different "distributions"?

All good points, the original intent was to only mark for rewrite the top level package, because register_assert_rewrite only needs the top-level package, but adding the subpackages is harmless (it will only increase the size of the set() used to hold the names).

But on second thought, perhaps we don't need to be so nit-picky here and just add all modules/packages we find?

asottile

I'm of the opinion that we should not support eggs -- and I don't think this even does add egg support as they will not pass the exists(...) check later on when looking to rewrite the actual files

maybe I don't understand the issue completely 🤔

asottile · 2019-12-04T16:43:43Z

src/_pytest/config/__init__.py

+    if not seen_some:
+        # at this point we did not find any packages or modules suitable for assertion
+        # rewriting, so we try again by stripping the first path component (to account for
+        # "src" based source trees for example)


we should not do this arbitrarily -- src is just a use of package_dir but it could be anywhere or any number of directories deep

perhaps a better approach would be to use sys.path to determine whether they are modules or not?

we should not do this arbitrarily -- src is just a use of package_dir but it could be anywhere or any number of directories deep

That's just a comment, we are not hard-coding src anywhere... we do strip the first directory (if we don't find a suitable mode/package) and try again in the next iteration.

asottile · 2019-12-04T16:45:42Z

src/_pytest/config/__init__.py

+        https://github.com/pytest-dev/pytest-mock/issues/167
+    """
+    package_files = list(package_files)
+    seen_some = False
    for fn in package_files:
        is_simple_module = "/" not in fn and fn.endswith(".py")
        is_package = fn.count("/") == 1 and fn.endswith("__init__.py")


hmmm the existing code seems to be a pre-PEP420 world, I wonder if we could update this to not be so dependent on __init__.py files

Not sure, pytest itself doesn't seem to support this and AFAIR many tools were struggling to add support to this as well...

asottile · 2019-12-04T16:52:45Z

though it is interesting that importlib_metadata is handing out the SOURCES instead of the installed-files metadata, that feels like a bug -- maybe @jaraco has some ideas there

nicoddemus · 2019-12-04T17:28:49Z

I'm of the opinion that we should not support eggs

That was my initial reaction as well, but pip install --editable still works with eggs, and that's something we definitely should support IMO (that was the origin of the issue, the assertion rewriter was not rewriting pytest-mock's own files in develop mode).

asottile · 2019-12-04T17:33:17Z

makes sense, we can probably iterate installed-files.txt instead of what importlib_metadata hands us to fix this?

nicoddemus · 2019-12-04T17:59:36Z

makes sense, we can probably iterate installed-files.txt instead of what importlib_metadata hands us to fix this?

I'm not sure what you mean exactly, but I think whatever you think is appropriate here can be done; this code dates from our 2016 sprint, if there are improvements to be done to it we should.

Can you post an example of using installed-files.txtand what it outputs for a package? Also if you are inclined to push to this PR please do so. 😁

asottile · 2019-12-04T18:05:27Z

here's an example:

$ tail -n999 venv/lib/python3.6/site-packages/pytest_mock-1.12.1-py3.6.egg-info/{installed-files,SOURCES}.txt
==> venv/lib/python3.6/site-packages/pytest_mock-1.12.1-py3.6.egg-info/installed-files.txt <==
../pytest_mock/__init__.py
../pytest_mock/__pycache__/__init__.cpython-36.pyc
../pytest_mock/__pycache__/_version.cpython-36.pyc
../pytest_mock/__pycache__/plugin.cpython-36.pyc
../pytest_mock/_version.py
../pytest_mock/plugin.py
PKG-INFO
SOURCES.txt
dependency_links.txt
entry_points.txt
requires.txt
top_level.txt

==> venv/lib/python3.6/site-packages/pytest_mock-1.12.1-py3.6.egg-info/SOURCES.txt <==
.gitignore
.pre-commit-config.yaml
CHANGELOG.rst
HOWTORELEASE.rst
LICENSE
README.rst
setup.cfg
setup.py
tox.ini
.github/FUNDING.yml
.github/workflows/main.yml
src/pytest_mock/__init__.py
src/pytest_mock/_version.py
src/pytest_mock/plugin.py
src/pytest_mock.egg-info/PKG-INFO
src/pytest_mock.egg-info/SOURCES.txt
src/pytest_mock.egg-info/dependency_links.txt
src/pytest_mock.egg-info/entry_points.txt
src/pytest_mock.egg-info/requires.txt
src/pytest_mock.egg-info/top_level.txt
tests/test_pytest_mock.py

though hmmm, that file doesn't show up for editable installs so now I'm just confused 🤔

asottile

I think this is fine then, I can't see a better way to solve this without importlib-metadata being more clever about editable isntalls

nicoddemus · 2019-12-04T18:29:22Z

here's an example

Hmmm can I access that contents through importlib-metadata, or only by accessing the filesystem directly?

nicoddemus · 2019-12-04T18:30:17Z

I think we can go with the PR as is: it solves the existing problem without incurring additional overhead; we can improve this later if we find a more elegant way to accomplish this. 👍

Fix pytest-dev#6301

nicoddemus · 2019-12-26T16:37:19Z

Backported in #6368

amatissart · 2020-01-12T13:44:22Z

This change causes a new warning in one of our projects, that uses lovely-pytest-docker==0.1.0 as a plugin. It looks like this plugin is packaged with a submodule named pytest:

lovely/pytest/__init__.py
lovely/pytest/docker/__init__.py
lovely/pytest/docker/compose.py
...

As a result this warning is now triggered

/lib/python3.6/site-packages/_pytest/config/__init__.py:891
  /lib/python3.6/site-packages/_pytest/config/__init__.py:891: PytestAssertRewriteWarning: 
Module already imported so cannot be rewritten: pytest
    self._mark_plugins_for_rewrite(hook)

which is probably harmless, but still confusing.

I am not familiar with the details of python packages ; is there a problem with how this specific dependency is packaged ?

asottile · 2020-01-12T15:47:53Z

yeah they probably should not use pytest (our namespace) as their namespace

bluetech reviewed Dec 4, 2019

View reviewed changes

src/_pytest/config/__init__.py Outdated Show resolved Hide resolved

asottile reviewed Dec 4, 2019

View reviewed changes

asottile approved these changes Dec 4, 2019

View reviewed changes

Fix assertion rewriting module detection for egg dists

c7f9fda

Fix pytest-dev#6301

nicoddemus force-pushed the egg-rewrite-6301 branch from e8fa03f to c7f9fda Compare December 4, 2019 18:31

asottile merged commit 7ff91d8 into pytest-dev:master Dec 6, 2019

nicoddemus deleted the egg-rewrite-6301 branch December 26, 2019 16:37

nicoddemus added the backported PR has been backported to the current bug-fix branch label Dec 26, 2019

blueyed mentioned this pull request Mar 18, 2020

_mark_plugins_for_rewrite: use entrypoint module directly blueyed/pytest#297

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix assertion rewriting module detection for egg dists #6313

Fix assertion rewriting module detection for egg dists #6313

nicoddemus commented Dec 3, 2019

bluetech left a comment

nicoddemus commented Dec 4, 2019

asottile left a comment

asottile Dec 4, 2019

nicoddemus Dec 4, 2019

asottile Dec 4, 2019

nicoddemus Dec 4, 2019

asottile commented Dec 4, 2019

nicoddemus commented Dec 4, 2019

asottile commented Dec 4, 2019

nicoddemus commented Dec 4, 2019

asottile commented Dec 4, 2019

asottile left a comment

nicoddemus commented Dec 4, 2019

nicoddemus commented Dec 4, 2019

nicoddemus commented Dec 26, 2019

amatissart commented Jan 12, 2020

asottile commented Jan 12, 2020

Fix assertion rewriting module detection for egg dists #6313

Fix assertion rewriting module detection for egg dists #6313

Conversation

nicoddemus commented Dec 3, 2019

bluetech left a comment

Choose a reason for hiding this comment

nicoddemus commented Dec 4, 2019

asottile left a comment

Choose a reason for hiding this comment

asottile Dec 4, 2019

Choose a reason for hiding this comment

nicoddemus Dec 4, 2019

Choose a reason for hiding this comment

asottile Dec 4, 2019

Choose a reason for hiding this comment

nicoddemus Dec 4, 2019

Choose a reason for hiding this comment

asottile commented Dec 4, 2019

nicoddemus commented Dec 4, 2019

asottile commented Dec 4, 2019

nicoddemus commented Dec 4, 2019

asottile commented Dec 4, 2019

asottile left a comment

Choose a reason for hiding this comment

nicoddemus commented Dec 4, 2019

nicoddemus commented Dec 4, 2019

nicoddemus commented Dec 26, 2019

amatissart commented Jan 12, 2020

asottile commented Jan 12, 2020