Open pganssle opened 4 years ago
Thanks Paul for writing up this proposal.
While I agree in principle - that the code under test should mirror the supported user's environment as closely as possible, as developers we make lots of compromises that violate that expectation:
I acknowledge that testing as editable-installed is an imperfect representation of the target environment. It is, however, a very close approximation and has at least these advantages that I appreciate:
src
directory to every project means the "getting started" begins with "create three directories, one for your repo, one for your source, and one for your package".python -c "import six"
).I'm also concerned with the disadvantages of the proposal. Both approaches for (OP 2) involve several changes to the codebase and add debt. They require more non-default settings and limit the options for overriding. For example, the "run from another directory" approach has these disadvantages:
Regarding (OP 1), I know in the past I've run into bugs in tox
for which the best known workaround was to use an editable install. One was tox-dev/tox#467 but the more serious one was tox-dev/tox#373 where the long-term resolution (pytest-dev/pytest#2042) is still imperfect and runs doctests on a different copy of the source than the code under test (another manifestation of "which copy of the code matters").
Some tests in the test suite already honor the proposed intention (e.g. test_integration, test_distutils_adoption), exercising critical behavior that would be masked by an editable install. Most of the tests, however, are exercised at a scope (unit, functional) that's unaffected by an editable install. Moreover, adopting test-as-installed still doesn't address some of the biggest gaps this project has for testing (capturing real-world scenarios like upgrades and source-only bootstrapping).
All of this experience leads me to believe that the costs of this change as proposed would vastly outweigh the benefits.
If running all tests as installed is important, I suggest to consider another approach - create a separate tox environment or other isolated build process that's dedicated to running tests as installed. This opt-in feature could be ignored by most workflows but invoked in select CI environments to provide the necessary protections. Perhaps this feature could run in dedicated docker containers or VMs so as to more accurately represent execution outside of a virtualenv as well.
Could we start with such an approach and evaluate the benefit before considering impacting the default developer workflows?
I don't think I'm able to confidently make changes to this project at this point, so you can feel free to do as you wish.
I think that if you want to continue your aggressive approach to modernizing the code and your (in my opinion) lax approach to testing, I fear that it will affect the reputation of this project and the PyPA more generally.
If this project cannot pass the tests when it's been installed, that should be a serious red flag.
It may be worth re-considering this, since after the 50.0.0 release, we've now hit several compatibility issues that would have been found with this testing approach (3.5 and 3.6 compatibility. most notably).
In the aforementioned commit, I've added support for testing as installed. Just run tox -e native
or tox -e py36-native
to run the tests as installed. Unfortunately, the tests fail with ImportError: Error importing plugin "setuptools.tests.fixtures": No module named 'setuptools.tests'
because tests aren't available in the package as installed.
A better test could have prevented the letsencrypt problem for probably many users See: https://github.com/pypa/setuptools/pull/1788
Unfortunately, the tests fail with
ImportError: Error importing plugin "setuptools.tests.fixtures": No module named 'setuptools.tests'
because tests aren't available in the package as installed.
I believe that this issue can be solved by renaming the fixtures.py
file to conftest.py
, since pytest natively supports per-dir local plugins.
I have a proof of concept that expands upon https://github.com/pypa/setuptools/commit/645bda8220d80f402a7b7d8e04588bc8906cab59 with some minor changes to improve isolation that seems to be working (I am currently testing it on #3015 against the CI).
When I tried to go one step further and implement the changedir
trick explained in @pganssle blog, I still managed to run the tests but there were a few complications:
pyproject.toml
: this is the case of pytest-enabler
that basically stopped working@pganssle do you know if without the changedir
or switching to a src-layout (but using a bare pytest
) there could still be some leaking? For example if a test uses subprocess.Popen
can it accidentally make use of implicit PYTHONPATH?
After some investigation (details in #3015) my thoughts are:
PYTHONPATH
for subprocess.Popen
/ runpy
/ etc ... So the isolation is leaky even if we rely on the "bare" pytest
command technique described in the linked articlechangedir
technique might interfere with the way the tests run. By changing the directory, tools and pytest plugins (such as pytest-enabler) may not be able to find their configuration files (e.g. pyproject.toml
)setuptools/tests
), but are not part of the distribution, the import
system may get very confused (they seem to be fine for CPython, but problematic in PyPy).
$PWD
, which might be incompatible with Python's import system:setuptools
package will exist in site-packages
, but the setuptools.tests
package will exist in the $PWD. I imagine that the problem here is that setuptools
is not a namespace package (either by PEP420 or previous definitions), so its subpackages cannot be distributed over different locations.Therefore, I would say that:
usedevelop=True
and accept the limitations and shortcomings
Now the question is, which one of the 2 approaches is the best for setuptools
? (Or do we want to write a more complex CI (a) that enforces the test isolation by dynamically modifying the way the files are organised just before running pytest?)
Update: The investigation in #3015 was completed. The available PR implementation makes it possible to run the test suite against an installed version of setuptools, however:
pep517
.I am happy to drop the PR if we think that this added complexity is not worth.
Just tested that. Looks like negative result 😢
tested as well pytest execution with --import-mode=importlib
and result is the same.
FYI some details about patches which I'm using
--- a/pkg_resources/_vendor/appdirs.py 2020-12-21 17:30:01.000000000 +0000
+++ b/pkg_resources/_vendor/appdirs.py 2021-01-04 01:07:34.939590382 +0000
@@ -1,4 +1,3 @@
-#!/usr/bin/env python
# -*- coding: utf-8 -*-
# Copyright (c) 2005-2010 ActiveState Software Inc.
# Copyright (c) 2013 Eddy Petrișor
Hi @kloczek, I welcome you to post doubts and problems specific to the patch in the discussion of the PR itself: #3015. I will post my comments and recommendations for you there :)
@kloczek
E _pytest.pathlib.ImportPathMismatchError: ('setuptools.config', '/home/tkloczko/rpmbuild/BUILDROOT/python-setuptools-60.5.4-2.fc35.x86_64/usr/lib/python3.8/site-packages/setuptools/config.py', PosixPath('/home/tkloczko/rpmbuild/BUILD/setuptools-60.5.4/setuptools/config.py'))
This is a typical misconfiguration of the directory layout on your side. Caused by having two locations with copies of setuptools
on PYTHONPATH
.
As https://github.com/pypa/setuptools/pull/3015 has been closed and because this ticket still is opened my undestanding is that solution of the "test as installed" issue still is on ToDo list. Am I right? 🤔
Thank you 😄
I think that I know where ios the main issue with testing setuptools
"as installed".
[tkloczko@devel-g2v setuptools-61.1.0]$ grep -r setuptools.tests setuptools
setuptools/tests/server.py: # The index files should be located in setuptools/tests/indexes
setuptools/tests/server.py: return 'http://127.0.0.1:%s/setuptools/tests/indexes/' % port
setuptools/tests/test_depends.py: mod_name = 'setuptools.tests.mod_with_constant'
setuptools/tests/test_depends.py: assert 'setuptools.tests.mod_with_constant' not in sys.modules
setuptools/tests/test_easy_install.py:from setuptools.tests.server import MockServer, path_to_url
setuptools/tests/test_easy_install.py:from setuptools.tests import fail_on_ascii
setuptools/tests/test_manifest.py:from setuptools.tests.textwrap import DALS
setuptools/tests/test_sdist.py:from setuptools.tests import fail_on_ascii
setuptools/tests/test_setuptools.py: f, p, i = dep.find_module('setuptools.tests')
setuptools/tests/test_setuptools.py: 'setuptools.tests.test_setuptools', '__doc__') == __doc__
setuptools/tests/test_setuptools.py: from setuptools.tests import __path__
setuptools/tests/test_setuptools.py: setuptools.convert_path('setuptools/tests')
Everywhere where is used setuptools.tests.foo
it causes that exactly that path needs to be used.
I think that best would be move setuptools/tests/ to /tests and conftest.py to tests/conftest.py and everthing should be adjusted for those new locations of those files. This would allow to stop installing setuptools provate test suite.
This is a typical misconfiguration of the directory layout on your side. Caused by having two locations with copies of
setuptools
onPYTHONPATH
.
This is as well typical mathgodology of testing modules usinn pytest used for example in Fedora.
Everywhere it works exacept setuptoos
. Basing on that (it works in most of the cases .. not that it works in Fedora) I would not call that "misconfiguration".
I'm using that methodology even on bigger scale than Fedora uses
[tkloczko@devel-g2v SPECS]$ grep ^%pytest *|wc -l
756
[tkloczko@devel-g2v SPECS.fedora]$ grep ^%pytest *|wc -l
610
and trust me .. ItWorks™️ Here is ehat in my case that %pytest macro does:
\
CFLAGS="-O2 -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -fdata-sections -ffunction-sections -flto=auto -flto-partition=none";
CXXFLAGS="-O2 -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -fdata-sections -ffunction-sections -flto=auto -flto-partition=none";
FFLAGS="-O2 -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -fdata-sections -ffunction-sections -flto=auto -flto-partition=none -I/usr/lib64/gfortran/modules";
FCFLAGS="-O2 -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -fdata-sections -ffunction-sections -flto=auto -flto-partition=none -I/usr/lib64/gfortran/modules";
LDFLAGS="-Wl,-z,relro -Wl,--as-needed -Wl,--gc-sections -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld -flto=auto -flto-partition=none -fuse-linker-plugin -Wl,--build-id=sha1";
CC="/usr/bin/gcc"; CXX="/usr/bin/g++"; FC="/usr/bin/gfortran";
AR="/usr/bin/gcc-ar"; NM="/usr/bin/gcc-nm"; RANLIB="/usr/bin/gcc-ranlib";
export CFLAGS CXXFLAGS FFLAGS FCFLAGS LDFLAGS CC CXX FC AR NM RANLIB;
\
PATH=/home/tkloczko/rpmbuild/BUILDROOT/%{NAME}-%{VERSION}-%{RELEASE}.x86_64/usr/bin:$PATH \
LD_LIBRARY_PATH=/home/tkloczko/rpmbuild/BUILDROOT/%{NAME}-%{VERSION}-%{RELEASE}.x86_64/usr/lib64 \
PYTHONDONTWRITEBYTECODE=1 \
SETUPTOOLS_SCM_PRETEND_VERSION=%{version} \
PYTHONPATH=${PYTHONPATH:-/home/tkloczko/rpmbuild/BUILDROOT/%{NAME}-%{VERSION}-%{RELEASE}.x86_64/usr/lib64/python3.8/site-packages:/home/tkloczko/rpmbuild/BUILDROOT/%{NAME}-%{VERSION}-%{RELEASE}.x86_64/usr/lib/python3.8/site-packages} \
\
/usr/bin/pytest -ra
Above is only extended to use it in more cases than originally it was designed. Core metchodology is the same.
solution of the "test as installed" issue still is on ToDo list. Am I right?
Aspirationally, yes, although I would caveat: it's still desirable for a developer to be able to rapidly develop on the project. That is, test as authored for fast iteration and subsequently, perhaps in CI, test as installed. I believe I have a branch or PR in place that does attempt to test as installed but it fails due to the packaging mismatch (tests are located in the package under test but aren't installed).
I think that best would be move setuptools/tests/ to /tests and conftest.py to tests/conftest.py and everthing should be adjusted for those new locations of those files.
I think it's more complicated than that. Setuptools specifically presents two packages (setuptools
and pkg_resources
) each of which has their own test suite. I haven't checked, but I suspect that if you fix the issue of the setuptools
test suite by moving it to ./tests
, you'll have the same problem with pkg_resources
and then you'll need to do something similar, but ./tests
is already taken. I'm okay with taking the incremental step of moving setuptools
' tests to ./tests
and leave pkg_resources
to contain its own tests until such a time that it can be moved to its own project or retired.
Move pkg_resources
is part of the https://github.com/pypa/setuptools/issues/863.
That module is used by many more projects. Some time ago I've started adding notes to my rpm spec files about such dependenceas (below it is not complete list)
[tkloczko@devel-g2v SPECS]$ grep pkg_resources *
botan2.spec:# NOTE: build needs pkg_resources
clang.spec:# NOTE: build needs pkg_resources module
cmake.spec:# NOTE: build needs pkg_resources
dnf-plugins-core.spec:# NOTE: build needs pkg_resources module
dnf.spec:# NOTE: build needs pkg_resources module
imath.spec:# NOTE: pkg_resources is used during build
libcomps.spec:# NOTE: build uses pkg_resources module
python-importlib-metadata.spec:name. This functionality intends to replace most uses of pkg_resources entry
python-importlib-metadata.spec:eliminate the need to use the older and less efficient pkg_resources package.
python-lingua.spec:# NOTE: test suite uses pkg_resources module
python-pytest-console-scripts.spec:# NOTE: setuptools is added ac CR because it requires pkg_resources which is part of the setuptools
python-setuptools.spec:execute the software that requires pkg_resources.
python-setuptools.spec:execute the software that requires pkg_resources.
python-setuptools.spec:cat pkg_resources/_vendor/vendored.txt setuptools/_vendor/vendored.txt > allvendor.txt
python-setuptools.spec:%{python3_sitelib}/pkg_resources
python-setuptools.spec:%exclude %{python3_sitelib}/{setuptools,pkg_resources}/tests
serd.spec:# NOTE: build uses pkg_resources
z3.spec:# NOTE: build needs pkg_resources
In other woirds this issues needs to be resolved somehow completly independentry from setuptools
teste suite.
As Ive reported in https://github.com/pypa/setuptools/issues/3362 looks like now is possible to run pytest with below patch
--- a/pytest.ini~ 2022-06-11 11:09:46.000000000 +0000
+++ b/pytest.ini 2022-06-12 01:15:20.396239151 +0000
@@ -1,4 +1,5 @@
[pytest]
+testpaths = setuptools/tests
norecursedirs=dist build .tox .eggs
addopts=
--doctest-modules
Just retested latest 62.4.0 with above patch and there are some failing units. Here is pytest output:
Full log in attachment python-setuptools-pytest_full_log.txt
Second thing is that there is second batch of test units in pkg_resources/tests and here situation is much worse because everything fails on collectiong units however my understanding is that pkg_resources is on some trajectory to be removed from setuptools so probably that part is less important.
Nevertheless good thing is that currently it is possible to pass at least collecting units 😃
Just tested 64.0.0 and looks like setuptools.tests is no longer installed (which is fine because test suite should not be installed) however pytest is now completly useless and no longer is working my hack with passing pytest testpaths
.
+ PYTHONPATH=/home/tkloczko/rpmbuild/BUILDROOT/python-setuptools-64.0.0-2.fc35.x86_64/usr/lib/python3.8/site-packages
+ /usr/bin/pytest -ra -p no:randomly setuptools/tests
Traceback (most recent call last):
File "/usr/lib/python3.8/site-packages/_pytest/config/__init__.py", line 746, in import_plugin
__import__(importspec)
ModuleNotFoundError: No module named 'setuptools.tests'
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
File "/usr/bin/pytest", line 8, in <module>
sys.exit(console_main())
File "/usr/lib/python3.8/site-packages/_pytest/config/__init__.py", line 187, in console_main
code = main()
File "/usr/lib/python3.8/site-packages/_pytest/config/__init__.py", line 145, in main
config = _prepareconfig(args, plugins)
File "/usr/lib/python3.8/site-packages/_pytest/config/__init__.py", line 324, in _prepareconfig
config = pluginmanager.hook.pytest_cmdline_parse(
File "/usr/lib/python3.8/site-packages/pluggy/_hooks.py", line 265, in __call__
return self._hookexec(self.name, self.get_hookimpls(), kwargs, firstresult)
File "/usr/lib/python3.8/site-packages/pluggy/_manager.py", line 80, in _hookexec
return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
File "/usr/lib/python3.8/site-packages/pluggy/_callers.py", line 55, in _multicall
gen.send(outcome)
File "/usr/lib/python3.8/site-packages/_pytest/helpconfig.py", line 102, in pytest_cmdline_parse
config: Config = outcome.get_result()
File "/usr/lib/python3.8/site-packages/pluggy/_result.py", line 60, in get_result
raise ex[1].with_traceback(ex[2])
File "/usr/lib/python3.8/site-packages/pluggy/_callers.py", line 39, in _multicall
res = hook_impl.function(*args)
File "/usr/lib/python3.8/site-packages/_pytest/config/__init__.py", line 1016, in pytest_cmdline_parse
self.parse(args)
File "/usr/lib/python3.8/site-packages/_pytest/config/__init__.py", line 1304, in parse
self._preparse(args, addopts=addopts)
File "/usr/lib/python3.8/site-packages/_pytest/config/__init__.py", line 1206, in _preparse
self.hook.pytest_load_initial_conftests(
File "/usr/lib/python3.8/site-packages/pluggy/_hooks.py", line 265, in __call__
return self._hookexec(self.name, self.get_hookimpls(), kwargs, firstresult)
File "/usr/lib/python3.8/site-packages/pluggy/_manager.py", line 80, in _hookexec
return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
File "/usr/lib/python3.8/site-packages/pluggy/_callers.py", line 60, in _multicall
return outcome.get_result()
File "/usr/lib/python3.8/site-packages/pluggy/_result.py", line 60, in get_result
raise ex[1].with_traceback(ex[2])
File "/usr/lib/python3.8/site-packages/pluggy/_callers.py", line 39, in _multicall
res = hook_impl.function(*args)
File "/usr/lib/python3.8/site-packages/_pytest/config/__init__.py", line 1083, in pytest_load_initial_conftests
self.pluginmanager._set_initial_conftests(
File "/usr/lib/python3.8/site-packages/_pytest/config/__init__.py", line 529, in _set_initial_conftests
self._try_load_conftest(anchor, namespace.importmode, rootpath)
File "/usr/lib/python3.8/site-packages/_pytest/config/__init__.py", line 546, in _try_load_conftest
self._getconftestmodules(anchor, importmode, rootpath)
File "/usr/lib/python3.8/site-packages/_pytest/config/__init__.py", line 575, in _getconftestmodules
mod = self._importconftest(conftestpath, importmode, rootpath)
File "/usr/lib/python3.8/site-packages/_pytest/config/__init__.py", line 623, in _importconftest
self.consider_conftest(mod)
File "/usr/lib/python3.8/site-packages/_pytest/config/__init__.py", line 704, in consider_conftest
self.register(conftestmodule, name=conftestmodule.__file__)
File "/usr/lib/python3.8/site-packages/_pytest/config/__init__.py", line 470, in register
self.consider_module(plugin)
File "/usr/lib/python3.8/site-packages/_pytest/config/__init__.py", line 712, in consider_module
self._import_plugin_specs(getattr(mod, "pytest_plugins", []))
File "/usr/lib/python3.8/site-packages/_pytest/config/__init__.py", line 719, in _import_plugin_specs
self.import_plugin(import_spec)
File "/usr/lib/python3.8/site-packages/_pytest/config/__init__.py", line 748, in import_plugin
raise ImportError(
File "/usr/lib/python3.8/site-packages/_pytest/config/__init__.py", line 746, in import_plugin
__import__(importspec)
ImportError: Error importing plugin "setuptools.tests.fixtures": No module named 'setuptools.tests'
Right now, we are testing an editable install of
setuptools
and possibly relying on the fact that the tests are run from the repository root, which is a problem. We should instead test our projects as installed, meaning we should:usedevelop=True
flag fromtox.ini
.src/
layout and/or run the tests from another directory per the suggestions in this blog post.This is especially important after the resolution to #2259, which makes a pretty significant change to the way our project works when it is installed normally as opposed to in an editable install. Considering that we're basically considering editable installs of
setuptools
to be an unsupported configuration, we should definitely be testing only installed configurations.