New expect_tests UI

expect_tests/pipeline.py

Index: expect_tests/pipeline.py
diff --git a/expect_tests/pipeline.py b/expect_tests/pipeline.py
index 04da90309c81da86e7ef537d18c3d6e82ec0c68d..2f383672c2ea9f63823779b23932a31c811bb64b 100644
--- a/expect_tests/pipeline.py
+++ b/expect_tests/pipeline.py
@@ -52,6 +52,31 @@ class ResetableStringIO(object):
     return getattr(self._stream, key)
 
 
+def get_python_root(path):
+  """Get the lowest directory with no __init__.py file.
+
+  When ``path`` is pointing inside a Python package, this function returns the
+  directory directly containing this package. If ``path`` points outside of
+  a Python package, the it returns ``path``.
+
+  Args:
+    path (str): arbitrary path
+  Returns:
+    root (str): ancestor directory, with no __init__.py file in it.
+  """
+  if not os.path.exists(path):
+    raise ValueError('path must exist: %s')
+
+  while path != os.path.dirname(path):
+    if not os.path.exists(os.path.join(path, '__init__.py')):
+      return path
+    path = os.path.dirname(path)
+
+  # This is not supposed to happen, but in case somebody adds a __init__.py
+  # at the filesystem root ...
+  raise IOError("Unable to find a python root for %s" % path)
+
+
 def get_package_path(package_name, path):
   """Return path toward 'package_name'.
 
@@ -511,6 +536,140 @@ def result_loop_single_path(cover_ctx, kill_switch, result_queue, opts,
     p.join()
 
 
+def parse_test_glob(test_glob):
+  """A test glob is composed of a path and a glob expression like:

[iannucci, 2014/11/12 20:26:24]

do we still support the negative glob e.g. `-path/to/something:*Slow*`?

[pgervais, 2014/11/13 17:55:46]

Not currently (see above comment)

+    '<path>:<glob>'. The path should point to a directory or a file inside

[iannucci, 2014/11/12 20:26:24]

TODO: let's scan packages too so that <path> can also be a glob. Then
expect_tests can use a default of '*:*' when no tests are specified.

[pgervais, 2014/11/13 17:55:46]

TODO added.

+    a Python package (it can be the root directory of that package).
+    The glob is a Python name used to filter tests.
+
+    Example:
+    'my/nice/package/test1/:TestA*', the package root being 'my/nice/package':
+    this matches all tests whose name starts with 'TestA' inside all files
+    matching test1/*_test.py.

[iannucci, 2014/11/12 20:26:24]

not strictly true, right? non-unittest tests would also match if they had a
similar name, correct?

TestA* would also be matching the unittest classname so a single test might be
`TestABunchOfStuff.testCatFoodDeployment`?

[pgervais, 2014/11/13 17:55:46]

True, the definition of a 'test name' is not very clear here. I'll try to
clarify that in the docstring.

+
+    Args:
+       test_glob (str): a test glob
+    Returns:
+       (path, test_filter): absolute path and test filter glob.
+  """
+  parts = test_glob.split(':')
+  if len(parts) > 2:
+    raise ValueError('A test_glob should contain at most one colon (got %s)'
+                     % test_glob)
+  if len(parts) == 2:
+    path, test_filter = parts
+    if '/' in test_filter:

[iannucci, 2014/11/12 20:26:24]

Hm, I'm not sure this is strictly true... do we enforce this in Test.__init__?
There are use cases where the test name is e.g. `foo/simple/test_bar.in`, which
would correspond to an input file on disk.

[pgervais, 2014/11/13 17:55:46]

The test filter is only applied to test names, which are Python names. Thus
don't contain any slashes.

+      raise ValueError('A test filter cannot contain a slash (got %s)',
+                       test_filter)
+
+    if not test_filter:  # empty string case
+      test_filter = '*'
+  else:
+    path, test_filter = parts[0], '*'
+
+  path = os.path.abspath(path)
+  return path, test_filter
+
+
+class PackageTestingContext(object):

[iannucci, 2014/11/12 20:26:24]

may be worth having all this context stuff in it's own context.py instead of
pipeline.py

+  def __init__(self, cwd, package_name, filters):
+    """Information to run a set of tests in a single package.
+
+    See also parse_test_glob.
+    """
+    self.cwd = cwd
+    self.package_name = package_name
+    # list of (path, filter) pairs.
+    # The path is where to look for tests for. Only tests whose name matches the
+    # glob are kept.
+    self.filters = filters
+
+  @classmethod
+  def from_path(cls, path, filters='*'):

[iannucci, 2014/11/12 20:26:24]

maybe make filters `('*',)` so that you don't need the basestring case (and
filters will always be an iterable-of-strings). Or make empty-sequence mean '*'
and then have it default to `()`

[pgervais, 2014/11/13 17:55:47]

This is one of the features of Python that I like: being able to drop the
iterable in a parameter when you only have one item. I've fixed that.

That said, you're right that the empty sequence means ('*',), and this case is
not taken into account right now.

+    path = os.path.abspath(path)
+    cwd = get_python_root(path)
+    package_name = os.path.relpath(path, cwd).split(os.path.sep)[0]
+    # list of (path, filter) pairs.
+    # The path is where to look for tests for. Only tests whose name matches the
+    # glob are kept.
+    if isinstance(filters, basestring):
+      filters = [(path, filters)]
+    else:
+      filters = [(path, filt) for filt in filters]
+
+    return cls(cwd, package_name, filters)
+
+  @classmethod
+  def from_context_list(cls, contexts):
+    """Merge several PackageTestingContext pointing to the same package."""
+    cwd = set(context.cwd for context in contexts)
+    assert len(cwd) == 1, \
+           'from_context_list processes contexts with the same working '\
+           'directory only.'
+
+    package_name = set(context.package_name for context in contexts)
+    assert len(package_name) == 1, \
+           'from_context_list processes contexts with the same package '\
+           'name only.'

[iannucci, 2014/11/12 20:26:24]

may be friendlier to have `def merge_contexts(cls, contexts) -> contexts` which
would simplify a set of contexts to their minimal set of contexts.

Is there a way to derive package_name or cwd from each other so that it becomes
impossible to have two different packages with the same cwd, or vice versa? Then
the derivable one could be an @property

[pgervais, 2014/11/13 17:55:46]

If you consider only the RuntimeContext object, the answer is no. However, they
are constrained by the filesystem because the os.path.join of them has to exist.

But there could be two packages in the same directory or two packages with the
same name in different directories (though it's not common).

+
+    filters = []
+    for context in contexts:
+      filters.extend(context.filters)
+
+    return cls(cwd.pop(), package_name.pop(), filters)
+
+
+class ProcessingContext(object):
+  def __init__(self, testing_contexts):
+    """Information to run a set of tasks in a given working directory.
+
+    Args:
+    testing_contexts (list): list of PackageTestingContext instances
+    """
+    self.cwd = testing_contexts[0].cwd
+
+    # Merge testing_contexts by package
+    groups = {}
+    for context in testing_contexts:
+      if context.cwd != self.cwd:
+        raise ValueError('All package must have the same value for "cwd"')
+      groups.setdefault(context.package_name, []).append(context)
+
+    self.testing_contexts = [PackageTestingContext.from_context_list(contexts)
+                             for contexts in groups.itervalues()]

[iannucci, 2014/11/12 20:26:24]

Yeah, I think the merge contexts function I mentioned above would be a cleaner
interface. It could even be a standalone function?

+
+
+def get_runtime_contexts(test_globs):
+  """Compute the list of packages/filters to get tests from."""
+  # Step 1: compute list of packages + subtree
+  testing_contexts = []
+  for test_glob in test_globs:
+    path, test_filter = parse_test_glob(test_glob)
+    if os.path.exists(os.path.join(path, '__init__.py')):
+      testing_contexts.append(
+        PackageTestingContext.from_path(path, test_filter))
+    else:
+      # Look for all packages in path.
+      subpaths = []
+      black_list = get_config(path)
+
+      for filename in filter(lambda x: x not in black_list, os.listdir(path)):
+        abs_filename = os.path.join(path, filename)
+        if (os.path.isdir(abs_filename)
+            and os.path.isfile(os.path.join(abs_filename, '__init__.py'))):
+          subpaths.append(abs_filename)
+
+      testing_contexts.extend(
+        [PackageTestingContext.from_path(subpath, test_filter)
+         for subpath in subpaths])
+
+  # Step 2: group by working directory - one process per wd.
+  groups = {}
+  for context in testing_contexts:
+    groups.setdefault(context.cwd, []).append(context)
+  return [ProcessingContext(contexts) for contexts in groups.itervalues()]
+
+
 def result_loop(cover_ctx, opts):
   """Run the specified operation in all paths in parallel.
 
@@ -520,6 +679,8 @@ def result_loop(cover_ctx, opts):
   The operation to perform (list/test/debug/train) is defined by opts.handler.
   """
 
+  runtime_contexts = get_runtime_context(opts.test_glob)
+
   def ensure_echo_on():
     """Restore echo on in the terminal.