Inject an automatic dep on junit for all junit_tests targets.

It's silly to require an explicit dep.

This required creating a new "JUnit" subsystem.  While there,
I moved the junit runner tool to that subsystem, so the runner
and the library are in the same place.

It's still clunkier than it needs to be to inject deps onto targets,
but that's a problem for another time.

This change removes the explicit junit dep from one of our examples,
to prove that this all works.  A subsequent change will remove any
other unnecessary explicit deps on junit.

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

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

examples/tests/java/org/pantsbuild/example/hello/greet/BUILD

@@ -6,7 +6,6 @@
 junit_tests(name='greet',
   sources=globs('*.java'),
   dependencies=[
-    '3rdparty:junit',
     'examples/src/java/org/pantsbuild/example/hello/greet',
   ],
   resources=[

src/python/pants/backend/jvm/subsystems/BUILD

@@ -34,6 +34,19 @@ python_library(
   ],
 )
 
+python_library(
+  name = 'junit',
+  sources = ['junit.py'],
+  dependencies = [
+    ':jvm_tool_mixin',
+    'src/python/pants/backend/jvm/subsystems:shader',
+    'src/python/pants/backend/jvm/targets:jvm',
+    'src/python/pants/util:memo',
+    'src/python/pants/build_graph',
+    'src/python/pants/subsystem',
+  ],
+)
+
 python_library(
   name = 'jvm',
   sources = ['jvm.py'],

src/python/pants/backend/jvm/subsystems/junit.py

@@ -0,0 +1,69 @@
+# coding=utf-8
+# Copyright 2016 Pants project contributors (see CONTRIBUTORS.md).
+# Licensed under the Apache License, Version 2.0 (see LICENSE).
+
+from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
+                        unicode_literals, with_statement)
+
+from pants.backend.jvm.subsystems.jvm_tool_mixin import JvmToolMixin
+from pants.backend.jvm.subsystems.shader import Shader
+from pants.backend.jvm.targets.jar_dependency import JarDependency
+from pants.backend.jvm.targets.jar_library import JarLibrary
+from pants.build_graph.address import Address
+from pants.subsystem.subsystem import Subsystem
+from pants.util.memo import memoized_method
+
+
+class JUnit(JvmToolMixin, Subsystem):
+  options_scope = 'junit'
+
+  LIBRARY_REV = '4.12'
+  RUNNER_MAIN = 'org.pantsbuild.tools.junit.ConsoleRunner'
+
+  _LIBRARY_JAR = JarDependency(org='junit', name='junit', rev=LIBRARY_REV)
+  _RUNNER_JAR = JarDependency(org='org.pantsbuild', name='junit-runner', rev='1.0.13')
+
+  @classmethod
+  def register_options(cls, register):
+    super(JUnit, cls).register_options(register)
+    cls.register_jvm_tool(register,
+                          'junit_library',
+                          classpath=[
+                            cls._LIBRARY_JAR,
+                          ])
+
+    cls.register_jvm_tool(register,
+                          'junit',
+                          classpath=[
+                            cls._RUNNER_JAR,
+                          ],
+                          main=cls.RUNNER_MAIN,
+                          # TODO(John Sirois): Investigate how much less we can get away with.
+                          # Clearly both tests and the runner need access to the same @Test,
+                          # @Before, as well as other annotations, but there is also the Assert
+                          # class and some subset of the @Rules, @Theories and @RunWith APIs.
+                          custom_rules=[
+                            Shader.exclude_package('junit.framework', recursive=True),
+                            Shader.exclude_package('org.junit', recursive=True),
+                            Shader.exclude_package('org.hamcrest', recursive=True),
+                            Shader.exclude_package('org.pantsbuild.junit.annotations',
+                                                   recursive=True),
+                          ])
+
+  @memoized_method
+  def library_spec(self, buildgraph):
+    """Returns a target spec for the junit library, useable as a dependency.
+
+    :param pants.build_graph.build_graph.BuildGraph buildgraph: buildgraph object.
+    :return: a target spec
+    """
+    junit_addr = Address.parse(self.get_options().junit_library)
+    if not buildgraph.contains_address(junit_addr):
+      buildgraph.inject_synthetic_target(junit_addr, JarLibrary, jars=[self._LIBRARY_JAR],
+                                         scope='forced')
+    return junit_addr.spec
+
+  def runner_classpath(self, context):
+    """Returns an iterable of classpath elements for the runner.
+    """
+    return self.tool_classpath_from_products(context.products, 'junit', self.options_scope)

src/python/pants/backend/jvm/targets/BUILD

@@ -62,6 +62,7 @@ python_library(
     '3rdparty/python:six',
     ':jvm',
     'src/python/pants/backend/jvm/subsystems:java',
+    'src/python/pants/backend/jvm/subsystems:junit',
     'src/python/pants/base:exceptions',
     'src/python/pants/build_graph',
   ],

src/python/pants/backend/jvm/targets/java_tests.py

@@ -5,6 +5,7 @@
 from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
                         unicode_literals, with_statement)
 
+from pants.backend.jvm.subsystems.junit import JUnit
 from pants.backend.jvm.subsystems.jvm_platform import JvmPlatform
 from pants.backend.jvm.targets.jvm_target import JvmTarget
 from pants.base.exceptions import TargetDefinitionException
@@ -27,6 +28,10 @@ class JavaTests(JvmTarget):
                             CONCURRENCY_PARALLEL_METHODS,
                             CONCURRENCY_PARALLEL_CLASSES_AND_METHODS]
 
+  @classmethod
+  def subsystems(cls):
+    return super(JavaTests, cls).subsystems() + (JUnit, )
+
   def __init__(self, cwd=None, test_platform=None, payload=None, timeout=None,
                extra_jvm_options=None, extra_env_vars=None, concurrency=None,
                threads=None, **kwargs):
@@ -85,6 +90,12 @@ def __init__(self, cwd=None, test_platform=None, payload=None, timeout=None,
     # applicable labels - fixup the 'java' misnomer.
     self.add_labels('java', 'tests')
 
+  @property
+  def traversable_dependency_specs(self):
+    for spec in super(JavaTests, self).traversable_dependency_specs:
+      yield spec
+    yield JUnit.global_instance().library_spec(self._build_graph)
+
   @property
   def test_platform(self):
     if self.payload.test_platform:

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

@@ -352,8 +352,8 @@ python_library(
     ':jvm_tool_task_mixin',
     ':reports',
     'src/python/pants/backend/jvm:argfile',
+    'src/python/pants/backend/jvm/subsystems:junit',
     'src/python/pants/backend/jvm/subsystems:jvm_platform',
-    'src/python/pants/backend/jvm/subsystems:shader',
     'src/python/pants/backend/jvm/targets:java',
     'src/python/pants/backend/jvm/targets:jvm',
     'src/python/pants/base:build_environment',

src/python/pants/backend/jvm/tasks/junit_run.py

@@ -17,9 +17,8 @@
 from twitter.common.collections import OrderedSet
 
 from pants.backend.jvm import argfile
+from pants.backend.jvm.subsystems.junit import JUnit
 from pants.backend.jvm.subsystems.jvm_platform import JvmPlatform
-from pants.backend.jvm.subsystems.shader import Shader
-from pants.backend.jvm.targets.jar_dependency import JarDependency
 from pants.backend.jvm.targets.java_tests import JavaTests as junit_tests
 from pants.backend.jvm.targets.jvm_target import JvmTarget
 from pants.backend.jvm.tasks.classpath_util import ClasspathUtil
@@ -207,8 +206,6 @@ class JUnitRun(TestRunnerTaskMixin, JvmToolTaskMixin, JvmTask):
   :API: public
   """
 
-  _MAIN = 'org.pantsbuild.tools.junit.ConsoleRunner'
-
   @classmethod
   def register_options(cls, register):
     super(JUnitRun, cls).register_options(register)
@@ -254,30 +251,13 @@ def register_options(cls, register):
              help='If true, generate an html summary report of tests that were run.')
     register('--open', type=bool,
              help='Attempt to open the html summary report in a browser (implies --html-report)')
-    cls.register_jvm_tool(register,
-                          'junit',
-                          classpath=[
-                            JarDependency(org='org.pantsbuild', name='junit-runner', rev='1.0.13'),
-                          ],
-                          main=JUnitRun._MAIN,
-                          # TODO(John Sirois): Investigate how much less we can get away with.
-                          # Clearly both tests and the runner need access to the same @Test,
-                          # @Before, as well as other annotations, but there is also the Assert
-                          # class and some subset of the @Rules, @Theories and @RunWith APIs.
-                          custom_rules=[
-                            Shader.exclude_package('junit.framework', recursive=True),
-                            Shader.exclude_package('org.junit', recursive=True),
-                            Shader.exclude_package('org.hamcrest', recursive=True),
-                            Shader.exclude_package('org.pantsbuild.junit.annotations',
-                                                   recursive=True),
-                          ])
     # TODO: Yuck, but will improve once coverage steps are in their own tasks.
     for c in [Coverage, Cobertura]:
       c.register_options(register, cls.register_jvm_tool)
 
   @classmethod
   def subsystem_dependencies(cls):
-    return super(JUnitRun, cls).subsystem_dependencies() + (DistributionLocator,)
+    return super(JUnitRun, cls).subsystem_dependencies() + (DistributionLocator, JUnit)
 
   @classmethod
   def request_classes_by_source(cls, test_specs):
@@ -519,7 +499,7 @@ def _run_tests(self, test_registry, output_dir, coverage=None):
         relevant_targets = {test_registry.get_owning_target(t) for t in batch}
         complete_classpath = OrderedSet()
         complete_classpath.update(classpath_prepend)
-        complete_classpath.update(self.tool_classpath('junit'))
+        complete_classpath.update(JUnit.global_instance().runner_classpath(self.context))
         complete_classpath.update(self.classpath(relevant_targets,
                                                  classpath_product=classpath_product))
         complete_classpath.update(classpath_append)
@@ -551,7 +531,7 @@ def _run_tests(self, test_registry, output_dir, coverage=None):
               executor=SubprocessExecutor(distribution),
               distribution=distribution,
               classpath=complete_classpath,
-              main=JUnitRun._MAIN,
+              main=JUnit.RUNNER_MAIN,
               jvm_options=self.jvm_options + extra_jvm_options + list(target_jvm_options),
               args=args + [test.render_test_spec() for test in batch_tests],
               workunit_factory=self.context.new_workunit,
@@ -580,7 +560,7 @@ def _run_tests(self, test_registry, output_dir, coverage=None):
                                                methodname=test.methodname))
       error_message_lines.append(
         '\njava {main} ... exited non-zero ({code}); {failed} failed {targets}.'
-          .format(main=JUnitRun._MAIN, code=result, failed=len(failed_targets),
+          .format(main=JUnit.RUNNER_MAIN, code=result, failed=len(failed_targets),
                   targets=pluralize(len(failed_targets), 'target'))
       )
       raise TestFailedTaskError('\n'.join(error_message_lines), failed_targets=list(failed_targets))

tests/python/pants_test/BUILD

@@ -62,6 +62,7 @@ python_tests(
   sources = ['test_maven_layout.py'],
   dependencies = [
     ':base_test',
+    'src/python/pants/backend/jvm/subsystems:junit',
     'src/python/pants/build_graph',
     'src/python/pants/source',
     'tests/python/pants_test/subsystem:subsystem_utils',

tests/python/pants_test/backend/jvm/targets/BUILD

@@ -33,9 +33,11 @@ python_tests(
   name='java_tests',
   sources=['test_java_tests.py'],
   dependencies=[
+    'src/python/pants/backend/jvm/subsystems:junit',
     'src/python/pants/backend/jvm/targets:jvm',
     'src/python/pants/build_graph',
     'tests/python/pants_test:base_test',
+    'tests/python/pants_test/subsystem:subsystem_utils',
   ],
 )
 

tests/python/pants_test/backend/jvm/targets/test_java_tests.py

@@ -5,15 +5,18 @@
 from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
                         unicode_literals, with_statement)
 
+from pants.backend.jvm.subsystems.junit import JUnit
 from pants.backend.jvm.targets.java_tests import JavaTests
 from pants.base.exceptions import TargetDefinitionException
 from pants.build_graph.target import Target
 from pants_test.base_test import BaseTest
+from pants_test.subsystem.subsystem_util import init_subsystem
 
 
 class JavaTestsTest(BaseTest):
 
   def test_validation(self):
+    init_subsystem(JUnit)
     target = self.make_target('//:mybird', Target)
     # A plain invocation with no frills
     test1 = self.make_target('//:test1', JavaTests, sources=["Test.java"], dependencies=[target])
@@ -55,3 +58,17 @@ def test_validation(self):
     # timeout parameter
     timeout = self.make_target('//:testtimeout1', JavaTests, sources=["Test.java"], timeout=999)
     self.assertEquals(999, timeout.timeout)
+
+  def test_implicit_junit_dep(self):
+    init_subsystem(JUnit)
+    # Check that the implicit dep is added, and doesn't replace other deps.
+    target = self.make_target('//:target', Target)
+    test1 = self.make_target('//:test1', JavaTests, dependencies=[target])
+    self.assertEquals(['JarLibrary(//:junit_library)', 'Target(//:target)'],
+                      sorted(str(x) for x in test1.dependencies))
+
+    # Check that having an explicit dep doesn't cause problems.
+    junit_target = self.build_graph.get_target_from_spec('//:junit_library')
+    test2 = self.make_target('//:test2', JavaTests, dependencies=[junit_target, target])
+    self.assertEquals(['JarLibrary(//:junit_library)', 'Target(//:target)'],
+                      sorted(str(x) for x in test2.dependencies))

tests/python/pants_test/backend/jvm/tasks/BUILD

@@ -355,6 +355,7 @@ python_tests(
   sources = ['test_junit_run.py'],
   dependencies = [
     '3rdparty/python:mock',
+    'src/python/pants/backend/jvm/subsystems:junit',
     'src/python/pants/backend/jvm/targets:java',
     'src/python/pants/backend/jvm/tasks:junit_run',
     'src/python/pants/backend/python/tasks:python',

tests/python/pants_test/backend/jvm/tasks/test_junit_run.py

@@ -11,6 +11,7 @@
 
 from mock import patch
 
+from pants.backend.jvm.subsystems.junit import JUnit
 from pants.backend.jvm.targets.java_tests import JavaTests
 from pants.backend.jvm.tasks.junit_run import JUnitRun
 from pants.backend.python.targets.python_tests import PythonTests
@@ -25,7 +26,7 @@
 from pants.util.dirutil import safe_file_dump
 from pants.util.timeout import TimeoutReached
 from pants_test.jvm.jvm_tool_task_test_base import JvmToolTaskTestBase
-from pants_test.subsystem.subsystem_util import global_subsystem_instance
+from pants_test.subsystem.subsystem_util import global_subsystem_instance, init_subsystem
 
 
 class JUnitRunnerTest(JvmToolTaskTestBase):
@@ -58,6 +59,10 @@ def test_junit_runner_success(self):
       """)
     )
 
+  def setUp(self):
+    super(JUnitRunnerTest, self).setUp()
+    init_subsystem(JUnit)
+
   def test_junit_runner_failure(self):
     with self.assertRaises(TaskError) as cm:
       self.execute_junit_runner(
@@ -173,7 +178,7 @@ def execute_junit_runner(self, content, create_some_resources=True, **kwargs):
     subprocess.check_call(
       [javac, '-d', test_classes_abs_path, '-cp', classpath, test_java_file_abs_path])
 
-    # If a target_name is specified, create a target with it, otherwise create a java_tests target.
+    # If a target_name is specified create a target with it, otherwise create a java_tests target.
     if 'target_name' in kwargs:
       target = self.target(kwargs['target_name'])
     else:
@@ -205,37 +210,37 @@ def test_junit_runner_raises_no_error_on_non_junit_target(self):
     """Run pants against a `python_tests` target, but set an option for the `test.junit` task. This
     should execute without error.
     """
-    self.add_to_build_file('foo', dedent('''
+    self.add_to_build_file('foo', dedent("""
         python_tests(
           name='hello',
           sources=['some_file.py'],
         )
-        '''
+        """
     ))
     self.set_options(test='#abc')
     task = self.create_task(self.context(target_roots=[self.target('foo:hello')]))
     task.execute()
 
   def test_empty_sources(self):
-    self.add_to_build_file('foo', dedent('''
+    self.add_to_build_file('foo', dedent("""
         java_tests(
           name='empty',
           sources=[],
         )
-        '''
+        """
     ))
     task = self.create_task(self.context(target_roots=[self.target('foo:empty')]))
     with self.assertRaisesRegexp(TargetDefinitionException,
                                  r'must include a non-empty set of sources'):
       task.execute()
 
   def test_allow_empty_sources(self):
-    self.add_to_build_file('foo', dedent('''
+    self.add_to_build_file('foo', dedent("""
         java_tests(
           name='empty',
           sources=[],
         )
-        '''
+        """
     ))
     self.set_options(allow_empty_sources=True)
     context = self.context(target_roots=[self.target('foo:empty')])

tests/python/pants_test/backend/project_info/tasks/BUILD

@@ -60,6 +60,7 @@ python_tests(
   name = 'export',
   sources = ['test_export.py'],
   dependencies = [
+    'src/python/pants/backend/jvm/subsystems:junit',
     'src/python/pants/backend/jvm/subsystems:jvm_platform',
     'src/python/pants/backend/jvm/subsystems:scala_platform',
     'src/python/pants/backend/jvm/targets:java',