New expect_tests UI

expect_tests/pipeline.py

 # Copyright 2014 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 import contextlib
 import ConfigParser
 import glob
 import imp
 import inspect
 import logging
@@ skipping 34 matching lines @@
   def __init__(self):
     self._stream = StringIO()
 
   def reset(self):
     self._stream = StringIO()
 
   def __getattr__(self, key):
     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'.
 
   If path is None, search for a package in sys.path.
   Otherwise, look for a direct subdirectory of path.
 
   If no package is found, returns None.
   """
   if path is None:
     _, package_path, _ = imp.find_module(package_name)
@@ skipping 214 matching lines @@
   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] == '-'))

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

I think neg_matcher got lost in the new world?

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

(discussed that offline) Yes, the neg_matcher was removed because it does not
make sense with the current UI. We'll have to improve it to bring it back.
Namely:

expect_tests <action> package:testname package:-badtestname

Would run all tests that match 'testname' in 'package, _then_ run all tests that
don't match 'badtestname' in 'package', which is probably not what you want.
Instead, we should have something like:

expect_tests <action> package:testname,-badtestname

which will run all tests in package that matches 'testname' and don't match
'badtestname'. Most of the tooling for this is already in place, but I'd rather
do that in another CL.

 
   SENTINEL = object()
 
   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))
 
@@ skipping 204 matching lines @@
         p.start()
 
       gen_loop_process(*test_gen_args)
       # Signal all run_loop_process that they can exit.
       test_gen_finished.set()
 
   for p in procs:
     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.
 
   Directories and packages to process are defined in opts.directory and
   opts.package.
 
   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.
 
     This is useful when killing a pdb session with C-c.
     """
     try:
       import termios
     except ImportError:
       termios = None
     if termios:
@@ skipping 70 matching lines @@
 
     if procs:
       error = opts.handler.result_stage_loop(opts, generate_objects(procs))
   except ResultStageAbort:
     pass
 
   if not kill_switch.is_set() and not result_queue.empty():
     error = True
 
   return error, kill_switch.is_set()