Refactored types to simplify pickling.

expect_tests/type_definitions.py

Index: expect_tests/type_definitions.py
diff --git a/expect_tests/type_definitions.py b/expect_tests/type_definitions.py
index cd5c89ca2ae2ef08be506cfe86ce197777a221a6..b71a732b6bb6c86c6154a53430a3c88660a0f0fe 100644
--- a/expect_tests/type_definitions.py
+++ b/expect_tests/type_definitions.py
@@ -2,6 +2,7 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+import copy
 import inspect
 import os
 import re
@@ -148,17 +149,10 @@ class FuncCall(object):
     return 'FuncCall(%r, *%r, **%r)' % (self.func, self.args, self.kwargs)
 
 
-_Test = namedtuple(
-    'Test', 'name func_call expect_dir expect_base ext covers breakpoints')
+class Test(object):
+  TEST_COVERS_MATCH = re.compile('.*/test/([^/]+)_test\.py$')
 
-TestInfo = namedtuple(
-  'TestInfo', 'name  expect_dir expect_base ext')
-
-
-class Test(_Test):
-  TEST_COVERS_MATCH = re.compile('.*/test/([^/]*)_test\.py$')
-
-  def __new__(cls, name, func_call, expect_dir=None, expect_base=None,
+  def __init__(self, name, func_call, expect_dir=None, expect_base=None,
               ext='json', covers=None, breakpoints=None, break_funcs=()):
     """Create a new test.
 
@@ -183,87 +177,127 @@ class Test(_Test):
                         mode. See |break_funcs| for an easier way to set this.
     @param break_funcs: A list of functions for which to set breakpoints.
     """
-    breakpoints = breakpoints or []
-    if not breakpoints or break_funcs:
-      for f in (break_funcs or (func_call.func,)):
-        if hasattr(f, 'im_func'):
-          f = f.im_func
-        breakpoints.append((f.func_code.co_filename,
-                            f.func_code.co_firstlineno,
-                            f.func_code.co_name))
-
+    self._func_call = func_call
+    self._name = name
     if expect_dir:
       expect_dir = expect_dir.rstrip('/')
-    return super(Test, cls).__new__(cls, name, func_call, expect_dir,
-                                    expect_base, ext, covers, breakpoints)
+    self._expect_dir = expect_dir
+    self._expect_base = expect_base
+    self._ext = ext
+    self._covers = covers
+    breakpoints = breakpoints or []
+    self._breakpoints = breakpoints

[dnj, 2014/09/17 20:10:52]

self._breakpoints = copy.copy(breakpoints or [])
...
build onto 'self._breakpoints'?

+
+    if self._func_call:
+      if not breakpoints or break_funcs:
+        breakpoints = copy.copy(breakpoints)

[dnj, 2014/09/17 20:10:52]

You create a 'breakpoints' copy and build onto it, but never retain it anywhere.
Is this supposed to be in 'self._breakpoints'?

Since this isn't a terribly difficult calculation, consider performing it in the
'breakpoints' property function, just b/c it's that much less to retain/pickle.

[pgervais, 2014/09/17 21:32:51]

Done.

+        for f in (break_funcs or (func_call.func,)):
+          if hasattr(f, 'im_func'):
+            f = f.im_func
+          breakpoints.append((f.func_code.co_filename,
+                              f.func_code.co_firstlineno,
+                              f.func_code.co_name))
 
+  @property
+  def name(self):
+    return self._name
 
-  @staticmethod
-  def covers_obj(obj):
-    test_file = inspect.getabsfile(obj)
-    covers = [test_file]
-    match = Test.TEST_COVERS_MATCH.match(test_file)
-    if match:
-      covers.append(os.path.join(
-          os.path.dirname(os.path.dirname(test_file)),
-          match.group(1) + '.py'
-      ))
-    return covers
+  @property
+  def func_call(self):
+    return self._func_call
 
-  @staticmethod
-  def expect_dir_obj(obj):
-    test_file = inspect.getabsfile(obj)
-    return os.path.splitext(test_file)[0] + '.expected'
+  @property
+  def expect_dir(self):
+    return self._expect_dir
 
-  def coverage_includes(self):
-    if self.covers is not None:
-      return self.covers
-    return self.covers_obj(self.func_call.func)
+  @property
+  def expect_base(self):
+    return self._expect_base
 
-  def expect_path(self, ext=None):
-    expect_dir = self.expect_dir
-    if expect_dir is None:
-      expect_dir = self.expect_dir_obj(self.func_call.func)
-    name = self.expect_base or self.name
-    name = ''.join('_' if c in '<>:"\\/|?*\0' else c for c in name)
-    return os.path.join(expect_dir, name + ('.%s' % (ext or self.ext)))
+  @property
+  def ext(self):
+    return self._ext
+
+  @property
+  def covers(self):
+    return self._covers
+
+  @property
+  def breakpoints(self):
+    return self._breakpoints
 
   def run(self, context=None):
     return self.func_call(context=context)
 
   def process(self, func=lambda test: test.run()):
-    """Applies |func| to the test, and yields (self, func(self)).
+    """Applies |func| to the test, and yields (self.get_info(), func(self)).
 
     For duck-typing compatibility with MultiTest.
 
     Bind(name='context') if used by your test function, is bound to None.
 
-    Used interally by expect_tests, you're not expected to call this yourself.
+    Used internally by expect_tests, you're not expected to call this yourself.
     """
-    yield self, func(self.bind(context=None))
+    yield self.get_info(), func(self.bind(context=None))
 
   def bind(self, *args, **kwargs):

[dnj, 2014/09/17 20:10:52]

Consider implementing '__copy__' or a 'copy()' function. Then this can be:
t = self.copy()
t._func_call = self._func_call.bind(*args, **kwargs)
return t

Can also use this with 'get_info'.

[pgervais, 2014/09/17 21:06:14]

I don't get the point. I think copy will be something like:

def copy(self):
   return Test(self._name,
        self._func_call,
        expect_dir=self.expect_dir,
        expect_base=self.expect_base,
        ext=self._ext,
        covers=self._covers,
        breakpoints=self._breakpoints)

So what's the gain?

-    return self._replace(func_call=self.func_call.bind(*args, **kwargs))
+    return Test(self._name,
+                self._func_call.bind(*args, **kwargs),
+                expect_dir=self.expect_dir,
+                expect_base=self.expect_base,
+                ext=self._ext,
+                covers=self._covers,
+                breakpoints=self._breakpoints)
 
   def restrict(self, tests):
     assert tests[0] is self
     return self
 
   def get_info(self):
-    """Strips test instance of hard-to-pickle stuff
+    """Strips test instance of information required for running test.
 
     Returns a TestInfo instance.
     """
-    return TestInfo(self.name, self.expect_dir, self.expect_base, self.ext)
+    return Test(self.name,
+                None,
+                expect_dir=self._expect_dir,
+                expect_base=self._expect_base,
+                ext=self._ext,
+                covers=self._covers,
+                breakpoints=self._breakpoints)
+
+  def coverage_includes(self):
+    if self._covers is not None:
+      return self._covers
+    return self.covers_obj(self._func_call.func)
 
+  @classmethod
+  def covers_obj(cls, obj):
+    test_file = inspect.getabsfile(obj)
+    covers = [test_file]
+    match = cls.TEST_COVERS_MATCH.match(test_file)
+    if match:
+      covers.append(os.path.join(
+          os.path.dirname(os.path.dirname(test_file)),
+          match.group(1) + '.py'
+      ))
+    return covers
 
-_MultiTest = namedtuple(
-    'MultiTest', 'name make_ctx_call destroy_ctx_call tests atomic')
+  def expect_path(self, ext=None):
+    expect_dir = self.expect_dir
+    if expect_dir is None:
+      expect_dir = self.expect_dir_obj(self._func_call.func)
+    name = self._expect_base or self.name
+    name = ''.join('_' if c in '<>:"\\/|?*\0' else c for c in name)
+    return os.path.join(expect_dir, name + ('.%s' % (ext or self.ext)))

[dnj, 2014/09/17 20:10:52]

(ext or self.ext,) if following explicit tupling convention.

[pgervais, 2014/09/17 21:32:50]

Done.

 
-MultiTestInfo = namedtuple('MultiTestInfo', 'name tests atomic')
+  @staticmethod
+  def expect_dir_obj(obj):
+    test_file = inspect.getabsfile(obj)
+    return os.path.splitext(test_file)[0] + '.expected'
 
 
-class MultiTest(_MultiTest):
+class MultiTest(object):
   """A wrapper around one or more Test instances.
 
   Allows the entire group to have common pre- and post- actions and an optional
@@ -285,18 +319,32 @@ class MultiTest(_MultiTest):
         either all at once, or not at all (i.e., subtests may not be filtered).
   """
 
-  def restrict(self, tests):
-    """A helper method to re-cast the MultiTest with fewer subtests.
+  def __init__(self, name, make_ctx_call, destroy_ctx_call, tests, atomic):
+    self._name = name
+    self._make_ctx_call = make_ctx_call
+    self._destroy_ctx_call = destroy_ctx_call
+    self._tests = tests
+    self._atomic = atomic
 
-    All fields will be identical except for tests. If this MultiTest is atomic,
-    then this method returns |self|.
+  @property
+  def name(self):
+    return self._name
 
-    Used interally by expect_tests, you're not expected to call this yourself.
-    """
-    if self.atomic:
-      return self
-    assert all(t in self.tests for t in tests)
-    return self._replace(tests=tests)
+  @property
+  def make_ctx_call(self):
+    return self._make_ctx_call
+
+  @property
+  def destroy_ctx_call(self):
+    return self._destroy_ctx_call
+
+  @property
+  def tests(self):
+    return self._tests
+
+  @property
+  def atomic(self):
+    return self._atomic
 
   def process(self, func=lambda test: test.run()):
     """Applies |func| to each sub-test, with properly bound context.
@@ -316,13 +364,23 @@ class MultiTest(_MultiTest):
     ctx_object = self.make_ctx_call()
     try:
       for test in self.tests:
-        yield test, func(test.bind(context=ctx_object))
+        yield test.get_info(), func(test.bind(context=ctx_object))
     finally:
       self.destroy_ctx_call.bind(context=ctx_object)()
 
-  @staticmethod
-  def expect_path(_ext=None):
-    return None
+  def restrict(self, tests):
+    """A helper method to re-cast the MultiTest with fewer subtests.
+
+    All fields will be identical except for tests. If this MultiTest is atomic,
+    then this method returns |self|.
+
+    Used internally by expect_tests, you're not expected to call this yourself.
+    """
+    if self.atomic:
+      return self
+    assert all(t in self.tests for t in tests)
+    return MultiTest(self._name, self._make_ctx_call, self._destroy_ctx_call,
+                     tests, self._atomic)
 
   def get_info(self):
     """Strips MultiTest instance of hard-to-pickle stuff
@@ -330,12 +388,14 @@ class MultiTest(_MultiTest):
     Returns a MultiTestInfo instance.
     """
     all_tests = [test.get_info() for test in self.tests]
-    test = MultiTestInfo(name=self.name,
-                         tests=all_tests,
-                         atomic=self.atomic
-                         )
+    test = MultiTest(self._name, None, None, all_tests, self._atomic)
     return test
 
+  @staticmethod
+  def expect_path(_ext=None):
+    return None
+
+
 
 class Handler(object):
   """Handler object.