Deprecate PytestRun env vars.

This adds standard @deprecated support for any access to env vars in
place of the equivalent options via a class decorator that generates a
pair of methods for each env/replacement-option pair.

Along the way it fixes a bug where `--coverage` was being exported but
not honored.

Testing Done:
Manually tested that advancing the version to `0.0.35` triggered
the expected error when running:
```console
$ PANTS_DEV=1 ./pants compile compile src/python/pants/backend/python/tasks/:python
```

CI went green here:
  https://travis-ci.org/pantsbuild/pants/builds/64960654

Bugs closed: 1618

Reviewed at https://rbcommons.com/s/twitter/r/2299/

build-support/bin/ci.sh

@@ -153,8 +153,8 @@ fi
 if [[ "${skip_internal_backends:-false}" == "false" ]]; then
   banner "Running internal backend python tests"
   (
-    PANTS_PYTHON_TEST_FAILSOFT=1 \
-      ./pants.pex ${PANTS_ARGS[@]} test \
+    ./pants.pex ${PANTS_ARGS[@]} test.pytest \
+      --fail-slow \
         $(./pants.pex list pants-plugins/tests/python:: | \
             xargs ./pants.pex filter --filter-type=python_tests)
   ) || die "Internal backend python test failure"
@@ -166,9 +166,10 @@ if [[ "${skip_python:-false}" == "false" ]]; then
   fi
   banner "Running core python tests${shard_desc}"
   (
-    PANTS_PY_COVERAGE=paths:pants/ \
-      PANTS_PYTHON_TEST_FAILSOFT=1 \
-      ./pants.pex ${PANTS_ARGS[@]} test.pytest --shard=${python_unit_shard} \
+    ./pants.pex ${PANTS_ARGS[@]} test.pytest \
+      --fail-slow \
+      --coverage=paths:pants/ \
+      --shard=${python_unit_shard} \
         $(./pants.pex list tests/python:: | \
             xargs ./pants.pex filter --filter-type=python_tests | \
             grep -v integration)
@@ -182,7 +183,7 @@ if [[ "${skip_contrib:-false}" == "false" ]]; then
     # test (ie: pants_test.contrib) namespace packages.
     # TODO(John Sirois): Get to the bottom of the issue and kill --no-fast, see:
     #  https://github.com/pantsbuild/pants/issues/1149
-    PANTS_PYTHON_TEST_FAILSOFT=1 ./pants.pex ${PANTS_ARGS[@]} test.pytest --no-fast contrib::
+    ./pants.pex ${PANTS_ARGS[@]} test.pytest --fail-slow --no-fast contrib::
   ) || die "Contrib python test failure"
 fi
 
@@ -192,11 +193,10 @@ if [[ "${skip_integration:-false}" == "false" ]]; then
   fi
   banner "Running Pants Integration tests${shard_desc}"
   (
-    PANTS_PYTHON_TEST_FAILSOFT=1 \
-      ./pants.pex ${PANTS_ARGS[@]} test.pytest --shard=${python_intg_shard} \
-        $(./pants.pex list tests/python:: | \
-            xargs ./pants.pex filter --filter-type=python_tests | \
-            grep integration)
+    ./pants.pex ${PANTS_ARGS[@]} test.pytest --fail-slow --shard=${python_intg_shard} \
+      $(./pants.pex list tests/python:: | \
+          xargs ./pants.pex filter --filter-type=python_tests | \
+          grep integration)
   ) || die "Pants Integration test failure"
 fi
 

examples/src/python/example/readme.md

@@ -331,11 +331,11 @@ parameters:
 
 ### Code Coverage
 
-To get code coverage data, set the `PANTS_PY_COVERAGE` environment variable. If you don't
-configured coverage data, it doesn't do much:
+To get code coverage data, set the `--coverage` flag in `test.pytest` scope.
+If you haven't configured coverage data, it doesn't do much:
 
     :::bash
-    $ PANTS_PY_COVERAGE=1 ./pants test examples/tests/python/example_test/hello/greet:greet
+    $ ./pants test.pytest --coverage=1 examples/tests/python/example_test/hello/greet:greet
         ...lots of build output...
                          ============ 2 passed in 0.23 seconds ============
                          Name    Stmts   Miss  Cover
@@ -357,12 +357,11 @@ There are 2 alternatives to specifying coverage attributes on all
 attributes in-play to form a global coverage specification for the test
 run.
 
-`PANTS_PY_COVERAGE=modules:[module1](,...,[moduleN])` allows
-specification of package or module names to track coverage against. For
-example:
+`--coverage=modules:[module1](,...,[moduleN])` allows specification of
+package or module names to track coverage against. For example:
 
     :::bash
-    $ PANTS_PY_COVERAGE=modules:example.hello.greet,example.hello.main ./pants test examples/tests/python/example_test/hello/greet:greet
+    $ ./pants test.pytest --coverage=modules:example.hello.greet,example.hello.main examples/tests/python/example_test/hello/greet:greet
         ...lots of build output...
                      ============ 2 passed in 0.22 seconds ============
                      Name                                               Stmts   Miss Branch BrMiss  Cover
@@ -379,7 +378,7 @@ Similarly, a set of base paths can be specified containing the code for
 coverage to be measured over:
 
     :::bash
-    $ PANTS_PY_COVERAGE=paths:example/hello ./pants test examples/tests/python/example_test/hello/greet:greet
+    $ ./pants test.pytest --coverage=paths:example/hello examples/tests/python/example_test/hello/greet:greet
         ...lots of build output...
                      ============ 2 passed in 0.23 seconds ============
                      Name                                               Stmts   Miss Branch BrMiss  Cover

src/docs/invoking.md

@@ -91,8 +91,6 @@ to Pants.
 * `PANTS_IVY_CACHE_DIR`
 * `PANTS_IVY_SETTINGS_XML`
 * `PANTS_LEAVE_CHROOT`
-* `PANTS_PY_COVERAGE`
-* `PANTS_PYTHON_TEST_FAILSOFT`
 * `PANTS_VERBOSE`
 
 ### `pants.ini` Settings File

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

@@ -23,6 +23,7 @@ python_library(
     'src/python/pants/base:address_lookup_error',
     'src/python/pants/base:build_environment',
     'src/python/pants/base:build_graph',
+    'src/python/pants/base:deprecated',
     'src/python/pants/base:exceptions',
     'src/python/pants/base:generator',
     'src/python/pants/base:target',

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

@@ -25,6 +25,7 @@
 from pants.backend.python.python_setup import PythonRepos, PythonSetup
 from pants.backend.python.targets.python_tests import PythonTests
 from pants.backend.python.tasks.python_task import PythonTask
+from pants.base.deprecated import deprecated
 from pants.base.exceptions import TaskError, TestFailedTaskError
 from pants.base.target import Target
 from pants.base.workunit import WorkUnit
@@ -67,6 +68,43 @@ def failed_targets(self):
     return self._failed_targets
 
 
+def deprecated_env_accessors(removal_version, **replacement_mapping):
+  """Generates accessors for legacy env ver/replacement option pairs.
+
+  The generated accessors issue a deprecation warning when the deprecated env var is present and
+  enjoy the "compile" time removal forcing that normal @deprecated functions and methods do.
+  """
+  def create_accessor(env_name, option_name):
+    @deprecated(removal_version=removal_version,
+                hint_message='Use the {option} option instead of the deprecated {env} environment '
+                             'variable'.format(option=option_name, env=env_name))
+    def deprecated_accessor():
+      return os.environ.get(env_name)
+
+    def accessor(self):
+      value = None
+      if env_name in os.environ:
+        value = deprecated_accessor()
+      sanitized_option_name = option_name.lstrip('-').replace('-', '_')
+      value = self.get_options()[sanitized_option_name] or value
+      return value
+    return accessor
+
+  def decorator(clazz):
+    for env_name, option_name in replacement_mapping.items():
+      setattr(clazz, 'get_DEPRECATED_{}'.format(env_name), create_accessor(env_name, option_name))
+    return clazz
+
+  return decorator
+
+
+# TODO(John Sirois): Replace this helper and use of the accessors it generates with direct options
+# access prior to releasing 0.0.35
+@deprecated_env_accessors(removal_version='0.0.35',
+                          JUNIT_XML_BASE='--junit-xml-dir',
+                          PANTS_PROFILE='--profile',
+                          PANTS_PYTHON_TEST_FAILSOFT='--fail-slow',
+                          PANTS_PY_COVERAGE='--coverage')
 class PytestRun(PythonTask):
   _TESTING_TARGETS = [
     # Note: the requirement restrictions on pytest and pytest-cov match those in requirements.txt,
@@ -90,6 +128,17 @@ def register_options(cls, register):
              help='Run all tests in a single chroot. If turned off, each test target will '
                   'create a new chroot, which will be much slower, but more correct, as the'
                   'isolation verifies that all dependencies are correctly declared.')
+    register('--fail-slow', action='store_true', default=False,
+             help='Do not fail fast on the first test failure in a suite; instead run all tests '
+                  'and report errors only after all tests complete.')
+    register('--junit-xml-dir', metavar='<DIR>',
+             help='Specifying a directory causes junit xml results files to be emitted under '
+                  'that dir for each test run.')
+    register('--profile', metavar='<FILE>',
+             help="Specifying a file path causes tests to be profiled with the profiling data "
+                  "emitted to that file (prefix). Note that tests may run in a different cwd, so "
+                  "it's best to use an absolute path to make it easy to find the subprocess "
+                  "profiles later.")
     register('--options', action='append', help='Pass these options to pytest.')
     register('--coverage',
              help='Emit coverage information for specified paths/modules. Value has two forms: '
@@ -104,10 +153,6 @@ def supports_passthru_args(cls):
 
   def execute(self):
     def is_python_test(target):
-      # Note that we ignore PythonTestSuite, because we'll see the PythonTests targets
-      # it depends on anyway,so if we don't we'll end up running the tests twice.
-      # TODO(benjy): Once we're off the 'build' command we can get rid of python_test_suite,
-      # or make it an alias of dependencies().
       return isinstance(target, PythonTests)
 
     test_targets = list(filter(is_python_test, self.context.targets()))
@@ -131,8 +176,8 @@ def run_tests(self, targets, workunit):
     else:
       results = {}
       # Coverage often throws errors despite tests succeeding, so force failsoft in that case.
-      fail_hard = ('PANTS_PYTHON_TEST_FAILSOFT' not in os.environ and
-                   'PANTS_PY_COVERAGE' not in os.environ)
+      fail_hard = (not self.get_DEPRECATED_PANTS_PYTHON_TEST_FAILSOFT() and
+                   not self.get_DEPRECATED_PANTS_PY_COVERAGE())
       for target in targets:
         if isinstance(target, PythonTests):
           rv = self._do_run_tests([target], workunit)
@@ -203,7 +248,7 @@ def pytest_collection_modifyitems(session, config, items):
   @contextmanager
   def _maybe_emit_junit_xml(self, targets):
     args = []
-    xml_base = os.getenv('JUNIT_XML_BASE')
+    xml_base = self.get_DEPRECATED_JUNIT_XML_BASE()
     if xml_base and targets:
       xml_base = os.path.realpath(xml_base)
       xml_path = os.path.join(xml_base, Target.maybe_readable_identify(targets) + '.xml')
@@ -332,7 +377,7 @@ def is_python_lib(tgt):
 
   @contextmanager
   def _maybe_emit_coverage_data(self, targets, chroot, pex, workunit):
-    coverage = os.environ.get('PANTS_PY_COVERAGE')
+    coverage = self.get_DEPRECATED_PANTS_PY_COVERAGE()
     if coverage is None:
       yield []
       return
@@ -432,12 +477,9 @@ def _do_run_tests_with_args(self, pex, workunit, args):
       env = {
         'PYTHONUNBUFFERED': '1',
       }
-      # If profiling a test run, this will enable profiling on the test code itself.
-      # Note that tests may run in a different cwd, so it's best to set PANTS_PROFILE
-      # to an absolute path to make it easy to find the subprocess profiles later.
-      if 'PANTS_PROFILE' in os.environ:
-        env['PEX_PROFILE'] = '{0}.subprocess.{1:.6f}'.format(os.environ['PANTS_PROFILE'],
-                                                             time.time())
+      profile = self.get_DEPRECATED_PANTS_PROFILE()
+      if profile:
+        env['PEX_PROFILE'] = '{0}.subprocess.{1:.6f}'.format(profile, time.time())
       with environment_as(**env):
         rc = self._pex_run(pex, workunit, args=args, setsid=True)
         return PythonTestResult.rc(rc)