Deprecate the subsystem_instance utility function.

It was implemented as a contextmanager that resets subsystem
state in __exit__.  However this interacts badly with tests that
use that state (sometimes without even realizing it) outside the
subsystem_instance context. Several tests have to do weird
balancing acts to get around this.

Instead we introduce a global_subsystem_instance() that simply
inits and returns the global instance of a subsystem. Test code
can rely on our standard test base classes for cleanup between tests,
or it can reset subsystem state itself if it needs to do so mid-test.

This change allows us to remove various wrapper contextmanagers
sprinkled around our tests, and generally supports simplification
of several tests.

This is part 2 of my effort to simplify and standardize how we
create subsystems in tests.  See for part 1:

44be4da

Note to reviewers: The diff is a little hairy, but the focus is on
the change in tests/python/pants_test/subsystem/subsystem_util.py.
The rest is just modifying all relevant tests to use the new method,
and the proof of the validity of those is mostly that the tests pass...

Testing Done:
CI passes: https://travis-ci.org/pantsbuild/pants/builds/159169092

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

contrib/android/tests/python/pants_test/contrib/android/android_integration_test.py

@@ -9,7 +9,7 @@
 
 from pants.java.distribution.distribution import Distribution, DistributionLocator
 from pants_test.pants_run_integration_test import PantsRunIntegrationTest
-from pants_test.subsystem.subsystem_util import subsystem_instance
+from pants_test.subsystem.subsystem_util import global_subsystem_instance
 
 
 class AndroidIntegrationTest(PantsRunIntegrationTest):
@@ -38,8 +38,8 @@ def requirements(cls, tools):
     else:
       return False
     try:
-      with subsystem_instance(DistributionLocator) as locator:
-        locator.cached(minimum_version=cls.JAVA_MIN, maximum_version=cls.JAVA_MAX)
+      locator = global_subsystem_instance(DistributionLocator)
+      locator.cached(minimum_version=cls.JAVA_MIN, maximum_version=cls.JAVA_MAX)
     except Distribution.Error:
       return False
     return True

contrib/go/tests/python/pants_test/contrib/go/subsystems/test_fetchers.py

@@ -5,23 +5,20 @@
 from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
                         unicode_literals, with_statement)
 
-from contextlib import contextmanager
-
 from pants_test import base_test
-from pants_test.subsystem.subsystem_util import subsystem_instance
+from pants_test.subsystem.subsystem_util import global_subsystem_instance
 
 from pants.contrib.go.subsystems.fetcher_factory import FetcherFactory
 
 
 class FetchersTest(base_test.BaseTest):
-  @contextmanager
   def fetcher(self, import_path):
-    with subsystem_instance(FetcherFactory) as fetcher_factory:
-      yield fetcher_factory.get_fetcher(import_path)
+    fetcher_factory = global_subsystem_instance(FetcherFactory)
+    return fetcher_factory.get_fetcher(import_path)
 
   def check_default(self, import_path, expected_root):
-    with self.fetcher(import_path) as fetcher:
-      self.assertEqual(expected_root, fetcher.root())
+    fetcher = self.fetcher(import_path)
+    self.assertEqual(expected_root, fetcher.root())
 
   def test_default_bitbucket(self):
     self.check_default('bitbucket.org/rj/sqlite3-go',

contrib/go/tests/python/pants_test/contrib/go/subsystems/test_go_distribution.py

@@ -7,36 +7,34 @@
 
 import os
 import unittest
-from contextlib import contextmanager
 
 from pants.util.contextutil import environment_as
-from pants_test.subsystem.subsystem_util import subsystem_instance
+from pants_test.subsystem.subsystem_util import global_subsystem_instance
 
 from pants.contrib.go.subsystems.go_distribution import GoDistribution
 
 
 class GoDistributionTest(unittest.TestCase):
 
-  @contextmanager
   def distribution(self):
-    with subsystem_instance(GoDistribution.Factory) as factory:
-      yield factory.create()
+    factory = global_subsystem_instance(GoDistribution.Factory)
+    return factory.create()
 
   def test_bootstrap(self):
-    with self.distribution() as go_distribution:
-      go_cmd = go_distribution.create_go_cmd(cmd='env', args=['GOROOT'])
-      output = go_cmd.check_output()
-      self.assertEqual(go_distribution.goroot, output.strip())
+    go_distribution = self.distribution()
+    go_cmd = go_distribution.create_go_cmd(cmd='env', args=['GOROOT'])
+    output = go_cmd.check_output()
+    self.assertEqual(go_distribution.goroot, output.strip())
 
   def assert_no_gopath(self):
-    with self.distribution() as go_distribution:
-      go_cmd = go_distribution.create_go_cmd(cmd='env', args=['GOPATH'])
+    go_distribution = self.distribution()
+    go_cmd = go_distribution.create_go_cmd(cmd='env', args=['GOPATH'])
 
-      self.assertEqual({'GOROOT': go_distribution.goroot, 'GOPATH': ''}, go_cmd.env)
-      self.assertEqual('go', os.path.basename(go_cmd.cmdline[0]))
-      self.assertEqual(['env', 'GOPATH'], go_cmd.cmdline[1:])
-      self.assertRegexpMatches(str(go_cmd), r'^GOROOT=[^ ]+ GOPATH= .*/go env GOPATH')
-      self.assertEqual('', go_cmd.check_output().strip())
+    self.assertEqual({'GOROOT': go_distribution.goroot, 'GOPATH': ''}, go_cmd.env)
+    self.assertEqual('go', os.path.basename(go_cmd.cmdline[0]))
+    self.assertEqual(['env', 'GOPATH'], go_cmd.cmdline[1:])
+    self.assertRegexpMatches(str(go_cmd), r'^GOROOT=[^ ]+ GOPATH= .*/go env GOPATH')
+    self.assertEqual('', go_cmd.check_output().strip())
 
   def test_go_command_no_gopath(self):
     self.assert_no_gopath()
@@ -49,11 +47,11 @@ def test_go_command_no_gopath_overrides_user_gopath_issue2321(self):
       self.assert_no_gopath()
 
   def test_go_command_gopath(self):
-    with self.distribution() as go_distribution:
-      go_cmd = go_distribution.create_go_cmd(cmd='env', gopath='/tmp/fred', args=['GOROOT'])
-
-      self.assertEqual({'GOROOT': go_distribution.goroot,
-                        'GOPATH': '/tmp/fred'}, go_cmd.env)
-      self.assertEqual('go', os.path.basename(go_cmd.cmdline[0]))
-      self.assertEqual(['env', 'GOROOT'], go_cmd.cmdline[1:])
-      self.assertRegexpMatches(str(go_cmd), r'^GOROOT=[^ ]+ GOPATH=/tmp/fred .*/go env GOROOT$')
+    go_distribution = self.distribution()
+    go_cmd = go_distribution.create_go_cmd(cmd='env', gopath='/tmp/fred', args=['GOROOT'])
+
+    self.assertEqual({'GOROOT': go_distribution.goroot,
+                      'GOPATH': '/tmp/fred'}, go_cmd.env)
+    self.assertEqual('go', os.path.basename(go_cmd.cmdline[0]))
+    self.assertEqual(['env', 'GOROOT'], go_cmd.cmdline[1:])
+    self.assertRegexpMatches(str(go_cmd), r'^GOROOT=[^ ]+ GOPATH=/tmp/fred .*/go env GOROOT$')

contrib/go/tests/python/pants_test/contrib/go/tasks/test_go_compile_integration.py

@@ -8,7 +8,7 @@
 import os
 
 from pants_test.pants_run_integration_test import PantsRunIntegrationTest
-from pants_test.subsystem.subsystem_util import subsystem_instance
+from pants_test.subsystem.subsystem_util import global_subsystem_instance
 from pants_test.testutils.file_test_util import contains_exact_files
 
 from pants.contrib.go.subsystems.go_distribution import GoDistribution
@@ -22,16 +22,16 @@ def test_go_compile_simple(self):
               'contrib/go/examples/src/go/libA']
       pants_run = self.run_pants_with_workdir(args, workdir)
       self.assert_success(pants_run)
-      with subsystem_instance(GoDistribution.Factory) as factory:
-        go_dist = factory.create()
-        goos = go_dist.create_go_cmd('env', args=['GOOS']).check_output().strip()
-        goarch = go_dist.create_go_cmd('env', args=['GOARCH']).check_output().strip()
-        expected_files = set('contrib.go.examples.src.go.{libname}.{libname}/'
-                             'pkg/{goos}_{goarch}/{libname}.a'
-                             .format(libname=libname, goos=goos, goarch=goarch)
-                             for libname in ('libA', 'libB', 'libC', 'libD', 'libE'))
-        self.assertTrue(contains_exact_files(os.path.join(workdir, 'compile', 'go'),
-                                             expected_files, ignore_links=True))
+      factory = global_subsystem_instance(GoDistribution.Factory)
+      go_dist = factory.create()
+      goos = go_dist.create_go_cmd('env', args=['GOOS']).check_output().strip()
+      goarch = go_dist.create_go_cmd('env', args=['GOARCH']).check_output().strip()
+      expected_files = set('contrib.go.examples.src.go.{libname}.{libname}/'
+                           'pkg/{goos}_{goarch}/{libname}.a'
+                           .format(libname=libname, goos=goos, goarch=goarch)
+                           for libname in ('libA', 'libB', 'libC', 'libD', 'libE'))
+      self.assertTrue(contains_exact_files(os.path.join(workdir, 'compile', 'go'),
+                                           expected_files, ignore_links=True))
 
   def test_go_compile_cgo(self):
     args = ['compile', 'contrib/go/examples/src/go/cgo']

contrib/node/tests/python/pants_test/contrib/node/subsystems/test_node_distribution.py

@@ -9,64 +9,57 @@
 import os
 import subprocess
 import unittest
-from contextlib import contextmanager
 
-from pants_test.subsystem.subsystem_util import subsystem_instance
+from pants_test.subsystem.subsystem_util import global_subsystem_instance
 
 from pants.contrib.node.subsystems.node_distribution import NodeDistribution
 
 
 class NodeDistributionTest(unittest.TestCase):
 
-  @contextmanager
-  def distribution(self):
-    with subsystem_instance(NodeDistribution.Factory) as factory:
-      yield factory.create()
+  def setUp(self):
+    self.distribution = global_subsystem_instance(NodeDistribution.Factory).create()
 
   def test_bootstrap(self):
-    with self.distribution() as node_distribution:
-      node_cmd = node_distribution.node_command(args=['--version'])
-      output = node_cmd.check_output()
-      self.assertEqual(node_distribution.version, output.strip())
+    node_cmd = self.distribution.node_command(args=['--version'])
+    output = node_cmd.check_output()
+    self.assertEqual(self.distribution.version, output.strip())
 
   def test_node(self):
-    with self.distribution() as node_distribution:
-      node_command = node_distribution.node_command(args=['--interactive'])  # Force a REPL session.
-      repl = node_command.run(stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+    node_command = self.distribution.node_command(args=['--interactive'])  # Force a REPL session.
+    repl = node_command.run(stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
 
-      out, err = repl.communicate('console.log("Hello World!")')
-      self.assertEqual('', err)
-      self.assertEqual(0, repl.returncode)
+    out, err = repl.communicate('console.log("Hello World!")')
+    self.assertEqual('', err)
+    self.assertEqual(0, repl.returncode)
 
-      for line in out.splitlines():
-        if line.endswith('Hello World!'):
-          break
-      else:
-        self.fail('Did not find the expected "Hello World!" in the REPL session '
-                  'output:\n{}'.format(out))
+    for line in out.splitlines():
+      if line.endswith('Hello World!'):
+        break
+    else:
+      self.fail('Did not find the expected "Hello World!" in the REPL session '
+                'output:\n{}'.format(out))
 
   def test_npm(self):
-    with self.distribution() as node_distribution:
-      npm_version_flag = node_distribution.npm_command(args=['--version'])
-      raw_version = npm_version_flag.check_output().strip()
+    npm_version_flag = self.distribution.npm_command(args=['--version'])
+    raw_version = npm_version_flag.check_output().strip()
 
-      npm_version_cmd = node_distribution.npm_command(args=['version', '--json'])
-      versions_json = npm_version_cmd.check_output()
-      versions = json.loads(versions_json)
+    npm_version_cmd = self.distribution.npm_command(args=['version', '--json'])
+    versions_json = npm_version_cmd.check_output()
+    versions = json.loads(versions_json)
 
-      self.assertEqual(raw_version, versions['npm'])
+    self.assertEqual(raw_version, versions['npm'])
 
   def test_bin_dir_on_path(self):
-    with self.distribution() as node_distribution:
-      node_cmd = node_distribution.node_command(args=['--eval', 'console.log(process.env["PATH"])'])
+    node_cmd = self.distribution.node_command(args=['--eval', 'console.log(process.env["PATH"])'])
 
-      # Test the case in which we do not pass in env,
-      # which should fall back to env=os.environ.copy()
-      output = node_cmd.check_output().strip()
-      self.assertEqual(node_cmd.bin_dir_path, output.split(os.pathsep)[0])
+    # Test the case in which we do not pass in env,
+    # which should fall back to env=os.environ.copy()
+    output = node_cmd.check_output().strip()
+    self.assertEqual(node_cmd.bin_dir_path, output.split(os.pathsep)[0])
 
-      output = node_cmd.check_output(env={'PATH': '/test/path'}).strip()
-      self.assertEqual(node_cmd.bin_dir_path + os.path.pathsep + '/test/path', output)
+    output = node_cmd.check_output(env={'PATH': '/test/path'}).strip()
+    self.assertEqual(node_cmd.bin_dir_path + os.path.pathsep + '/test/path', output)
 
-      output = node_cmd.check_output(env={'PATH': ''}).strip()
-      self.assertEqual(node_cmd.bin_dir_path, output)
+    output = node_cmd.check_output(env={'PATH': ''}).strip()
+    self.assertEqual(node_cmd.bin_dir_path, output)

src/python/pants/build_graph/build_configuration.py

@@ -5,7 +5,6 @@
 from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
                         unicode_literals, with_statement)
 
-import inspect
 import logging
 from collections import Iterable, namedtuple
 
@@ -25,10 +24,6 @@ class BuildConfiguration(object):
 
   ParseState = namedtuple('ParseState', ['registered_addressable_instances', 'parse_globals'])
 
-  @staticmethod
-  def _is_subsystem_type(obj):
-    return inspect.isclass(obj) and issubclass(obj, Subsystem)
-
   def __init__(self):
     self._target_by_alias = {}
     self._target_macro_factory_by_alias = {}
@@ -117,7 +112,7 @@ def register_subsystems(self, subsystems):
     """
     if not isinstance(subsystems, Iterable):
       raise TypeError('The subsystems must be an iterable, given {}'.format(subsystems))
-    invalid_subsystems = [s for s in subsystems if not self._is_subsystem_type(s)]
+    invalid_subsystems = [s for s in subsystems if not Subsystem.is_subsystem_type(s)]
     if invalid_subsystems:
       raise TypeError('The following items from the given subsystems are not Subsystem '
                       'subclasses:\n\t{}'.format('\n\t'.join(str(i) for i in invalid_subsystems)))

src/python/pants/java/distribution/distribution.py

@@ -474,7 +474,7 @@ def _get_stricter_version(a, b, name, stricter):
         return version_a
       return stricter(version_a, version_b)
 
-    # take the tighter constraint of method args and subsystem options
+    # Take the tighter constraint of method args and subsystem options.
     minimum_version = _get_stricter_version(minimum_version,
                                             self._minimum_version,
                                             "minimum_version",

src/python/pants/subsystem/subsystem.py

@@ -5,6 +5,8 @@
 from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
                         unicode_literals, with_statement)
 
+import inspect
+
 from twitter.common.collections import OrderedSet
 
 from pants.option.optionable import Optionable
@@ -54,6 +56,10 @@ def __init__(self, cycle):
           '{} scope: {}'.format(subsystem, subsystem.options_scope) for subsystem in cycle))
       super(Subsystem.CycleException, self).__init__(message)
 
+  @classmethod
+  def is_subsystem_type(cls, obj):
+    return inspect.isclass(obj) and issubclass(obj, cls)
+
   @classmethod
   def scoped(cls, optionable):
     """Returns a dependency on this subsystem, scoped to `optionable`.

tests/python/pants_test/backend/jvm/subsystems/BUILD

@@ -44,6 +44,7 @@ python_tests(
   sources=['test_shader_integration.py'],
   dependencies=[
     'src/python/pants/fs',
+    'src/python/pants/java/distribution',
     'src/python/pants/util:contextutil',
     'tests/python/pants_test/subsystem:subsystem_utils',
     'tests/python/pants_test:int-test',
@@ -58,8 +59,10 @@ python_tests(
     'src/python/pants/backend/jvm/subsystems:jar_dependency_management',
     'src/python/pants/java/distribution',
     'src/python/pants/java:executor',
+    'src/python/pants/subsystem',
     'src/python/pants/util:contextutil',
     'src/python/pants/util:dirutil',
+    'tests/python/pants_test:base_test',
     'tests/python/pants_test/subsystem:subsystem_utils',
   ]
 )