Fix PytestRun passthru arg handling. (pantsbuild#5594)

Previously passthru arguments were shlex-split even though they were
already presented in list form via `--options` and the passthru args
facility. Add a unit test to confirm passthru args are not over-split
and deprecate `--options` in favor of the standard passthru args
facility.

.travis.yml

@@ -3,6 +3,7 @@ env:
     - PANTS_CONFIG_FILES="${TRAVIS_BUILD_DIR}/pants.travis-ci.ini"
     - ANDROID_SDK_INSTALL_LOCATION="${HOME}/opt/android-sdk-install"
     - ANDROID_HOME="$ANDROID_SDK_INSTALL_LOCATION/android-sdk-linux"
+    - PYTEST_PASSTHRU_ARGS="-v --duration=3"
 
 before_cache:
   # Ensure permissions to do the below removals, which happen with or without caching enabled.

build-support/bin/ci.sh

@@ -185,7 +185,7 @@ if [[ "${skip_internal_backends:-false}" == "false" ]]; then
   start_travis_section "BackendTests" "Running internal backend python tests"
   (
     ./pants.pex ${PANTS_ARGS[@]} test.pytest \
-    pants-plugins/tests/python::
+    pants-plugins/tests/python:: -- ${PYTEST_PASSTHRU_ARGS}
   ) || die "Internal backend python test failure"
   end_travis_section
 fi
@@ -198,7 +198,7 @@ if [[ "${skip_python:-false}" == "false" ]]; then
   (
     ./pants.pex --tag='-integration' ${PANTS_ARGS[@]} test.pytest --chroot \
       --test-pytest-test-shard=${python_unit_shard} \
-      tests/python::
+      tests/python:: -- ${PYTEST_PASSTHRU_ARGS}
   ) || die "Core python test failure"
   end_travis_section
 fi
@@ -212,7 +212,7 @@ if [[ "${skip_contrib:-false}" == "false" ]]; then
     ./pants.pex ${PANTS_ARGS[@]} --exclude-target-regexp='.*/testprojects/.*' \
     --build-ignore=$SKIP_ANDROID_PATTERN test.pytest \
     --test-pytest-test-shard=${python_contrib_shard} \
-    contrib:: \
+    contrib:: -- ${PYTEST_PASSTHRU_ARGS}
   ) || die "Contrib python test failure"
   end_travis_section
 fi
@@ -237,7 +237,7 @@ if [[ "${skip_integration:-false}" == "false" ]]; then
   (
     ./pants.pex ${PANTS_ARGS[@]} --tag='+integration' test.pytest \
       --test-pytest-test-shard=${python_intg_shard} \
-      tests/python::
+      tests/python:: -- ${PYTEST_PASSTHRU_ARGS}
   ) || die "Pants Integration test failure"
   end_travis_section
 fi

pants.travis-ci.ini

@@ -11,7 +11,5 @@ use_nailgun: False
 worker_count: 4
 
 [test.pytest]
-options: ['--duration=3']
-
 # Increase isolation. This is the direction that all tests will head in soon via #4586.
 fast: false

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

@@ -35,7 +35,6 @@
 from pants.util.memo import memoized_method, memoized_property
 from pants.util.objects import datatype
 from pants.util.process_handler import SubprocessProcessHandler
-from pants.util.strutil import safe_shlex_split
 from pants.util.xml_parser import XmlParser
 
 
@@ -111,7 +110,12 @@ def register_options(cls, register):
                   "it's best to use an absolute path to make it easy to find the subprocess "
                   "profiles later.")
 
-    register('--options', type=list, fingerprint=True, help='Pass these options to pytest.')
+    register('--options', type=list, fingerprint=True,
+             removal_version='1.7.0.dev0',
+             removal_hint='You can supply py.test options using the generic pass through the args '
+                          'facility. At the end of the pants command line, add `-- <py.test pass'
+                          'through args>`.',
+             help='Pass these options to pytest.')
 
     register('--coverage', fingerprint=True,
              help='Emit coverage information for specified packages or directories (absolute or '
@@ -676,8 +680,10 @@ def _run_pytest(self, fail_fast, test_targets, workdirs):
         args.extend(['-s'])
       if self.get_options().colors:
         args.extend(['--color', 'yes'])
-      for options in self.get_options().options + self.get_passthru_args():
-        args.extend(safe_shlex_split(options))
+
+      args.extend(self.get_options().options)
+      args.extend(self.get_passthru_args())
+
       args.extend(test_args)
       args.extend(sources_map.keys())
 

tests/python/pants_test/backend/python/tasks/test_pytest_run.py

@@ -5,7 +5,9 @@
 from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
                         unicode_literals, with_statement)
 
+import functools
 import os
+from contextlib import contextmanager
 from textwrap import dedent
 
 import coverage
@@ -700,3 +702,105 @@ def test_sharding_invalid_shard_bad_format(self):
 
     with self.assertRaises(PytestRun.InvalidShardSpecification):
       self.run_tests(targets=[self.green], test_shard='1/a')
+
+  @contextmanager
+  def marking_tests(self):
+    init_subsystem(Target.Arguments)
+    init_subsystem(SourceRootConfig)
+
+    with temporary_dir() as marker_dir:
+      self.create_file(
+        'test/python/passthru/test_passthru.py',
+        dedent("""
+            import inspect
+            import os
+            import pytest
+            import unittest
+  
+  
+            class PassthruTest(unittest.TestCase):
+              def touch(self, path):
+                with open(path, 'wb') as fp:
+                  fp.close()
+                  
+              def mark_test_run(self):
+                caller_frame_record = inspect.stack()[1]
+                
+                # For the slot breakdown of a frame record tuple, see:
+                #   https://docs.python.org/2/library/inspect.html#the-interpreter-stack
+                _, _, _, caller_func_name, _, _ = caller_frame_record
+                
+                marker_file = os.path.join({marker_dir!r}, caller_func_name)
+                self.touch(marker_file)
+  
+              def test_one(self):
+                self.mark_test_run()
+  
+              @pytest.mark.purple
+              def test_two(self):
+                self.mark_test_run()
+  
+              def test_three(self):
+                self.mark_test_run()
+                
+              @pytest.mark.red
+              def test_four(self):
+                self.mark_test_run()
+              
+              @pytest.mark.green
+              def test_five(self):
+                self.mark_test_run()                
+          """.format(marker_dir=marker_dir)))
+
+      def assert_mark(exists, name):
+        message = '{} {!r} to be executed.'.format('Expected' if exists else 'Did not expect', name)
+        marker_file = os.path.join(marker_dir, name)
+        self.assertEqual(exists, os.path.exists(marker_file), message)
+
+      test = self.make_target(spec='test/python/passthru', target_type=PythonTests)
+      yield test, functools.partial(assert_mark, True), functools.partial(assert_mark, False)
+
+  def test_passthrough_args_facility_single_style(self):
+    with self.marking_tests() as (target, assert_test_run, assert_test_not_run):
+      self.run_tests([target], '-ktest_one or test_two')
+      assert_test_run('test_one')
+      assert_test_run('test_two')
+      assert_test_not_run('test_three')
+      assert_test_not_run('test_four')
+      assert_test_not_run('test_five')
+
+  def test_passthrough_args_facility_plus_arg_style(self):
+    with self.marking_tests() as (target, assert_test_run, assert_test_not_run):
+      self.run_tests([target], '-m', 'purple or red')
+      assert_test_not_run('test_one')
+      assert_test_run('test_two')
+      assert_test_not_run('test_three')
+      assert_test_run('test_four')
+      assert_test_not_run('test_five')
+
+  def test_passthrough_args_legacy_option_single_style(self):
+    with self.marking_tests() as (target, assert_test_run, assert_test_not_run):
+      self.run_tests([target], options=['-ktest_two or test_three'])
+      assert_test_not_run('test_one')
+      assert_test_run('test_two')
+      assert_test_run('test_three')
+      assert_test_not_run('test_four')
+      assert_test_not_run('test_five')
+
+  def test_passthrough_args_legacy_option_plus_arg_style(self):
+    with self.marking_tests() as (target, assert_test_run, assert_test_not_run):
+      self.run_tests([target], options=['-m', 'red or green'])
+      assert_test_not_run('test_one')
+      assert_test_not_run('test_two')
+      assert_test_not_run('test_three')
+      assert_test_run('test_four')
+      assert_test_run('test_five')
+
+  def test_passthrough_args_combined(self):
+    with self.marking_tests() as (target, assert_test_run, assert_test_not_run):
+      self.run_tests([target], '-mgreen or purple', options=['-ktest_two or test_three'])
+      assert_test_not_run('test_one')
+      assert_test_run('test_two')
+      assert_test_not_run('test_three')
+      assert_test_not_run('test_four')
+      assert_test_not_run('test_five')

tests/python/pants_test/backend/python/tasks/test_pytest_run_integration.py

@@ -20,9 +20,10 @@ class PytestRunIntegrationTest(PantsRunIntegrationTest):
   def test_pytest_run_timeout_succeeds(self):
     pants_run = self.run_pants(['clean-all',
                                 'test.pytest',
-                                '--test-pytest-options=-k within_timeout',
                                 '--timeout-default=2',
-                                'testprojects/tests/python/pants/timeout:exceeds_timeout'])
+                                'testprojects/tests/python/pants/timeout:exceeds_timeout',
+                                '--',
+                                '-kwithin_timeout'])
     self.assert_success(pants_run)
 
   def test_pytest_run_conftest_succeeds(self):
@@ -35,11 +36,12 @@ def test_pytest_run_timeout_fails(self):
     start = time.time()
     pants_run = self.run_pants(['clean-all',
                                 'test.pytest',
-                                '--test-pytest-coverage=auto',
-                                '--test-pytest-options=-k exceeds_timeout',
-                                '--test-pytest-timeout-default=1',
-                                '--cache-test-pytest-ignore',
-                                'testprojects/tests/python/pants/timeout:exceeds_timeout'])
+                                '--coverage=auto',
+                                '--timeout-default=1',
+                                '--cache-ignore',
+                                'testprojects/tests/python/pants/timeout:exceeds_timeout',
+                                '--',
+                                '-kexceeds_timeout'])
     end = time.time()
     self.assert_failure(pants_run)
 
@@ -56,10 +58,10 @@ def test_pytest_run_timeout_cant_terminate(self):
     start = time.time()
     pants_run = self.run_pants(['clean-all',
                                 'test.pytest',
-                                '--test-pytest-timeout-terminate-wait=2',
-                                '--test-pytest-timeout-default=1',
-                                '--test-pytest-coverage=auto',
-                                '--cache-test-pytest-ignore',
+                                '--timeout-terminate-wait=2',
+                                '--timeout-default=1',
+                                '--coverage=auto',
+                                '--cache-ignore',
                                 'testprojects/tests/python/pants/timeout:ignores_terminate'])
     end = time.time()
     self.assert_failure(pants_run)
@@ -81,7 +83,7 @@ def test_pytest_explicit_coverage(self):
     with temporary_dir(cleanup=False) as coverage_dir:
       pants_run = self.run_pants(['clean-all',
                                   'test.pytest',
-                                  '--test-pytest-coverage=pants',
+                                  '--coverage=pants',
                                   '--test-pytest-coverage-output-dir={}'.format(coverage_dir),
                                   'testprojects/tests/python/pants/constants_only'])
       self.assert_success(pants_run)