Error out if the user explicitly tries to test a non-test tgt.

Previously we would apply the union filter to all targets.
So `./pants test path/to:non-test-target` would noop.

Now we only apply the filter to targets that originated in a glob spec,
such as path/to/targets::. If the user tries to run tests on
a non-test target via a single address spec then we don't apply
the filter, and instead fail on attempting to cast the non-test
target to the TestTarget union (which yields a sensible error message).

This gives us the best of both worlds: We can give a sensible error,
instead of nooping, when the user specifies a single target, but
treat globs in the expected way.

This required introducing the concept of the "provenance" of a
BuildFileAddress. That is, the Spec it originated from. The term
"source" is overloaded in this context, so we use "provenance"
instead.

src/python/pants/base/BUILD

@@ -139,6 +139,8 @@ python_library(
     '3rdparty/python:dataclasses',
     'src/python/pants/util:collections',
     'src/python/pants/util:dirutil',
+    'src/python/pants/util:filtering',
+    'src/python/pants/util:memo',
     'src/python/pants/util:objects',
   ],
 )
@@ -213,3 +215,10 @@ python_library(
     'src/python/pants/util:strutil',
   ]
 )
+
+python_tests(
+  name='tests',
+  dependencies=[
+    ':specs'
+  ]
+)

src/python/pants/base/specs.py

@@ -192,6 +192,24 @@ def make_glob_patterns(self, address_mapper):
     ]
 
 
+_specificity = {
+  SingleAddress: 0,
+  SiblingAddresses: 1,
+  AscendantAddresses: 2,
+  DescendantAddresses: 3
+}
+
+
+def more_specific(spec1, spec2):
+  """Returns which of the two specs is more specific.
+
+  This is useful when a target matches multiple specs, and we want to associate it with
+  the "most specific" one, which will make the most intuitive sense to the user.
+  """
+  # Note that if either of spec1 or spec2 is None, the other will be returned.
+  return spec1 if _specificity.get(type(spec1), 99) < _specificity.get(type(spec2), 99) else spec2
+
+
 class SpecsMatcher(datatype([('tags', tuple), ('exclude_patterns', tuple)])):
   """Contains filters for the output of a Specs match.
 

src/python/pants/base/test_specs.py

@@ -0,0 +1,41 @@
+# Copyright 2019 Pants project contributors (see CONTRIBUTORS.md).
+# Licensed under the Apache License, Version 2.0 (see LICENSE).
+
+from pants.base.specs import (
+  AscendantAddresses,
+  DescendantAddresses,
+  SiblingAddresses,
+  SingleAddress,
+  more_specific,
+)
+
+
+def test_more_specific():
+  single_address = SingleAddress(directory='foo/bar', name='baz')
+  sibling_addresses = SiblingAddresses(directory='foo/bar')
+  ascendant_addresses = AscendantAddresses(directory='foo/bar')
+  descendant_addresses = DescendantAddresses(directory='foo/bar')
+
+  assert single_address == more_specific(single_address, None)
+  assert single_address == more_specific(single_address, sibling_addresses)
+  assert single_address == more_specific(single_address, ascendant_addresses)
+  assert single_address == more_specific(single_address, descendant_addresses)
+  assert single_address == more_specific(None, single_address)
+  assert single_address == more_specific(sibling_addresses, single_address)
+  assert single_address == more_specific(ascendant_addresses, single_address)
+  assert single_address == more_specific(descendant_addresses, single_address)
+
+  assert sibling_addresses == more_specific(sibling_addresses, None)
+  assert sibling_addresses == more_specific(sibling_addresses, ascendant_addresses)
+  assert sibling_addresses == more_specific(sibling_addresses, descendant_addresses)
+  assert sibling_addresses == more_specific(None, sibling_addresses)
+  assert sibling_addresses == more_specific(ascendant_addresses, sibling_addresses)
+  assert sibling_addresses == more_specific(descendant_addresses, sibling_addresses)
+
+  assert ascendant_addresses == more_specific(ascendant_addresses, None)
+  assert ascendant_addresses == more_specific(ascendant_addresses, descendant_addresses)
+  assert ascendant_addresses == more_specific(None, ascendant_addresses)
+  assert ascendant_addresses == more_specific(descendant_addresses, ascendant_addresses)
+
+  assert descendant_addresses == more_specific(descendant_addresses, None)
+  assert descendant_addresses == more_specific(None, descendant_addresses)

src/python/pants/build_graph/address.py

@@ -269,12 +269,14 @@ def __init__(self, build_file=None, target_name=None, rel_path=None):
     """
     rel_path = rel_path or build_file.relpath
     spec_path = os.path.dirname(rel_path)
-    super().__init__(spec_path=spec_path,
-                                           target_name=target_name or os.path.basename(spec_path))
+    super().__init__(spec_path=spec_path, target_name=target_name or os.path.basename(spec_path))
     self.rel_path = rel_path
 
   def to_address(self):
     """Convert this BuildFileAddress to an Address."""
+    # This is weird, since BuildFileAddress is a subtype of Address, but the engine's exact
+    # type matching requires a new instance.
+    # TODO: Possibly BuildFileAddress should wrap an Address instead of subclassing it.
     return Address(spec_path=self.spec_path, target_name=self.target_name)
 
   def __repr__(self):

src/python/pants/engine/addressable.py

@@ -3,8 +3,10 @@
 
 import inspect
 from collections.abc import MutableMapping, MutableSequence
+from dataclasses import dataclass
 from functools import update_wrapper
 
+from pants.base.specs import Spec
 from pants.build_graph.address import Address, BuildFileAddress
 from pants.engine.objects import Collection, Resolvable, Serializable
 from pants.util.objects import TypeConstraintError
@@ -13,13 +15,23 @@
 Addresses = Collection.of(Address)
 
 
+@dataclass(frozen=True)
+class ProvenancedBuildFileAddress:
+  """A BuildFileAddress along with the cmd-line spec it was generated from."""
+  build_file_address: BuildFileAddress
+  provenance: Spec
+
+
 class BuildFileAddresses(Collection.of(BuildFileAddress)):
   @property
   def addresses(self):
     """Converts the BuildFileAddress objects in this collection to Address objects."""
     return [bfa.to_address() for bfa in self]
 
 
+ProvenancedBuildFileAddresses = Collection.of(ProvenancedBuildFileAddress)
+
+
 class NotSerializableError(TypeError):
   """Indicates an addressable descriptor is illegally installed in a non-Serializable type."""
 

src/python/pants/engine/build_files.py

@@ -3,15 +3,22 @@
 
 import logging
 from collections.abc import MutableMapping, MutableSequence
+from dataclasses import dataclass
 from os.path import dirname, join
+from typing import Dict
 
 from twitter.common.collections import OrderedSet
 
 from pants.base.project_tree import Dir
-from pants.base.specs import SingleAddress, Spec, Specs
+from pants.base.specs import SingleAddress, Spec, Specs, more_specific
 from pants.build_graph.address import Address, BuildFileAddress
 from pants.build_graph.address_lookup_error import AddressLookupError
-from pants.engine.addressable import AddressableDescriptor, BuildFileAddresses
+from pants.engine.addressable import (
+  AddressableDescriptor,
+  BuildFileAddresses,
+  ProvenancedBuildFileAddress,
+  ProvenancedBuildFileAddresses,
+)
 from pants.engine.fs import Digest, FilesContent, PathGlobs, Snapshot
 from pants.engine.mapper import AddressFamily, AddressMap, AddressMapper, ResolveError
 from pants.engine.objects import Locatable, SerializableFactory, Validatable
@@ -179,6 +186,20 @@ def _hydrate(item_type, spec_path, **kwargs):
   return item
 
 
+@dataclass(frozen=True)
+class AddressMapperAndSpecs:
+  # TODO: We only need this class because Get() requires a single Param.
+  address_mapper: AddressMapper
+  specs: Specs
+
+
+@rule
+def bind_addressmapper_and_specs(
+    address_mapper: AddressMapper, specs: Specs
+) -> AddressMapperAndSpecs:
+  return AddressMapperAndSpecs(address_mapper, specs)
+
+
 @rule
 def addresses_from_address_families(
   address_mapper: AddressMapper, specs: Specs
@@ -190,13 +211,37 @@ def addresses_from_address_families(
      - the Spec matches no addresses for SingleAddresses.
   :raises: :class:`AddressLookupError` if no targets are matched for non-SingleAddress specs.
   """
+  # Capture a Snapshot covering all paths for these Specs, then group by directory.
+  provenanced_build_file_addresses = yield Get(
+    ProvenancedBuildFileAddresses,
+    AddressMapperAndSpecs, AddressMapperAndSpecs(address_mapper, specs))
+  yield BuildFileAddresses(
+    tuple(pbfa.build_file_address for pbfa in provenanced_build_file_addresses))
+
+
+@rule
+def provenanced_addresses_from_address_families(
+    address_mapper_and_specs: AddressMapperAndSpecs
+) -> ProvenancedBuildFileAddresses:
+  """Given an AddressMapper and list of Specs, return matching ProvenancedBuildFileAddresses.
+
+  :raises: :class:`ResolveError` if:
+     - there were no matching AddressFamilies, or
+     - the Spec matches no addresses for SingleAddresses.
+  :raises: :class:`AddressLookupError` if no targets are matched for non-SingleAddress specs.
+  """
+  address_mapper = address_mapper_and_specs.address_mapper
+  specs = address_mapper_and_specs.specs
+
   # Capture a Snapshot covering all paths for these Specs, then group by directory.
   snapshot = yield Get(Snapshot, PathGlobs, _spec_to_globs(address_mapper, specs))
   dirnames = {dirname(f) for f in snapshot.files}
   address_families = yield [Get(AddressFamily, Dir(d)) for d in dirnames]
   address_family_by_directory = {af.namespace: af for af in address_families}
 
   matched_addresses = OrderedSet()
+  addr_to_provenance: Dict[BuildFileAddress, Spec] = {}
+
   for spec in specs:
     # NB: if a spec is provided which expands to some number of targets, but those targets match
     # --exclude-target-regexp, we do NOT fail! This is why we wait to apply the tag and exclude
@@ -208,6 +253,9 @@ def addresses_from_address_families(
 
     try:
       all_addr_tgt_pairs = spec.address_target_pairs_from_address_families(addr_families_for_spec)
+      for addr, _ in all_addr_tgt_pairs:
+        # A target might be covered by multiple specs, so we take the most specific one.
+        addr_to_provenance[addr] = more_specific(addr_to_provenance.get(addr), spec)
     except Spec.AddressResolutionError as e:
       raise AddressLookupError(e) from e
     except SingleAddress._SingleAddressResolutionError as e:
@@ -219,7 +267,25 @@ def addresses_from_address_families(
     )
 
   # NB: This may be empty, as the result of filtering by tag and exclude patterns!
-  yield BuildFileAddresses(tuple(matched_addresses))
+  yield ProvenancedBuildFileAddresses(tuple(
+    ProvenancedBuildFileAddress(build_file_address=addr, provenance=addr_to_provenance.get(addr))
+    for addr in matched_addresses
+  ))
+
+
+@dataclass(frozen=True)
+class AddressProvenanceMap:
+  bfaddr_to_spec: Dict[BuildFileAddress, Spec]
+
+  def is_single_address(self, address: BuildFileAddress):
+    return isinstance(self.bfaddr_to_spec.get(address), SingleAddress)
+
+
+@rule
+def address_provenance_map(pbfas: ProvenancedBuildFileAddresses) -> AddressProvenanceMap:
+  return AddressProvenanceMap(bfaddr_to_spec={
+    pbfa.build_file_address: pbfa.provenance for pbfa in pbfas.dependencies
+  })
 
 
 def _spec_to_globs(address_mapper, specs):
@@ -249,6 +315,9 @@ def address_mapper_singleton() -> AddressMapper:
     # Spec handling: locate directories that contain build files, and request
     # AddressFamilies for each of them.
     addresses_from_address_families,
+    provenanced_addresses_from_address_families,
+    address_provenance_map,
+    bind_addressmapper_and_specs,
     # Root rules representing parameters that might be provided via root subjects.
     RootRule(Address),
     RootRule(BuildFileAddress),

src/python/pants/rules/core/test.py

@@ -8,6 +8,7 @@
 from pants.base.exiter import PANTS_FAILED_EXIT_CODE, PANTS_SUCCEEDED_EXIT_CODE
 from pants.build_graph.address import Address, BuildFileAddress
 from pants.engine.addressable import BuildFileAddresses
+from pants.engine.build_files import AddressProvenanceMap
 from pants.engine.console import Console
 from pants.engine.goal import Goal
 from pants.engine.legacy.graph import HydratedTarget
@@ -77,11 +78,13 @@ def fast_test(console: Console, addresses: BuildFileAddresses) -> Test:
 
 @rule
 def coordinator_of_tests(target: HydratedTarget,
-                         union_membership: UnionMembership) -> AddressAndTestResult:
+                         union_membership: UnionMembership,
+                         provenance_map: AddressProvenanceMap) -> AddressAndTestResult:
   # TODO(#6004): when streaming to live TTY, rely on V2 UI for this information. When not a
   # live TTY, periodically dump heavy hitters to stderr. See
   # https://github.com/pantsbuild/pants/issues/6004#issuecomment-492699898.
-  if union_membership.is_member(TestTarget, target.adaptor):
+  if (provenance_map.is_single_address(target.address) or
+      union_membership.is_member(TestTarget, target.adaptor)):
     logger.info("Starting tests: {}".format(target.address.reference()))
     # NB: This has the effect of "casting" a TargetAdaptor to a member of the TestTarget union.
     # The adaptor will always be a member because of the union membership check above, but if

tests/python/pants_test/rules/test_test.py

@@ -4,9 +4,11 @@
 import logging
 from textwrap import dedent
 
+from pants.base.specs import DescendantAddresses, SingleAddress
 from pants.build_graph.address import Address, BuildFileAddress
+from pants.engine.build_files import AddressProvenanceMap
 from pants.engine.legacy.graph import HydratedTarget
-from pants.engine.legacy.structs import PythonTestsAdaptor
+from pants.engine.legacy.structs import PythonBinaryAdaptor, PythonTestsAdaptor
 from pants.engine.rules import UnionMembership
 from pants.rules.core.core_test_model import TestTarget
 from pants.rules.core.test import (
@@ -115,6 +117,7 @@ def test_coordinator_python_test(self):
         coordinator_of_tests,
         HydratedTarget(addr, target_adaptor, ()),
         UnionMembership(union_rules={TestTarget: [PythonTestsAdaptor]}),
+        AddressProvenanceMap(bfaddr_to_spec={}),
         {
           (TestResult, PythonTestsAdaptor):
             lambda _: TestResult(status=Status.FAILURE, stdout='foo', stderr=''),
@@ -124,3 +127,66 @@ def test_coordinator_python_test(self):
       result,
       AddressAndTestResult(addr, TestResult(status=Status.FAILURE, stdout='foo', stderr=''))
     )
+
+  def test_globbed_test_target(self):
+    bfaddr = BuildFileAddress(None, 'tests', 'some/dir')
+    target_adaptor = PythonTestsAdaptor(type_alias='python_tests')
+    with self.captured_logging(logging.INFO):
+      result = run_rule(
+        coordinator_of_tests,
+        HydratedTarget(bfaddr.to_address(), target_adaptor, ()),
+        UnionMembership(union_rules={TestTarget: [PythonTestsAdaptor]}),
+        AddressProvenanceMap(bfaddr_to_spec={
+          bfaddr: DescendantAddresses(directory='some/dir')
+        }),
+        {
+          (TestResult, PythonTestsAdaptor):
+            lambda _: TestResult(status=Status.SUCCESS, stdout='foo', stderr=''),
+        })
+
+      self.assertEqual(
+        result,
+        AddressAndTestResult(bfaddr.to_address(),
+                             TestResult(status=Status.SUCCESS, stdout='foo', stderr=''))
+      )
+
+  def test_globbed_non_test_target(self):
+    bfaddr = BuildFileAddress(None, 'bin', 'some/dir')
+    target_adaptor = PythonBinaryAdaptor(type_alias='python_binary')
+    with self.captured_logging(logging.INFO):
+      result = run_rule(
+        coordinator_of_tests,
+        HydratedTarget(bfaddr.to_address(), target_adaptor, ()),
+        UnionMembership(union_rules={TestTarget: [PythonTestsAdaptor]}),
+        AddressProvenanceMap(bfaddr_to_spec={
+          bfaddr: DescendantAddresses(directory='some/dir')
+        }),
+        {
+          (TestResult, PythonTestsAdaptor):
+            lambda _: TestResult(status=Status.SUCCESS, stdout='foo', stderr=''),
+        })
+
+      self.assertEqual(
+        result,
+        AddressAndTestResult(bfaddr.to_address(), None)
+      )
+
+  def test_single_non_test_target(self):
+    bfaddr = BuildFileAddress(None, 'bin', 'some/dir')
+    target_adaptor = PythonBinaryAdaptor(type_alias='python_binary')
+    with self.captured_logging(logging.INFO):
+      # Note that this is not the same error message the end user will see, as we're resolving
+      # union Get requests in run_rule, not the real engine.  But this test still asserts that
+      # we error when we expect to error.
+      with self.assertRaisesRegex(AssertionError, r'Rule requested: .* which cannot be satisfied.'):
+        run_rule(
+          coordinator_of_tests,
+          HydratedTarget(bfaddr.to_address(), target_adaptor, ()),
+          UnionMembership(union_rules={TestTarget: [PythonTestsAdaptor]}),
+          AddressProvenanceMap(bfaddr_to_spec={
+            bfaddr: SingleAddress(directory='some/dir', name='bin')
+          }),
+          {
+            (TestResult, TestTarget):
+              lambda _: TestResult(status=Status.SUCCESS, stdout='foo', stderr=''),
+          })