Update path(s) tasks, add tests, extract pluralize to strutil

The path / paths tasks were untested, used older idioms and had behavior that didn't reflect their documentation.

This updates them so that the print out the paths rather than just have a count.
It switches to using the console_output method instead of execute
It updates the algorithm used to cover cases more effectively, even in the face of cycles.

It also extracts pluralize from reporting and moves it to strutil and adds tests for it.

Testing Done:
Wrote test suite, which uncovered weird behavior in the existing impl. Made it pass.

Bugs closed: 2274

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

closes pantsbuild#2274

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

@@ -284,6 +284,7 @@ python_library(
   dependencies = [
     ':console_task',
     'src/python/pants/base:exceptions',
+    'src/python/pants/util:strutil',
   ],
 )
 

src/python/pants/backend/core/tasks/paths.py

@@ -6,91 +6,90 @@
                         unicode_literals, with_statement)
 
 import copy
-from collections import defaultdict
+from collections import deque
 
 from pants.backend.core.tasks.console_task import ConsoleTask
 from pants.base.exceptions import TaskError
+from pants.util.strutil import pluralize
 
 
-class PathFinder(ConsoleTask):
-  def __init__(self, *args, **kwargs):
-    super(PathFinder, self).__init__(*args, **kwargs)
-    self.log = self.context.log
-    self.target_roots = self.context.target_roots
-
-  @classmethod
-  def _find_paths(cls, from_target, to_target, log):
-    log.debug('Looking for all paths from {} to {}'.format(from_target.address.reference(),
-                                                           to_target.address.reference()))
+def format_path(path):
+  return '[{}]'.format(', '.join([target.address.reference() for target in path]))
 
-    paths = cls._find_paths_rec(from_target, to_target)
-    print('Found {} paths'.format(len(paths)))
-    print('')
-    for path in paths:
-      log.debug('\t[{}]'.format(', '.join([target.address.reference() for target in path])))
 
-  all_paths = defaultdict(lambda: defaultdict(list))
+def find_paths_breadth_first(from_target, to_target, log):
+  """Yields the paths between from_target to to_target if they exist.
 
-  @classmethod
-  def _find_paths_rec(cls, from_target, to_target):
-    if from_target == to_target:
-      return [[from_target]]
+  The paths are returned ordered by length, shortest first.
+  If there are cycles, it checks visited edges to prevent recrossing them."""
+  log.debug('Looking for all paths from {} to {}'.format(from_target.address.reference(),
+                                                         to_target.address.reference()))
 
-    if from_target not in cls.all_paths or to_target not in cls.all_paths[from_target]:
-      paths = []
-      for dep in from_target.dependencies:
-        for path in cls._find_paths_rec(dep, to_target):
-          new_path = copy.copy(path)
-          new_path.insert(0, from_target)
-          paths.append(new_path)
+  if from_target == to_target:
+    yield [from_target]
+    return
 
-      cls.all_paths[from_target][to_target] = paths
+  visited_edges = set()
+  to_walk_paths = deque([[from_target]])
+  while len(to_walk_paths) > 0:
+    cur_path = to_walk_paths.popleft()
+    target = cur_path[-1]
 
-    return cls.all_paths[from_target][to_target]
+    if len(cur_path) > 1:
+      prev_target = cur_path[-2]
+    else:
+      prev_target = None
+    current_edge = (prev_target, target)
 
-  examined_targets = set()
+    if current_edge not in visited_edges:
+      for dep in target.dependencies:
+        dep_path = cur_path + [dep]
+        if dep == to_target:
+          yield dep_path
+        else:
+          to_walk_paths.append(dep_path)
+      visited_edges.add(current_edge)
 
-  @classmethod
-  def _find_path(cls, from_target, to_target, log):
-    log.debug('Looking for path from {} to {}'.format(from_target.address.reference(),
-                                                      to_target.address.reference()))
 
-    queue = [([from_target], 0)]
-    while True:
-      if not queue:
-        print('no path found from {} to {}!'.format(from_target.address.reference(),
-                                                    to_target.address.reference()))
-        break
-
-      path, indent = queue.pop(0)
-      next_target = path[-1]
-      if next_target in cls.examined_targets:
-        continue
-      cls.examined_targets.add(next_target)
+class PathFinder(ConsoleTask):
+  def __init__(self, *args, **kwargs):
+    super(PathFinder, self).__init__(*args, **kwargs)
+    self.log = self.context.log
+    self.target_roots = self.context.target_roots
 
-      log.debug('{} examining {}'.format('  ' * indent, next_target))
+  def validate_target_roots(self):
+    if len(self.target_roots) != 2:
+      raise TaskError('Specify two targets please (found {})'.format(len(self.target_roots)))
 
-      if next_target == to_target:
-        print('')
-        for target in path:
-          print('{}'.format(target.address.reference()))
-        break
 
-      for dep in next_target.dependencies:
-        queue.append((path + [dep], indent + 1))
+class Path(PathFinder):
+  """Find a dependency path from one target to another."""
 
+  def console_output(self, ignored_targets):
+    self.validate_target_roots()
 
-class Path(PathFinder):
-  def execute(self):
-    if len(self.target_roots) != 2:
-      raise TaskError('Specify two targets please (found {})'.format(len(self.target_roots)))
+    from_target = self.target_roots[0]
+    to_target = self.target_roots[1]
 
-    self._find_path(self.target_roots[0], self.target_roots[1], self.log)
+    for path in find_paths_breadth_first(from_target, to_target, self.log):
+      yield format_path(path)
+      break
+    else:
+      yield 'No path found from {} to {}!'.format(from_target.address.reference(),
+                                                  to_target.address.reference())
 
 
 class Paths(PathFinder):
-  def execute(self):
-    if len(self.target_roots) != 2:
-      raise TaskError('Specify two targets please (found {})'.format(len(self.target_roots)))
-
-    self._find_paths(self.target_roots[0], self.target_roots[1], self.log)
+  """Find all dependency paths from one target to another."""
+
+  def console_output(self, ignored_targets):
+    self.validate_target_roots()
+    from_target = self.target_roots[0]
+    to_target = self.target_roots[1]
+
+    paths = list(find_paths_breadth_first(from_target, to_target, self.log))
+    yield 'Found {}'.format(pluralize(len(paths), 'path'))
+    if paths:
+      yield ''
+      for path in paths:
+        yield '\t{}'.format(format_path(path))

src/python/pants/reporting/reporting_utils.py

@@ -5,6 +5,8 @@
 from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
                         unicode_literals, with_statement)
 
+from pants.util.strutil import pluralize
+
 
 def items_to_report_element(items, item_type):
   """Converts an iterable of items to a (message, detail) pair.
@@ -20,17 +22,10 @@ def items_to_report_element(items, item_type):
   This is useful when we want to say "N targets" or "K sources"
   and allow the user to see which ones by clicking on that text.
   """
-  def pluralize(x):
-    if x.endswith('s'):
-      return x + 'es'
-    else:
-      return x + 's'
-
-  items = [str(x) for x in items]
   n = len(items)
-  text = '{} {}'.format(n, item_type if n == 1 else pluralize(item_type))
+  text = pluralize(n, item_type)
   if n == 0:
     return text
   else:
-    detail = '\n'.join(items)
+    detail = '\n'.join(str(x) for x in items)
     return text, detail

src/python/pants/util/strutil.py

@@ -42,5 +42,24 @@ def safe_shlex_split(text_or_binary):
 
 
 def camelcase(string):
-  """Convert snake casing (containing - or _ charadcters) to camel casing."""
+  """Convert snake casing (containing - or _ characters) to camel casing."""
   return ''.join(word.capitalize() for word in re.split('[-_]', string))
+
+
+def pluralize(count, item_type):
+  """Pluralizes the item_type if the count does not equal one.
+
+  For example `pluralize(1, 'apple')` returns '1 apple',
+  while `pluralize(0, 'apple') returns '0 apples'.
+
+  :return The count and inflected item_type together as a string
+  :rtype string
+  """
+  def pluralize_string(x):
+    if x.endswith('s'):
+      return x + 'es'
+    else:
+      return x + 's'
+
+  text = '{} {}'.format(count, item_type if count == 1 else pluralize_string(item_type))
+  return text

tests/python/pants_test/backend/core/tasks/BUILD

@@ -108,6 +108,16 @@ python_tests(
   ]
 )
 
+python_tests(
+  name='paths',
+  sources=['test_paths.py'],
+  dependencies=[
+    'src/python/pants/backend/core/tasks:paths',
+    'src/python/pants/base:exceptions',
+    'tests/python/pants_test/tasks:task_test_base',
+  ]
+)
+
 python_tests(
   name='scm_publish',
   sources=['test_scm_publish.py'],

tests/python/pants_test/backend/core/tasks/test_paths.py

@@ -0,0 +1,130 @@
+# coding=utf-8
+# Copyright 2015 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.core.tasks.paths import Path, Paths
+from pants.base.exceptions import TaskError
+from pants_test.tasks.task_test_base import ConsoleTaskTestBase
+
+
+class PathsTest(ConsoleTaskTestBase):
+
+  @classmethod
+  def task_type(cls):
+    return Paths
+
+  def test_only_one_target(self):
+    target_a = self.make_target('a')
+    with self.assertRaises(TaskError) as cm:
+      self.execute_console_task(targets=[target_a])
+    self.assertIn('Specify two targets please', str(cm.exception))
+    self.assertIn('found 1', str(cm.exception))
+
+  def test_three_targets(self):
+    target_a = self.make_target('a')
+    target_b = self.make_target('b')
+    target_c = self.make_target('c')
+    with self.assertRaises(TaskError) as cm:
+      self.execute_console_task(targets=[target_a, target_b, target_c])
+    self.assertIn('Specify two targets please', str(cm.exception))
+    self.assertIn('found 3', str(cm.exception))
+
+  def test_path_dependency_first_finds_no_paths(self):
+    # Not sure if I like this behavior, but adding to document it
+    target_b = self.make_target('b')
+    target_a = self.make_target('a', dependencies=[target_b])
+
+    self.assert_console_output('Found 0 paths', targets=[target_b, target_a])
+
+  def test_single_edge_path(self):
+    target_b = self.make_target('b')
+    target_a = self.make_target('a', dependencies=[target_b])
+
+
+    self.assert_console_output('Found 1 path',
+                               '',
+                               '\t[a, b]',
+                               targets=[target_a, target_b])
+
+  def test_same_target_path(self):
+    target_b = self.make_target('b')
+
+    self.assert_console_output('Found 1 path',
+                               '',
+                               '\t[b]',
+                               targets=[target_b, target_b])
+
+  def test_two_paths(self):
+    target_b = self.make_target('b')
+    target_inner_1 = self.make_target('inner1', dependencies=[target_b])
+    target_inner_2 = self.make_target('inner2', dependencies=[target_b])
+    target_a = self.make_target('a', dependencies=[target_inner_1, target_inner_2])
+
+
+    self.assert_console_output('Found 2 paths',
+                               '',
+                               '\t[a, inner1, b]',
+                               '\t[a, inner2, b]',
+                               targets=[target_a, target_b])
+
+  def test_cycle_no_path(self):
+    target_b = self.make_target('b')
+    target_inner_1 = self.make_target('inner1')
+    target_inner_2 = self.make_target('inner2', dependencies=[target_inner_1])
+    target_a = self.make_target('a', dependencies=[target_inner_1])
+    target_inner_1.inject_dependency(target_inner_2.address)
+
+    self.assert_console_output('Found 0 paths',
+                               targets=[target_a, target_b])
+
+  def test_cycle_path(self):
+    target_b = self.make_target('b')
+    target_inner_1 = self.make_target('inner1', dependencies=[target_b])
+    target_inner_2 = self.make_target('inner2', dependencies=[target_inner_1, target_b])
+    target_inner_1.inject_dependency(target_inner_2.address)
+    target_a = self.make_target('a', dependencies=[target_inner_1])
+
+    self.assert_console_output('Found 3 paths',
+                               '',
+                               '\t[a, inner1, b]',
+                               '\t[a, inner1, inner2, b]',
+                               '\t[a, inner1, inner2, inner1, b]',
+                               targets=[target_a, target_b])
+
+  def test_overlapping_paths(self):
+    target_b = self.make_target('b')
+    target_inner_1 = self.make_target('inner1', dependencies=[target_b])
+    target_inner_2 = self.make_target('inner2', dependencies=[target_inner_1])
+    target_a = self.make_target('a', dependencies=[target_inner_1, target_inner_2])
+
+    self.assert_console_output('Found 2 paths',
+                               '',
+                               '\t[a, inner1, b]',
+                               '\t[a, inner2, inner1, b]',
+                               targets=[target_a, target_b])
+
+
+class PathTest(ConsoleTaskTestBase):
+
+  @classmethod
+  def task_type(cls):
+    return Path
+
+  def test_only_returns_first_path(self):
+    target_b = self.make_target('b')
+    target_inner_1 = self.make_target('inner1', dependencies=[target_b])
+    target_inner_2 = self.make_target('inner2', dependencies=[target_inner_1])
+    target_a = self.make_target('a', dependencies=[target_inner_1, target_inner_2])
+
+    self.assert_console_output('[a, inner1, b]',
+                               targets=[target_a, target_b])
+
+  def test_when_no_path(self):
+    target_b = self.make_target('b')
+    target_a = self.make_target('a')
+
+    self.assert_console_output('No path found from a to b!',
+                               targets=[target_a, target_b])

tests/python/pants_test/util/test_strutil.py

@@ -7,7 +7,7 @@
 
 import unittest
 
-from pants.util.strutil import camelcase
+from pants.util.strutil import camelcase, pluralize
 
 
 # TODO(Eric Ayers): Backfill tests for other methods in strutil.py
@@ -29,3 +29,11 @@ def test_camelcase(self):
     self.assertEquals('FooBar', camelcase('-foo-bar'))
     self.assertEquals('FooBar', camelcase('foo--bar'))
     self.assertEquals('FooBar', camelcase('foo-_bar'))
+
+  def test_pluralize(self):
+    self.assertEquals('1 bat', pluralize(1, 'bat'))
+    self.assertEquals('1 boss', pluralize(1, 'boss'))
+    self.assertEquals('2 bats', pluralize(2, 'bat'))
+    self.assertEquals('2 bosses', pluralize(2, 'boss'))
+    self.assertEquals('0 bats', pluralize(0, 'bat'))
+    self.assertEquals('0 bosses', pluralize(0, 'boss'))