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

get rid of "ImportMismatchError" #2042

Labels
topic: collection related to the collection phase type: enhancement new feature or API change, should be merged into features branch type: feature-branch new feature or API change, should be merged into features branch

Comments

@hpk42
Copy link
Contributor

hpk42 commented Nov 2, 2016

Over on the tox issue tracker, there was a discussion about the irritating "ImportMismatchError" that pytest throws if one collects from the source tree but the imports actually come from some installed location (e.g. some virtualenv). This happens more often with tox i guess because the default way of testing with tox is creating and installing a package and then collecting from the source tree.

@jaraco and me agreed that it's probably best to try improve pytest so that it, say with pytest-4.0, will automatically redirect from source to install location. Initially and up until the next major release we could introduce a --collect-redirect-to-installed=yes option or so which works like this: if during collection we find a python package (a directory with an __init__.py for now) we check if importing it will actually get it from another location. If so, we redirect to the install location and also report this fact. Only problem is that with python3 one can have python packages without __init__.py files so the actual logic needs to take that into account as well (how many people are really using this, btw?).

@hpk42 hpk42 added type: enhancement new feature or API change, should be merged into features branch topic: collection related to the collection phase type: feature-branch new feature or API change, should be merged into features branch labels Nov 2, 2016
@RonnyPfannschmidt
Copy link
Member

@hpk42 proposal: ignoring the error altogether if the contents of the wanted and the imported file are the same (and adding a mention of install/reinstall/develop maybe)

@hpk42
Copy link
Contributor Author

hpk42 commented Nov 9, 2016

ignoring the error seems dangerous to me as the module under question might import or depend on other modules which are not the same at source versus install location. So i can change files at the source location, run tests and they all appear to be running fine while in fact they are just running against the last installed version.

@RonnyPfannschmidt
Copy link
Member

thats why i made content equality a condition!

@The-Compiler
Copy link
Member

But with what @hpk42 brought up, the condition would need to be module equality, plus equality for all modules that module imports - right?

@hpk42
Copy link
Contributor Author

hpk42 commented Nov 9, 2016

sure, i was referring to that case where a module source under test is equal to the imported version of it. It might still import something that isn't. Imagine e.g. a typical inline test layout with __init__.py files. The test file mypkg/tests/test_x.py might be the same at the source and at the actually installed location but the modules it itself imports would come from install whereas you assume they come from source. This is precisely what ImportMismatchError tries to detect.

@RonnyPfannschmidt
Copy link
Member

well, from my pov we can either stop supporting broken folder layouts or we have to introduce broken hacks ^^ my proposal was such a hack

i mean this is one of the cases where we literally loose because there is no clean and backward compatible way to support this, so we pick the cost and pay it ^^

@hpk42
Copy link
Contributor Author

hpk42 commented Nov 9, 2016

I hold that the automatic redirection from source to install location is worth trying and could resolve this problem by default with pytest-4.0 (if not earlier -- nobody can really have relied on an ImportMismatchError appearing).

@jaraco
Copy link
Contributor

jaraco commented Nov 9, 2016

Only problem is that with python3 one can have python packages without init.py files so the actual logic needs to take that into account as well (how many people are really using this, btw?)

In pypa/setuptools#250, I'm working to resolve issues with namespace packages, and the leading solution will require all namespace package distributions to follow this model, even at the development source. And currently, any namespace packages installed by pip (or with setuptools --single-version-externally-managed install) will also have this characteristic (even on Python 2). That is all packages jaraco.* and zope.* and similar will not have __init__.py in the namespace package as installed by pip (not -e) and rely on the -nspkg.pth or PEP 420 (Python 3.3+) to create the package module.

@hpk42
Copy link
Contributor Author

hpk42 commented Nov 9, 2016

Is there a way to find out if a directory is the root of a package if there is no __init__.py?

@jaraco
Copy link
Contributor

jaraco commented Nov 9, 2016

I thought you might ask that, and the answer is essentially no. Or rather, on Python 3.3+, any directory on sys.path is always the root of a package:

$ mkdir foo
$ python -c "import foo; print(foo.__path__)"
_NamespacePath(['/Users/jaraco/foo'])

Probably the packaging infrastructure should expose the "namespace packages" declared for a given distribution, such that you could enumerate all installed packages and determine the namespace packages they declare (and from there resolve the paths pertinent to those packages), but in my initial investigation, I don't see that metadata exposed for installed packages.

@jaraco
Copy link
Contributor

jaraco commented Nov 9, 2016

@hpk42: Is it viable to actually perform the import and then inspect <module>.__path__? Or do you need to know if a directory is the root of a package by inspecting the file system only?

@RonnyPfannschmidt
Copy link
Member

we need to figure the module name from the filesystem so its a bit tricky to guess the root

@hpk42
Copy link
Contributor Author

hpk42 commented Nov 9, 2016

@jaraco i guess it's possible. Directories are not themselves represented in the collection tree. So as soon as we collect a python module and would throw an ImportMismatchError we can instead deliver a message "redirecting collection of source location X to installed location Y" and accept the import instead of throwing. We probably want to only report this once for the root, though, not for each module.

@RonnyPfannschmidt
Copy link
Member

for correct redirection we should compare a complete module tree before accepting it (imagine a installed package missing a module

@hpk42
Copy link
Contributor Author

hpk42 commented Nov 9, 2016

maybe this works: when importing a collected source python module which turns out to come from some other install-location we memorize a source-rootdir -> install-rootdir mapping and issue a report about the redirection. When we subsequently try to import another module based on a mapped root, we don't report it again but verify that the import will come from the install location and if not throw an ImportMismatchError (this should be very rare). We might also check at first redirection time that no module was imported already and was not mapped. Altogether requires a bit of state keeping but should be doable.

@nicoddemus
Copy link
Member

nicoddemus commented Nov 10, 2016

I'm still mid-way digesting the discussion, so sorry if I'm asking something which is obvious to everyone involved.

I'm used to run tests in 3 scenarios, which is:

  • tox
  • setup.py develop
  • configuring PYTHONPATH for conda (which is very similar to setup.py develop).

Of those, only when using tox an ImportMismatchError can happen and we know it is not actually an error because tox guarantees for the files to be the same since it was tox itself who copied them from source to destination.

In which situations an ImportMismatch error happens outside using tox? Some other tool or some workflow where that error is common and is actually an error, the fact that a module being imported was already imported under another name?

@hpk42
Copy link
Contributor Author

hpk42 commented Nov 10, 2016

regarding tox installing packages, it's possible that due to MANIFEST or other issues not all files are installed.

Import-Mismatch can happen out side tox e.g.: if you "pip install ." and then run tests with pytest in the case where you have tests on source files (app or tests) which were installed and which python will import from there. The same issue applied for PYTHONPATH.

@RonnyPfannschmidt
Copy link
Member

@nicoddemus when people do pip install instead of pip install -e that can happen as well

@nicoddemus
Copy link
Member

nicoddemus commented Nov 10, 2016

@hpk42 and @RonnyPfannschmidt thanks!

How about we add some way to force pytest to not raise the ImportMismatchError? This makes it possible to bypass this check on situations where we know for sure the error is a false positive, for example when running under tox. I'm thinking of something non-intrusive and simple to implement like PYTEST_IGNORE_IMPORT_MISMATCH=1. With it in pytest, tox could always set that environment variable behind the scenes automatically even if the user is not using pytest, which would solve the problem for all tox users.

While of course that's not the most elegant solution and does not address the underlying issue, it has some advantages to it:

  1. Very simple to implement so we can get this out very soon: tox sets an environment variable, pytest reads it and does not raise the error;
  2. Basically no maintenance burden;
  3. Backward compatible, for tox and pytest;
  4. Can be kept around even if we implement a proper solution later, because as this discussion shows the solution probably isn't easy and it might contain bugs itself.

This is not meant to solve the other user cases you described, but at least would solve the issue for a lot of tox users now, while we can discuss and refine a more long term solution. We might even decide to not document it at all, keep it as an "internal workaround" to at least alleviate tox users for now.

Just throwing this idea here to see what you guys think, don't mean to disrupt/stop the current discussion on how to treat the problem properly.

@RonnyPfannschmidt
Copy link
Member

a accessible workaround for tox/env var use seems really sensible as a first step to enable users to test at all and leaves us time for working on a more detailed "propper" fix

im in favour, good idea :)

@nicoddemus
Copy link
Member

@hpk42 and @jaraco, what are your thoughts on the PYTEST_IGNORE_IMPORT_MISMATCH workaround?

@wheerd
Copy link
Contributor

wheerd commented Dec 9, 2016

I am also having this issue when using the Windows Subsystem for Linux and trying to run my tests there. Apparently the linux/windows paths get mixed up.

I am not sure if this is a separate issue as I am somewhat surprised how the windows path even gets in there in the first place. Here is the traceback (with shortened paths):

Traceback (most recent call last):
  File "/usr/local/lib/python3.5/dist-packages/_pytest/config.py", line 325, in _getconftestmodules
    return self._path2confmods[path]
KeyError: local('/mnt/c/.../tests')

During handling of the above exception, another exception occurred:
Traceback (most recent call last):
  File "/usr/local/lib/python3.5/dist-packages/_pytest/config.py", line 356, in _importconftest
    return self._conftestpath2mod[conftestpath]
KeyError: local('/mnt/c/.../tests/conftest.py')

During handling of the above exception, another exception occurred:
Traceback (most recent call last):
  File "/usr/local/lib/python3.5/dist-packages/_pytest/config.py", line 362, in _importconftest
    mod = conftestpath.pyimport()
  File "/usr/local/lib/python3.5/dist-packages/py/_path/local.py", line 668, in pyimport
    raise self.ImportMismatchError(modname, modfile, self)
py._path.local.LocalPath.ImportMismatchError: ('tests.conftest', 'C:\\...\\tests\\conftest.py', local('/mnt/c/.../tests/conftest.py'))
ERROR: could not load /mnt/c/.../tests/conftest.py

@jaraco
Copy link
Contributor

jaraco commented Dec 9, 2016

what are your thoughts on the PYTEST_IGNORE_IMPORT_MISMATCH workaround?

That sounds reasonable to me. In my experience, it's only the tox-created environments that are experiencing the false positives.

@blueyed
Copy link
Contributor

blueyed commented Jan 19, 2017

Another case where this happens when running tests in a Docker container, where the source gets bind-mounted as an image (into something like /srv, /app or /src typically), and you had run the tests on the host already.
In this case no automatic redirection would be possible.

RonnyPfannschmidt added a commit to RonnyPfannschmidt/pytest that referenced this issue Jan 22, 2017
due to a devpi bug, we always get a sdist install which in turn triggers
the pytest issue pytest-dev#2042 / pytest-dev#726

going for pyargs and a changed folder, it should no longer happen
(and will be tested firther after rebasing the release branch)
@blueyed
Copy link
Contributor

blueyed commented Sep 25, 2018

@RonnyPfannschmidt
Please read some of the above comments: it typically happens when running tests first withing Docker and then without - that is caused by the file paths typically being different inside the Docker container, regardless of the project layout.
Also using symlinks appears to cause it (#2042 (comment)) in general.
I think we should add support for py to ignore this in case some env variable is set. That is useful in general.

@RonnyPfannschmidt
Copy link
Member

im all for fixing problems, but it sounds a lot like all we will be doing is hiding/ignoring

@blueyed
Copy link
Contributor

blueyed commented Sep 25, 2018

Yeah, I see.
pytest-dev/py#199 will allow to ignore it, and it needs to be enabled explicitly. This should allow for working around cases where py is wrong / getting in the way.

While you can (and should) fix the problem when run with Docker (find -name "*.pyc" -delete), there appear to be cases where this is not possible (symlink).
The symlink issue might be a bug in py after all (and should get reported/fixed there then).
It would be good if @gillesdouaire would report it with py.

@gillesdouaire
Copy link

thanks very much for the MR @blueyed . I'll report the symlink issue with py project.

@nicoddemus
Copy link
Member

py-1.7.0 has been released which supports using PY_IGNORE_IMPORTMISMATCH=1 to suppress this error. While this is not ideal, it is at least a escape hatch for cases where it is getting in the way. Thanks @blueyed for the patch! 🙇

@gillesdouaire
Copy link

Thanks! We just used the latest py and it works very well in our context :)

@blueyed
Copy link
Contributor

blueyed commented Oct 13, 2018

I think it would be worth to add a note about using the env to https://github.com/blueyed/pytest/blob/6bf4692c7d8100b19dc0178764398edbd7803701/src/_pytest/python.py#L487-L496 (if py >= 1.7).

clrpackages pushed a commit to clearlinux-pkgs/py that referenced this issue Oct 15, 2018
…idth on windows

Bruno Oliveira (5):
      Merge pull request #197 from nicoddemus/release-1.6.0
      Fix test and add CHANGELOG for #174
      Merge pull request #199 from blueyed/PY_IGNORE_IMPORTMISMATCH
      Merge pull request #201 from Victorious3/issue174
      Update CHANGELOG for 1.7.0

Daniel Hahler (1):
      pyimport: add support for PY_IGNORE_IMPORTMISMATCH

Victorious3 (1):
      Use shutil.get_terminal_size for correct terminal width on windows

1.7.0 (2018-10-11)
==================

- fix #174: use ``shutil.get_terminal_size()`` in Python 3.3+ to determine the size of the
  terminal, which produces more accurate results than the previous method.

- fix pytest-dev/pytest#2042: introduce new ``PY_IGNORE_IMPORTMISMATCH`` environment variable
  that suppresses ``ImportMismatchError`` exceptions when set to ``1``.
@shreyasbapat
Copy link

@blueyed
Copy link
Contributor

blueyed commented Feb 10, 2019

@shreyasbapat
Use the env?

@pytest-dev pytest-dev locked as resolved and limited conversation to collaborators Feb 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.