PytestRun apparently broken by pytest 3.7.0

[jsirois]

It looks like this pytest PR which landed in pytest 3.7.0 on 7/31/2018 breaks our custom node hook:

pants/src/python/pants/backend/python/tasks/pytest_run.py

Lines 391 to 454 in 4a4e481

	def _get_conftest_content(self, sources_map, rootdir_comm_path):
	# A conftest hook to modify the console output, replacing the chroot-based
	# source paths with the source-tree based ones, which are more readable to the end user.
	# Note that python stringifies a dict to its source representation, so we can use sources_map
	# as a format argument directly.
	#
	# We'd prefer to hook into pytest_runtest_logstart(), which actually prints the line we
	# want to fix, but we can't because we won't have access to any of its state, so
	# we can't actually change what it prints.
	#
	# Alternatively, we could hook into pytest_collect_file() and just set a custom nodeid
	# for the entire pytest run. However this interferes with pytest internals, including
	# fixture registration, leading to fixtures not running when they should.
	# It also requires the generated conftest to be in the root of the source tree, which
	# complicates matters when there's already a user conftest.py there.
	console_output_conftest_content = dedent("""
	### GENERATED BY PANTS ###
	import os
	import pytest
	class NodeRenamerPlugin(object):
	# Map from absolute source chroot path -> path of original source relative to the buildroot.
	_SOURCES_MAP = {sources_map!r}
	def __init__(self, rootdir):
	def rootdir_relative(path):
	return os.path.relpath(path, rootdir)
	self._sources_map = {{rootdir_relative(k): rootdir_relative(v)
	for k, v in self._SOURCES_MAP.items()}}
	@pytest.hookimpl(hookwrapper=True)
	def pytest_runtest_protocol(self, item, nextitem):
	# Temporarily change the nodeid, which pytest uses for display.
	real_nodeid = item.nodeid
	real_path = real_nodeid.split('::', 1)[0]
	fixed_path = self._sources_map.get(real_path, real_path)
	fixed_nodeid = fixed_path + real_nodeid[len(real_path):]
	try:
	item._nodeid = fixed_nodeid
	yield
	finally:
	item._nodeid = real_nodeid
	# The path to write out the py.test rootdir to.
	_ROOTDIR_COMM_PATH = {rootdir_comm_path!r}
	def pytest_configure(config):
	rootdir = str(config.rootdir)
	with open(_ROOTDIR_COMM_PATH, 'w') as fp:
	fp.write(rootdir)
	config.pluginmanager.register(NodeRenamerPlugin(rootdir), 'pants_test_renamer')
	""".format(sources_map=dict(sources_map), rootdir_comm_path=rootdir_comm_path))
	# Add in the sharding conftest, if any.
	shard_conftest_content = self._get_shard_conftest_content()
	return (console_output_conftest_content + shard_conftest_content).encode('utf8')

Still investigating.

[stuhood]

More info: in #6283, @jsirois bounded the pytest requirement to pytest>=3.0.7,<3.7, and although a few folks reported that as a local fix for that issue, it didn't seem to have fixed travis. He cleared the caches, and still saw the failure.

[stuhood]

I don't really have a minimal repro for this one, because even forcing pytest==3.7.0 (confirmed by PEX_VERBOSE) doesn't trigger it locally.

EDIT: Ah, correction:

./pants \
  --owner-of=tests/python/pants_test/backend/python/tasks/test_pytest_run.py \
  test \
  -- \
  -k test_pytest_run.py

repros.

[stuhood]

This is also seemingly non-deterministic... a different set of tests fails each time.

[stuhood]

The ZipImportError, For posterity:

.pants.d/pyprep/sources/74c7486b8853f0fa1285a2916a1f57df4e3635b6/pants_test/task_test_base.py:49:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
.pants.d/pyprep/sources/74c7486b8853f0fa1285a2916a1f57df4e3635b6/pants_test/backend/python/tasks/test_pytest_run.py:708: in test_sharding_invalid_shard_too_small
    self.run_tests(targets=[self.green], test_shard='-1/1')
.pants.d/pyprep/sources/74c7486b8853f0fa1285a2916a1f57df4e3635b6/pants_test/backend/python/tasks/test_pytest_run.py:41: in run_tests
    context = self._prepare_test_run(targets, *passthru_args, **options)
.pants.d/pyprep/sources/74c7486b8853f0fa1285a2916a1f57df4e3635b6/pants_test/backend/python/tasks/test_pytest_run.py:80: in _prepare_test_run
    pp_task_type(context, os.path.join(self.pants_workdir, 'pp')).execute()
.pants.d/pyprep/sources/74c7486b8853f0fa1285a2916a1f57df4e3635b6/pants/backend/python/tasks/pytest_prep.py:78: in execute
    pytest_binary = self.create_pex(pex_info)
.pants.d/pyprep/sources/74c7486b8853f0fa1285a2916a1f57df4e3635b6/pants/backend/python/tasks/python_execution_task_base.py:115: in create_pex
    interpreter, self.extra_requirements())
.pants.d/pyprep/sources/74c7486b8853f0fa1285a2916a1f57df4e3635b6/pants/backend/python/tasks/resolve_requirements_task_base.py:99: in resolve_requirement_strings
    dump_requirements(builder, interpreter, reqs, self.context.log)
.pants.d/pyprep/sources/74c7486b8853f0fa1285a2916a1f57df4e3635b6/pants/backend/python/tasks/pex_build_util.py:120: in dump_requirements
    distributions = resolve_multi(interpreter, deduped_reqs, platforms, find_links)
.pants.d/pyprep/sources/74c7486b8853f0fa1285a2916a1f57df4e3635b6/pants/backend/python/tasks/pex_build_util.py:164: in resolve_multi
    use_manylinux=python_setup.use_manylinux)
.pants.d/pyprep/requirements/CPython-2.7.15/f46d3d5642f927abde800a45286df68a827dfef7/.deps/pex-1.4.5-py2.py3-none-any.whl/pex/resolver.py:453: in resolve
    return resolver.resolve(resolvables_from_iterable(requirements, builder))
.pants.d/pyprep/requirements/CPython-2.7.15/f46d3d5642f927abde800a45286df68a827dfef7/.deps/pex-1.4.5-py2.py3-none-any.whl/pex/resolver.py:248: in resolve
    dist = self.build(package, resolvable.options)
.pants.d/pyprep/requirements/CPython-2.7.15/f46d3d5642f927abde800a45286df68a827dfef7/.deps/pex-1.4.5-py2.py3-none-any.whl/pex/resolver.py:332: in build
    return DistributionHelper.distribution_from_path(target)
.pants.d/pyprep/requirements/CPython-2.7.15/f46d3d5642f927abde800a45286df68a827dfef7/.deps/pex-1.4.5-py2.py3-none-any.whl/pex/util.py:94: in distribution_from_path
    distributions = set(find_distributions(path))
.pants.d/pyprep/requirements/CPython-2.7.15/f46d3d5642f927abde800a45286df68a827dfef7/.deps/pex-1.4.5-py2.py3-none-any.whl/pex/finders.py:48: in __call__
    for dist in finder(importer, path_item, only=only):
.pants.d/pyprep/requirements/CPython-2.7.15/f46d3d5642f927abde800a45286df68a827dfef7/.deps/pex-1.4.5-py2.py3-none-any.whl/pex/finders.py:191: in find_wheels_in_zip
    dist = wheel_from_metadata(path_item, metadata)
.pants.d/pyprep/requirements/CPython-2.7.15/f46d3d5642f927abde800a45286df68a827dfef7/.deps/pex-1.4.5-py2.py3-none-any.whl/pex/finders.py:153: in wheel_from_metadata
    pkg_info = Parser().parsestr(metadata.get_metadata(pkg_resources.DistInfoDistribution.PKG_INFO))
.pants.d/test/pytest-prep/CPython-2.7.15/6305bdc6171f2f5b3dae0fd2ad3961771f0d3bad/.bootstrap/pkg_resources/__init__.py:1463: in get_metadata
    value = self._get(self._fn(self.egg_info, name))
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <pex.finders.WheelMetadata instance at 0x10bb8ed88>
path = '/private/var/folders/91/bym4mtpx1p503xmvpp8fbdl40000gp/T/tmpfFTHPK_BUILD_ROOT/.pants.d/python-setup/resolved_requirements/CPython-2.7.15/scandir-1.7-cp27-cp27m-macosx_10_12_x86_64.whl/scandir-1.7.dist-info/METADATA'

    def _get(self, path):
        if hasattr(self.loader, 'get_data'):
>           return self.loader.get_data(path)
E           ZipImportError: bad local file header: /private/var/folders/91/bym4mtpx1p503xmvpp8fbdl40000gp/T/tmpfFTHPK_BUILD_ROOT/.pants.d/python-setup/resolved_requirements/CPython-2.7.15/scandir-1.7-cp27-cp27m-macosx_10_12_x86_64.whl

[stuhood]

@jsirois : It looks like there is a very succinct repro of the ImportError in master via:

./pants --owner-of=tests/python/pants_test/backend/python/tasks/test_gather_sources.py test -- -k test_gather_sources

...which unfortunately seems to be blocking #6289.

[jsirois]

Nice. I do repro and then fix by:

diff --git a/3rdparty/python/requirements.txt b/3rdparty/python/requirements.txt
index 5811ad431..cfc5c0cf2 100644
--- a/3rdparty/python/requirements.txt
+++ b/3rdparty/python/requirements.txt
@@ -27,7 +27,7 @@ pywatchman==1.4.1
 requests[security]>=2.5.0,<2.19
 scandir==1.2
 setproctitle==1.1.10
-setuptools==33.1.1
+setuptools==30.0.0
 six>=1.9.0,<2
 subprocess32==3.2.7 ; python_version<'3'
 thrift>=0.9.1
diff --git a/src/python/pants/backend/python/subsystems/python_setup.py b/src/python/pants/backend/python/subsystems/python_setup.py
index 9eead0359..cbec22f17 100644
--- a/src/python/pants/backend/python/subsystems/python_setup.py
+++ b/src/python/pants/backend/python/subsystems/python_setup.py
@@ -26,7 +26,7 @@ class PythonSetup(Subsystem):
                   "or 'PyPy' (A pypy interpreter of any version). Multiple constraint strings will "
                   "be ORed together. These constraints are applied in addition to any "
                   "compatibilities required by the relevant targets.")
-    register('--setuptools-version', advanced=True, default='33.1.1',
+    register('--setuptools-version', advanced=True, default='30.0.0',
              help='The setuptools version for this python environment.')
     register('--wheel-version', advanced=True, default='0.29.0',
              help='The wheel version for this python environment.')

Which undoes #6225. Digging more in #6225 to see if a compromise can be reached.

[stuhood]

@jsirois : I also just determined that is related to the interplay between the two test methods in that class: test_gather_files and test_gather_sources. The -k test_gather_sources actually causes them both to run (because it matches test_gather_sources.py).

I was able to confirm that running either method in isolation doesn't repro. So I think that it may be some nastiness related to re-using the interpreter cache between the two tests...?

[jsirois]

And reverting bd434de with the patch above in effect (setuptools pinned lower), also green.

Thanks Stu - pondering...

[jsirois]

Notes: likely the root problem is the zipimporter cache issue described here: pypa/setuptools#168 (comment)

I'm pretty sure we hit this in the dim past of pex / pants. I'll dig along these lines this morning.

[jsirois]

And the underlying python issue, a good read: https://bugs.python.org/issue19081

[stuhood]

I have a workaround for this for #6289, although fixing the underlying issue would continue to be awesome.

[jsirois]

OK - we hit this zipimporter issue because the TestBase buildroot is setup at the class level but also cleaned between each test run. A quick scan says this is a speed hack to support only doing _init_engine once per test class instead of once per test method call. I'll see if this can be untangled.

[benjyw]

Looks like @asherf was unable to upgrade pytest in #8177. So should we assume this is still an issue? It is a major one - the version of pytest we're restricting to is ancient.