When source fields are strings, not collections, raise an error; Test…

… deferred sources addresses error

Currently if a string is passed to sources in a target definition, it will be treated as a list of single character filenames, which blows up in a confusing way later. This makes passing a string an error.

While I was in here, I added unit tests covering the too many addresses case for deferred sources and uncovered a bug where if the address kwarg isn't passed, instead of raising WrongNumberOfAddresses, it would instead hit an AttributeError on None.

Testing Done:
Wrote a few regression tests and made them pass.

Jenkins and travis passed

- http://jenkins.pantsbuild.org/job/pantsbuild/job/pants/branch/PR-3550/1/
- https://travis-ci.org/pantsbuild/pants/builds/135065849

Bugs closed: 3507, 3550

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

closes pantsbuild#3507, closes pantsbuild#3550

src/python/pants/build_graph/target.py

@@ -731,15 +731,23 @@ def create_sources_field(self, sources, sources_rel_path, address=None, key_arg=
     if isinstance(sources, Addresses):
       # Currently, this is only created by the result of from_target() which takes a single argument
       if len(sources.addresses) != 1:
+        key_arg_section = "'{}' to be ".format(key_arg) if key_arg else ""
+        spec_section = " to '{}'".format(address.spec) if address else ""
         raise self.WrongNumberOfAddresses(
-          "Expected a single address to from_target() as argument to {spec}"
-          .format(spec=address.spec))
+          "Expected {key_arg_section}a single address to from_target() as argument{spec_section}"
+          .format(key_arg_section=key_arg_section, spec_section=spec_section))
       referenced_address = Address.parse(sources.addresses[0], relative_to=sources.rel_path)
       return DeferredSourcesField(ref_address=referenced_address)
     elif sources is None:
       sources = FilesetWithSpec.empty(sources_rel_path)
-    elif not isinstance(sources, FilesetWithSpec):
+    elif isinstance(sources, FilesetWithSpec):
+      pass
+    elif isinstance(sources, (set, list, tuple)):
       # Received a literal sources list: convert to a FilesetWithSpec via Files.
       sources = Files.create_fileset_with_spec(sources_rel_path, *sources)
+    else:
+      key_arg_section = "'{}' to be ".format(key_arg) if key_arg else ""
+      raise TargetDefinitionException(self, "Expected {}a glob, an address or a list, but was {}"
+                                            .format(key_arg_section, type(sources)))
 
     return SourcesField(sources=sources)

tests/python/pants_test/build_graph/test_target.py

@@ -7,7 +7,9 @@
 
 import os.path
 
+from pants.base.exceptions import TargetDefinitionException
 from pants.base.payload import Payload
+from pants.build_graph.address import Address, Addresses
 from pants.build_graph.target import Target
 from pants.source.payload_fields import DeferredSourcesField
 from pants_test.base_test import BaseTest
@@ -98,3 +100,41 @@ def test_target_id_short(self):
     short_id = short_target.id
     self.assertEqual(short_id,
                      'dummy.dummy1.dummy2.dummy3.dummy4.dummy5.dummy6.dummy7.dummy8.dummy9.foo')
+
+  def test_create_sources_field_with_string_fails(self):
+    target = self.make_target(':a-target', Target)
+
+    # No key_arg.
+    with self.assertRaises(TargetDefinitionException) as cm:
+      target.create_sources_field(sources='a-string', sources_rel_path='')
+    self.assertIn("Expected a glob, an address or a list, but was <type \'unicode\'>",
+                  str(cm.exception))
+
+    # With key_arg.
+    with self.assertRaises(TargetDefinitionException) as cm:
+      target.create_sources_field(sources='a-string', sources_rel_path='', key_arg='my_cool_field')
+    self.assertIn("Expected 'my_cool_field' to be a glob, an address or a list, but was <type \'unicode\'>",
+                  str(cm.exception))
+    #could also test address case, but looks like nothing really uses it.
+
+  def test_sources_with_more_than_one_address_fails(self):
+    addresses = Addresses(['a', 'b', 'c'], '')
+    t = self.make_target(':t', Target)
+
+    # With address, no key_arg.
+    with self.assertRaises(Target.WrongNumberOfAddresses) as cm:
+      t.create_sources_field(sources=addresses, sources_rel_path='', address=Address.parse('a:b'))
+    self.assertIn("Expected a single address to from_target() as argument to 'a:b'",
+                  str(cm.exception))
+
+    # With no address.
+    with self.assertRaises(Target.WrongNumberOfAddresses) as cm:
+      t.create_sources_field(sources=addresses, sources_rel_path='')
+    self.assertIn("Expected a single address to from_target() as argument",
+                  str(cm.exception))
+
+    # With key_arg.
+    with self.assertRaises(Target.WrongNumberOfAddresses) as cm:
+      t.create_sources_field(sources=addresses, sources_rel_path='', key_arg='cool_field')
+    self.assertIn("Expected 'cool_field' to be a single address to from_target() as argument",
+                  str(cm.exception))