Fix PytestRun to honor user conftest.py plugins. (#7784)

Previously codebases that leveraged pytest's `conftest.py` support could
fail when tested with `./pants test` in various situations. This was
ultimately because we injected our own `conftest.py` to register our
custom pytest plugins and plugin discovery of `conftest.py` plugins is
brittle. We avoid the brittleness by moving our plugin into a `sys.path`
module activated with `-p` fixing a previously failing test added in
`test_conftests_discovery_with_coverage`.

src/python/pants/backend/python/tasks/BUILD

@@ -17,6 +17,7 @@ python_library(
     'src/python/pants/backend/python/subsystems',
     'src/python/pants/backend/python/targets',
     'src/python/pants/backend/python/tasks/coverage:plugin',
+    'src/python/pants/backend/python/tasks/pytest:plugin',
     'src/python/pants/base:build_environment',
     'src/python/pants/base:exceptions',
     'src/python/pants/base:fingerprint_strategy',

src/python/pants/backend/python/tasks/pytest/BUILD

@@ -0,0 +1,7 @@
+# Copyright 2019 Pants project contributors (see CONTRIBUTORS.md).
+# Licensed under the Apache License, Version 2.0 (see LICENSE).
+
+resources(
+  name='plugin',
+  sources=['plugin.py']
+)

src/python/pants/backend/python/tasks/pytest/plugin.py

@@ -0,0 +1,121 @@
+# coding=utf-8
+# Copyright 2019 Pants project contributors (see CONTRIBUTORS.md).
+# Licensed under the Apache License, Version 2.0 (see LICENSE).
+
+from __future__ import absolute_import, division, print_function, unicode_literals
+
+import json
+import os
+
+import pytest
+
+
+class NodeRenamerPlugin(object):
+  """A pytest plugin to modify the console output of test names.
+
+  Replaces the chroot-based source paths with the source-tree based ones, which are more readable
+  to the end user.
+  """
+
+  def __init__(self, rootdir, sources_map):
+    def rootdir_relative(path):
+      return os.path.relpath(path, rootdir)
+
+    self._sources_map = {rootdir_relative(k): rootdir_relative(v) for k, v in sources_map.items()}
+
+  # 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.
+  @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
+
+
+class ShardingPlugin(object):
+  def __init__(self, shard, num_shards):
+    self._shard = shard
+    self._num_shards = num_shards
+
+  def pytest_report_header(self, config):
+    return ('shard: {shard} of {num_shards} (0-based shard numbering)'
+            .format(shard=self._shard, num_shards=self._num_shards))
+
+  def pytest_collection_modifyitems(self, session, config, items):
+    total_count = len(items)
+    removed = 0
+    def is_conftest(itm):
+      return itm.fspath and itm.fspath.basename == 'conftest.py'
+    for i, item in enumerate(list(x for x in items if not is_conftest(x))):
+      if i % self._num_shards != self._shard:
+        del items[i - removed]
+        removed += 1
+    reporter = config.pluginmanager.getplugin('terminalreporter')
+    reporter.write_line('Only executing {count} of {total} total tests in shard {shard} of '
+                        '{num_shards}'.format(count=total_count - removed,
+                                              total=total_count,
+                                              shard=self._shard,
+                                              num_shards=self._num_shards),
+                        bold=True, invert=True, yellow=True)
+
+
+def pytest_addoption(parser):
+  group = parser.getgroup('pants', 'Pants testing support')
+  group.addoption('--pants-rootdir-comm-path',
+                  dest='rootdir_comm_path',
+                  action='store',
+                  metavar='PATH',
+                  help='Path to a file to write the pytest rootdir to.')
+  group.addoption('--pants-sources-map-path',
+                  dest='sources_map_path',
+                  action='store',
+                  metavar='PATH',
+                  help='Path to a source mapping file that should contain JSON object mapping from'
+                       'absolute source chroot path keys to the path of the original source '
+                       'relative to the buildroot.')
+  group.addoption('--pants-shard',
+                  dest='shard',
+                  action='store',
+                  default=0,
+                  type=int,
+                  help='The shard of tests to select for this run.')
+  group.addoption('--pants-num-shards',
+                  dest='num_shards',
+                  action='store',
+                  default=1,
+                  type=int,
+                  help='The total number of shards being used to complete the run.')
+
+
+def pytest_configure(config):
+  if config.getoption('help'):
+    # Don't configure our plugins when the user is just asking for help.
+    return
+
+  rootdir = str(config.rootdir)
+  rootdir_comm_path = config.getoption('rootdir_comm_path')
+  with open(rootdir_comm_path, 'w') as fp:
+    fp.write(rootdir)
+
+  sources_map_path = config.getoption('sources_map_path')
+  with open(sources_map_path) as fp:
+    sources_map = json.load(fp)
+
+  config.pluginmanager.register(NodeRenamerPlugin(rootdir, sources_map), 'pants_test_renamer')
+
+  num_shards = config.getoption('num_shards')
+  if num_shards > 1:
+    shard = config.getoption('shard')
+    config.pluginmanager.register(ShardingPlugin(shard, num_shards))

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

@@ -16,15 +16,34 @@
 from pants.backend.python.subsystems.pytest import PyTest
 from pants.backend.python.targets.python_tests import PythonTests
 from pants.backend.python.tasks.python_execution_task_base import PythonExecutionTaskBase
+from pants.util.memo import memoized_classproperty
 
 
 class PytestPrep(PythonExecutionTaskBase):
-  """Prepares a PEX binary for the current test context with `py.test` as its entry-point."""
+  """Prepares a PEX binary for the current test context with `pytest` as its entry-point."""
 
   class PytestBinary(object):
-    """A `py.test` PEX binary with an embedded default (empty) `pytest.ini` config file."""
+    """A `pytest` PEX binary with an embedded default (empty) `pytest.ini` config file."""
 
-    _COVERAGE_PLUGIN_MODULE_NAME = '__{}__'.format(__name__.replace('.', '_'))
+    @staticmethod
+    def make_plugin_name(name):
+      return '__{}_{}_plugin__'.format(__name__.replace('.', '_'), name)
+
+    @memoized_classproperty
+    def coverage_plugin_module(cls):
+      """Return the name of the coverage plugin module embedded in this pytest binary.
+
+      :rtype: str
+      """
+      return cls.make_plugin_name('coverage')
+
+    @memoized_classproperty
+    def pytest_plugin_module(cls):
+      """Return the name of the pytest plugin module embedded in this pytest binary.
+
+      :rtype: str
+      """
+      return cls.make_plugin_name('pytest')
 
     def __init__(self, interpreter, pex):
       # Here we hack around `coverage.cmdline` nuking the 0th element of `sys.path` (our root pex)
@@ -55,20 +74,12 @@ def interpreter(self):
 
     @property
     def config_path(self):
-      """Return the absolute path of the `pytest.ini` config file in this py.test binary.
+      """Return the absolute path of the `pytest.ini` config file in this pytest binary.
 
       :rtype: str
       """
       return os.path.join(self._pex.path(), 'pytest.ini')
 
-    @classmethod
-    def coverage_plugin_module(cls):
-      """Return the name of the coverage plugin module embedded in this py.test binary.
-
-      :rtype: str
-      """
-      return cls._COVERAGE_PLUGIN_MODULE_NAME
-
   @classmethod
   def implementation_version(cls):
     return super(PytestPrep, cls).implementation_version() + [('PytestPrep', 2)]
@@ -81,13 +92,18 @@ def product_types(cls):
   def subsystem_dependencies(cls):
     return super(PytestPrep, cls).subsystem_dependencies() + (PyTest,)
 
-  def extra_requirements(self):
-    return PyTest.global_instance().get_requirement_strings()
+  @classmethod
+  def _module_resource(cls, module_name, resource_relpath):
+    return cls.ExtraFile(path='{}.py'.format(module_name),
+                         content=pkg_resources.resource_string(__name__, resource_relpath))
 
   def extra_files(self):
     yield self.ExtraFile.empty('pytest.ini')
-    yield self.ExtraFile(path='{}.py'.format(self.PytestBinary.coverage_plugin_module()),
-                         content=pkg_resources.resource_string(__name__, 'coverage/plugin.py'))
+    yield self._module_resource(self.PytestBinary.pytest_plugin_module, 'pytest/plugin.py')
+    yield self._module_resource(self.PytestBinary.coverage_plugin_module, 'coverage/plugin.py')
+
+  def extra_requirements(self):
+    return PyTest.global_instance().get_requirement_strings()
 
   def execute(self):
     if not self.context.targets(lambda t: isinstance(t, PythonTests)):

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

@@ -16,6 +16,8 @@
 from io import StringIO
 from textwrap import dedent
 
+from future.utils import PY3
+
 from pants.backend.python.targets.python_tests import PythonTests
 from pants.backend.python.tasks.gather_sources import GatherSources
 from pants.backend.python.tasks.pytest_prep import PytestPrep
@@ -189,7 +191,7 @@ def _ensure_section(cp, section):
   def _add_plugin_config(cls, cp, src_chroot_path, src_to_target_base):
     # We use a coverage plugin to map PEX chroot source paths back to their original repo paths for
     # report output.
-    plugin_module = PytestPrep.PytestBinary.coverage_plugin_module()
+    plugin_module = PytestPrep.PytestBinary.coverage_plugin_module
     cls._ensure_section(cp, 'run')
     cp.set('run', 'plugins', plugin_module)
 
@@ -355,133 +357,55 @@ def coverage_run(subcommand, arguments):
             coverage_xml = os.path.join(coverage_workdir, 'coverage.xml')
             coverage_run('xml', ['-i', '--rcfile', coverage_rc, '-o', coverage_xml])
 
-  def _get_shard_conftest_content(self):
+  def _get_sharding_args(self):
     shard_spec = self.get_options().test_shard
     if shard_spec is None:
-      return ''
+      return []
 
     try:
       sharder = Sharder(shard_spec)
-      if sharder.nshards < 2:
-        return ''
-      return dedent("""
-
-        ### GENERATED BY PANTS ###
-
-        def pytest_report_header(config):
-          return 'shard: {shard} of {nshards} (0-based shard numbering)'
-
-        def pytest_collection_modifyitems(session, config, items):
-          total_count = len(items)
-          removed = 0
-          def is_conftest(itm):
-            return itm.fspath and itm.fspath.basename == 'conftest.py'
-          for i, item in enumerate(list(x for x in items if not is_conftest(x))):
-            if i % {nshards} != {shard}:
-              del items[i - removed]
-              removed += 1
-          reporter = config.pluginmanager.getplugin('terminalreporter')
-          reporter.write_line('Only executing {{}} of {{}} total tests in shard {shard} of '
-                              '{nshards}'.format(total_count - removed, total_count),
-                              bold=True, invert=True, yellow=True)
-        """.format(shard=sharder.shard, nshards=sharder.nshards))
+      return [
+        '--pants-shard', '{}'.format(sharder.shard),
+        '--pants-num-shards', '{}'.format(sharder.nshards)
+      ]
     except Sharder.InvalidShardSpec as e:
       raise self.InvalidShardSpecification(e)
 
-  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
-
   @contextmanager
-  def _conftest(self, sources_map):
-    """Creates a conftest.py to customize our pytest run."""
+  def _pants_pytest_plugin_args(self, sources_map):
+    """Configures the pants pytest plugin to customize our pytest run."""
     # Note that it's important to put the tmpdir under the workdir, because pytest
     # uses all arguments that look like paths to compute its rootdir, and we want
     # it to pick the buildroot.
-    with temporary_dir(root_dir=self.workdir) as conftest_dir:
-      rootdir_comm_path = os.path.join(conftest_dir, 'pytest_rootdir.path')
+    with temporary_dir(root_dir=self.workdir) as comm_dir:
+      rootdir_comm_path = os.path.join(comm_dir, 'pytest_rootdir.path')
 
       def get_pytest_rootdir():
         with open(rootdir_comm_path, 'r') as fp:
           return fp.read()
 
-      conftest_content = self._get_conftest_content(sources_map,
-                                                    rootdir_comm_path=rootdir_comm_path)
+      sources_map_path = os.path.join(comm_dir, 'sources_map.json')
+      mode = 'w' if PY3 else 'wb'
+      with open(sources_map_path, mode) as fp:
+        json.dump(sources_map, fp)
+
+      renaming_args = [
+        '--pants-rootdir-comm-path', rootdir_comm_path,
+        '--pants-sources-map-path', sources_map_path
+      ]
 
-      conftest = os.path.join(conftest_dir, 'conftest.py')
-      with open(conftest, 'w') as fp:
-        fp.write(conftest_content)
-      yield conftest, get_pytest_rootdir
+      yield renaming_args + self._get_sharding_args(), get_pytest_rootdir
 
   @contextmanager
   def _test_runner(self, workdirs, test_targets, sources_map):
     pytest_binary = self.context.products.get_data(PytestPrep.PytestBinary)
-    with self._conftest(sources_map) as (conftest, get_pytest_rootdir):
+    with self._pants_pytest_plugin_args(sources_map) as (plugin_args, get_pytest_rootdir):
       with self._maybe_emit_coverage_data(workdirs,
                                           test_targets,
                                           pytest_binary.pex) as coverage_args:
-        yield pytest_binary, [conftest] + coverage_args, get_pytest_rootdir
+        yield (pytest_binary,
+               ['-p', pytest_binary.pytest_plugin_module] + plugin_args + coverage_args,
+               get_pytest_rootdir)
 
   def _constrain_pytest_interpreter_search_path(self):
     """Return an environment for invoking a pex which ensures the use of the selected interpreter.