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..03d89ac8ed89a8a70e41fad5e71957ed451621d0 100644
--- a/expect_tests/pipeline.py
+++ b/expect_tests/pipeline.py
@@ -7,6 +7,7 @@ import ConfigParser
 import glob
 import imp
 import inspect
+import itertools
 import logging
 import multiprocessing
 import os
@@ -52,6 +53,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 ...

[agable, 2014/11/12 03:15:09]

Love the French spacing between words and punctuation.

[pgervais, 2014/11/13 00:28:26]

ALERT: troll detected in comment line 77.

+  raise IOError("Unable to find a python root for %s" % path)
+
+
 def get_package_path(package_name, path):
   """Return path toward 'package_name'.
 
@@ -114,7 +140,7 @@ def is_test_file(filename):
   return filename.endswith('_test.py')
 
 
-def walk_package(package_name, path):
+def walk_package(package_name, path, subpath=None):
   """Return all test files inside a single package.
 
   In all cases, this function returns the full package name of files ending
@@ -139,6 +165,8 @@ def walk_package(package_name, path):
   Args:
   package_name (str): name of the package, as expected by import.
   path (str): path containing the above module (optional)
+  subpath (str, optional): path inside the package, pointing to a subpackage.
+     This is used to restrict the listing to part of the package.
 
   Returns:
   test_modules (list of str): name of modules containing tests. Each element is
@@ -164,7 +192,14 @@ def walk_package(package_name, path):
 
   explored = set()
 
-  for dirpath, dirnames, filenames in os.walk(package_path, followlinks=True):
+  if subpath:
+    start_path = os.path.join(package_path, subpath)
+    if not os.path.exists(os.path.join(start_path)):

[agable, 2014/11/12 03:15:10]

os.path.join not necessary

[pgervais, 2014/11/13 00:28:26]

Done.

+      raise ValueError('Provided subpath does not exist: %s' % start_path)
+  else:
+    start_path = package_path
+
+  for dirpath, dirnames, filenames in os.walk(start_path, followlinks=True):
     # Keep only submodules not blacklisted, break symlink cycles
     blacklist = get_config(dirpath)
     dirnames[:] = [d for d in dirnames
@@ -174,7 +209,7 @@ def walk_package(package_name, path):
     realpaths = [os.path.realpath(os.path.join(dirpath, d)) for d in dirnames]
     explored.update(realpaths)
 
-    assert dirpath.startswith(package_path)
+    assert dirpath.startswith(start_path)
     base_module_name = os.path.relpath(dirpath, base_path).split(os.path.sep)
     test_modules.extend(['.'.join(base_module_name
                                   + [inspect.getmodulename(filename)])
@@ -199,32 +234,8 @@ def load_module(modname):
   return mod
 
 
-def get_test_gens_directory(path, cwd):
-  """Given a path, return list of MultiTest or Test instances.
-
-  See UnittestTestCase for possible return values.
-
-  This function loads modules, thus no two conflicting packages (like appengine)
-  can be loaded at the same time: use separate processes for that.
-  """
-  assert isinstance(path, basestring), 'path must be a string'
-  assert os.path.isdir(path), 'path is not a directory: %s' % path
-  sys.path.insert(0, os.path.abspath(path))
-
-  test_gens = []
-  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'))):
-      test_gens += get_test_gens_package(abs_filename, cwd,
-                                         update_syspath=False)
-  return test_gens
-
-
-def get_test_gens_package(package, cwd, update_syspath=True):
-  """Given a path, return list of MultiTest or Test instances.
+def get_test_gens_package(testing_context, subpath=None):
+  """Given a testing context, return list of generators of *Test instances.
 
   See UnittestTestCase for possible return values.
 
@@ -232,24 +243,21 @@ def get_test_gens_package(package, cwd, update_syspath=True):
   should be loaded at the same time: use separate processes for that.
 
   Args:
-    package (str): path to a Python package.
-    update_syspath (boolean): if True, the parent directory of 'package' is
-      prepended to sys.path.
-  """
-  assert isinstance(package, basestring), "package name should be a string."
-  assert os.path.isfile(os.path.join(package, '__init__.py')), \
-         "'package' is not pointing to a package. It must be a " + \
-         "path to a directory containing a __init__.py file."
+    testing_context (PackageTestingContext): what to test.
+    subpath (str): relative path in the tested package to restrict the search
+      to. Relative to
+      os.path.join(testing_context.cwd, testing_context.package_name)

[agable, 2014/11/12 03:15:09]

Haven't read all the way down yet, but it seems like the subpath could be a
field on the TestingContext too.

 
+  Returns:
+    gens_list (list of generator of tests): tests are instances of Test
+      or MultiTest.
+  """
   test_gens = []
-  path = os.path.abspath(os.path.dirname(package))
-  if update_syspath:
-    sys.path.insert(0, path)
 
-  package_name = os.path.split(package.rstrip(os.path.sep))[-1]
-
-  for modname in walk_package(package_name, path):
-    with use_chdir(cwd):
+  # TODO(pgervais) add filtering on test names (use testing_context.filters)
+  for modname in walk_package(testing_context.package_name,
+                              testing_context.cwd, subpath=subpath):
+    with use_chdir(testing_context.cwd):
       mod = load_module(modname)
     for obj in mod.__dict__.values():
       if util.is_test_generator(obj):
@@ -259,89 +267,85 @@ def get_test_gens_package(package, cwd, update_syspath=True):
   return test_gens
 
 
-def gen_loop_process(gens, test_queue, result_queue, opts, kill_switch,
-                     cover_ctx, temp_dir):
+def gen_loop_process(testing_contexts, test_queue, result_queue, opts,
+                     kill_switch, cover_ctx, temp_dir):
   """Generate `Test`s from |gens|, and feed them into |test_queue|.
 
   Non-Test instances will be translated into `UnknownError` objects.
 
   Args:
-    gens: list of generators yielding Test() instances.
+    testing_contexts (list of PackageTestingContext): describe tests to
+      process.

[agable, 2014/11/12 03:15:09]

Indent +4 for continuation? I dunno.

     test_queue (multiprocessing.Queue):

[agable, 2014/11/12 03:15:10]

Love these args with no descriptions

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

I think this was my fault :)

[pgervais, 2014/11/13 00:28:26]

This time I think it was mine, I added them last time because the type
information is useful, and the description awkward.

     result_queue (multiprocessing.Queue):
     opts (argparse.Namespace):
     kill_switch (multiprocessing.Event):
     cover_ctx (cover.CoverageContext().create_subprocess_context)
   """
-  tempfile.tempdir = temp_dir
-
-  # Implicitly append '*' to globs that don't specify it.
-  globs = ['%s%s' % (g, '*' if '*' not in g else '') for g in opts.test_glob]
-
-  matcher = re.compile(
-      '^%s$' % '|'.join('(?:%s)' % glob.fnmatch.translate(g)
-                        for g in globs if g[0] != '-'))
-  if matcher.pattern == '^$':
-    matcher = re.compile('^.*$')
-
-  neg_matcher = re.compile(
-      '^%s$' % '|'.join('(?:%s)' % glob.fnmatch.translate(g[1:])
-                        for g in globs if g[0] == '-'))
 
   SENTINEL = object()
+  tempfile.tempdir = temp_dir
 
   def generate_tests():
-    paths_seen = set()
     seen_tests = False
     try:
-      for gen in gens:
-        gen_cover_ctx = cover_ctx(include=util.get_cover_list(gen))
-
-        with gen_cover_ctx:
-          gen_inst = gen()
-
-        while not kill_switch.is_set():
-          with gen_cover_ctx:
-            root_test = next(gen_inst, SENTINEL)
-
-          if root_test is SENTINEL:
-            break
-
-          if kill_switch.is_set():
-            break
-
-          ok_tests = []
-
-          if isinstance(root_test, MultiTest):
-            subtests = root_test.tests
-          else:
-            subtests = [root_test]
-
-          for subtest in subtests:
-            if not isinstance(subtest, Test):
-              result_queue.put_nowait(
-                  UnknownError(
-                      'Got non-[Multi]Test isinstance from generator: %r.'
-                      % subtest))
-              continue
-
-            test_path = subtest.expect_path()
-            if test_path is not None and test_path in paths_seen:
-              result_queue.put_nowait(
-                  TestError(subtest, 'Duplicate expectation path.'))
-            else:
-              if test_path is not None:
-                paths_seen.add(test_path)
-              name = subtest.name
-              if not neg_matcher.match(name) and matcher.match(name):
-                ok_tests.append(subtest)
-
-          if ok_tests:
-            seen_tests = True
-            yield root_test.restrict(ok_tests)
+      for testing_context in testing_contexts:
+        for subpath, matcher in testing_context.itermatchers():
+          paths_seen = set()
+
+          with cover_ctx:
+            gens = get_test_gens_package(testing_context, subpath=subpath)
+
+          for gen in gens:
+            gen_cover_ctx = cover_ctx(include=util.get_cover_list(gen))
+
+            with gen_cover_ctx:
+              gen_inst = gen()
+
+            while not kill_switch.is_set():
+              with gen_cover_ctx:
+                root_test = next(gen_inst, SENTINEL)
+
+              if root_test is SENTINEL:
+                break
+
+              if kill_switch.is_set():

[agable, 2014/11/12 03:15:09]

I know this is copied, but doesn't it make sense to check the kill switch first,
so you don't waste any effort?

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

We do that in the loop condition.

+                break
+
+              ok_tests = []
+
+              if isinstance(root_test, MultiTest):
+                subtests = root_test.tests
+              else:
+                subtests = [root_test]
+
+              for subtest in subtests:
+                if not isinstance(subtest, Test):
+                  result_queue.put_nowait(
+                      UnknownError(
+                          'Got non-[Multi]Test isinstance from generator: %r.'
+                          % subtest))
+                  continue
+
+                test_path = subtest.expect_path()
+                if test_path is not None and test_path in paths_seen:
+                  result_queue.put_nowait(
+                      TestError(subtest, 'Duplicate expectation path.'))
+                else:
+                  if test_path is not None:
+                    paths_seen.add(test_path)
+                  name = subtest.name
+                  # if not neg_matcher.match(name) and matcher.match(name):
+                  if matcher.match(name):
+                    ok_tests.append(subtest)
+
+              if ok_tests:
+                seen_tests = True
+                yield root_test.restrict(ok_tests)
 
       if not seen_tests:
         result_queue.put_nowait(NoMatchingTestsError())
+
     except KeyboardInterrupt:
       pass
 
@@ -439,8 +443,8 @@ def run_loop_process(test_queue, result_queue, opts,
     result_queue.put_nowait)
 
 
-def result_loop_single_path(cover_ctx, kill_switch, result_queue, opts,
-                            path, path_is_package):
+def result_loop_single_context(cover_ctx, kill_switch, result_queue, opts,
+                               processing_context):
   """Run the specified operation on a single path.
 
   The path provided by the `path` argument is considered to be either a Python
@@ -458,22 +462,9 @@ def result_loop_single_path(cover_ctx, kill_switch, result_queue, opts,
     kill_switch (multiprocessing.Event):
     result_queue (multiprocessing.Queue):
     opts: output of argparse.ArgumentParser.parse_args (see main.py)
-    path (str): path a a Python package or a directory containing Python
-       packages.
-    path_is_package (boolean): tells whether 'path' is a package or not.
-  """
-  assert isinstance(path, basestring), 'path must be a string'
-
-  if path_is_package:
-    work_path = os.path.dirname(os.path.abspath(path))
-  else:
-    work_path = path
-
-  with cover_ctx:
-    if path_is_package:
-      test_gens = get_test_gens_package(path, work_path)
-    else:
-      test_gens = get_test_gens_directory(path, work_path)
+    processing_context (ProcessingContext): the task to perform.
+    """
+  sys.path.insert(0, processing_context.cwd)
 
   # This flag is set when test generation has finished.
   test_gen_finished = multiprocessing.Event()
@@ -481,7 +472,7 @@ def result_loop_single_path(cover_ctx, kill_switch, result_queue, opts,
 
   with TempDir() as temp_dir:
     test_gen_args = (
-        test_gens, test_queue, result_queue, opts,
+        processing_context.testing_contexts, test_queue, result_queue, opts,
         kill_switch, cover_ctx, temp_dir
     )
 
@@ -494,7 +485,7 @@ def result_loop_single_path(cover_ctx, kill_switch, result_queue, opts,
               target=run_loop_process,
               args=(test_queue, result_queue, opts,
                     kill_switch, test_gen_finished, cover_ctx, temp_dir,
-                    work_path),
+                    processing_context.cwd),
               name='run_loop_process %d' % job_num)
           for job_num in xrange(opts.jobs)
       ]
@@ -511,6 +502,169 @@ 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:
+    '<path>:<glob>'. The path should point to a directory or a file inside
+    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.
+
+    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:
+      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):
+  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 a relative path to a subdirectory of
+    #   os.path.join(self.cwd, self.package_name) where to look for tests for.

[agable, 2014/11/12 03:15:10]

"in which to look for tests"

[pgervais, 2014/11/13 00:28:26]

Done. Thanks for the English review!

+    # Only tests whose name matches the glob are kept.
+    self.filters = filters
+
+  def itermatchers(self):
+    """Iterate over all filters, and yield matchers for each of them.
+
+    Returns:

[agable, 2014/11/12 03:15:09]

"Returns" isn't exactly accurate for a generator.

[pgervais, 2014/11/13 00:28:26]

Done.

+      path (str): restrict test listing to this subpackage.
+      matcher (SRE_Pattern): whitelist matcher
+    """
+    for filt in self.filters:
+      # Implicitely surround globs with *
+
+      one_glob = '%s%s' % (filt[1], '*' if '*' not in filt[1] else '')
+      matcher = re.compile('^(?:%s)$' % glob.fnmatch.translate(one_glob))
+
+      ## matcher = re.compile(

[agable, 2014/11/12 03:15:09]

Not sure what these commented-out segments are/were for.

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

I think they were copied from above and then translated

[pgervais, 2014/11/13 00:28:26]

Removed them.

+      ##   '^%s$' % '|'.join('(?:%s)' % glob.fnmatch.translate(g)
+      ##                     for g in globs if g[0] != '-'))
+      if matcher.pattern == '^$':
+        matcher = re.compile('^.*$')
+
+      ## neg_matcher = re.compile(
+      ##   '^%s$' % '|'.join('(?:%s)' % glob.fnmatch.translate(g[1:])
+      ##                     for g in globs if g[0] == '-'))
+      yield filt[0], matcher
+
+  @classmethod
+  def from_path(cls, path, filters='*'):
+    path = os.path.abspath(path)
+    if not os.path.exists(path):
+      raise ValueError('Path does not exist: %s' % path)
+    cwd = get_python_root(path)
+    package_name = os.path.relpath(path, cwd).split(os.path.sep)[0]
+    # list of (path, filter) pairs.

[agable, 2014/11/12 03:15:09]

This comment is out of place. I think it belongs about 5 lines down above
filters =

[pgervais, 2014/11/13 00:28:26]

Dropped

+    # The path is where to look for tests for. Only tests whose name matches the

[agable, 2014/11/12 03:15:09]

"where to look for tests for" again

[pgervais, 2014/11/13 00:28:26]

Done.

+    # glob are kept.
+    relpath = os.path.relpath(path, os.path.join(cwd, package_name))
+
+    if isinstance(filters, basestring):
+      filters = [(relpath, filters)]
+    else:
+      filters = [(relpath, 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, \

[agable, 2014/11/12 03:15:10]

Not clear why here you use assert, while above and below you use 'if not ...:
raise'.

[pgervais, 2014/11/13 00:28:26]

Good catch. Fixed.

+           'from_context_list processes contexts with the same working '\

[agable, 2014/11/12 03:15:09]

See comment below; this should be something like 'from_context_list was given
process contexts containing the following cwds (...), but can only except
contexts which all share a single cwd.'

[pgervais, 2014/11/13 00:28:26]

Done.

+           '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.'
+
+    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"')

[agable, 2014/11/12 03:15:10]

I don't think that ValueError is the right thing to raise here. I would be more
appropriate if the cwd could be said to be the 'value' of the TestingContext,
but I'm not sure that's intuitive. Whether you keep this or define your own
exception class, make the error message more helpful: state which package had
the wrong cwd, and what cwd it should have had instead.

Actually, the above comment applies to your exceptions and asserts throughout --
they should include helpful debugging information instead of just saying "this
went wrong but I won't tell you when or for what".

[pgervais, 2014/11/13 00:28:26]

I've fixed the error messages in the above two places.

As for ValueError, while I agree it can be too vague, I also think there is a
value to having some general classes of exception. In the present case, I don't
think I would have a need for a different class here. The argument is that the
root cause is that the provided value of an argument is wrong. It doesn't change
much to have a way to say 'the object you provided is the right one, but one
internal value in there is wrong'. 

+      groups.setdefault(context.package_name, []).append(context)
+
+    self.testing_contexts = [PackageTestingContext.from_context_list(contexts)
+                             for contexts in groups.itervalues()]
+
+
+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 +674,8 @@ def result_loop(cover_ctx, opts):
   The operation to perform (list/test/debug/train) is defined by opts.handler.
   """
 
+  processing_contexts = get_runtime_contexts(opts.test_glob)
+
   def ensure_echo_on():
     """Restore echo on in the terminal.
 
@@ -556,18 +712,10 @@ def result_loop(cover_ctx, opts):
 
   procs = [
     multiprocessing.Process(
-      target=result_loop_single_path,
-      args=(cover_ctx, kill_switch, result_queue, opts, os.path.abspath(p),
-            False)
-      )
-    for p in opts.directory
-    ] + [
-    multiprocessing.Process(
-      target=result_loop_single_path,
-      args=(cover_ctx, kill_switch, result_queue, opts, os.path.abspath(p),
-            True)
+      target=result_loop_single_context,
+      args=(cover_ctx, kill_switch, result_queue, opts, c)
       )
-    for p in opts.package
+    for c in processing_contexts
     ]
 
   error = False