[v3,17/28] framework/profile: Don't alter the TestProfile before running

Submitted by Dylan Baker on Oct. 31, 2016, 5:50 p.m.

Details

Message ID 0cff9f550575fea8beeb3c965ffbd05573ad792f.1477936071.git-series.dylan@pnwbakers.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Piglit

Not browsing as part of any series.

Commit Message

Dylan Baker Oct. 31, 2016, 5:50 p.m.
This patch changes the way that the tests from TestProfile are
processed. Rather than having a process_test_list method that modifies
TestProfile.test_list, a new itertests() method creates an iterator that
yields tests that are not excluded by filters.

This saves a bit of code, increases the memory usage, and reduces
surprise.

It would be nice if there wasn't a need to create a concrete test list,
but there wouldn't be any way to get the number of tests otherwise.

Signed-off-by: Dylan Baker <dylanx.c.baker@intel.com>
---
 framework/profile.py                 | 110 ++++++++++------------------
 framework/programs/print_commands.py |   3 +-
 unittests/framework/test_profile.py  |   6 +--
 3 files changed, 42 insertions(+), 77 deletions(-)

Patch hide | download patch | download mbox

diff --git a/framework/profile.py b/framework/profile.py
index b6c2b0e..288e598 100644
--- a/framework/profile.py
+++ b/framework/profile.py
@@ -193,35 +193,6 @@  class TestDict(collections.MutableMapping):
         yield
         self.__allow_reassignment -= 1
 
-    def filter(self, callable):
-        """Filter tests out of the testdict before running.
-
-        This method destructively filters results out of the test test
-        dictionary list using the callable provided.
-
-        Arguments:
-        callable -- a callable object that returns truthy if the item remains,
-                    falsy if it should be removed
-
-        """
-        for k, v in list(six.iteritems(self)):
-            if not callable(k, v):
-                del self[k]
-
-    def reorder(self, order):
-        """Reorder the TestDict to match the order of the provided list."""
-        new = collections.OrderedDict()
-        try:
-            for k in order:
-                new[k] = self.__container[k]
-        except KeyError:
-            # If there is a name in order that isn't available in self there
-            # will be a KeyError, this is expected. In this case fail
-            # gracefully and report the error to the user.
-            raise exceptions.PiglitFatalError(
-                'Cannot reorder test: "{}", it is not in the profile.'.format(k))
-        self.__container = new
-
 
 class TestProfile(object):
     """ Class that holds a list of tests for execution
@@ -284,28 +255,6 @@  class TestProfile(object):
         """
         self._monitoring = Monitoring(monitored)
 
-    def prepare_test_list(self):
-        """ Prepare tests for running
-
-        Flattens the nested group hierarchy into a flat dictionary using '/'
-        delimited groups by calling self.flatten_group_hierarchy(), then
-        runs it's own filters plus the filters in the self.filters name
-
-        """
-        if self.forced_test_list:
-            # Remove all tests not in the test list, then reorder the tests to
-            # match the testlist. This still allows additional filters to be
-            # run afterwards.
-            self.test_list.filter(lambda n, _: n in self.forced_test_list)
-            self.test_list.reorder(self.forced_test_list)
-
-        # Filter out unwanted tests
-        self.test_list.filter(lambda n, t: all(f(n, t) for f in self.filters))
-
-        if not self.test_list:
-            raise exceptions.PiglitFatalError(
-                'There are no tests scheduled to run. Aborting run.')
-
     @contextlib.contextmanager
     def group_manager(self, test_class, group, prefix=None, **default_args):
         """A context manager to make working with flat groups simple.
@@ -397,9 +346,8 @@  class TestProfile(object):
 
         This method creates a copy with references to the original instance
         (using copy.copy), except for the test_list attribute, which is copied
-        using copy.deepcopy, which is necessary to ensure that
-        prepare_test_list only affects the right instance. This allows profiles
-        to be "subclassed" by other profiles, without modifying the original.
+        using copy.deepcopy. This allows profiles to be "subclassed" by other
+        profiles, without modifying the original.
         """
         new = copy.copy(self)
         new.test_list = copy.deepcopy(self.test_list)
@@ -407,6 +355,22 @@  class TestProfile(object):
         new.filters = copy.copy(self.filters)
         return new
 
+    def itertests(self):
+        """Iterate over tests while filtering.
+
+        This iterator is non-destructive.
+        """
+        if self.forced_test_list:
+            opts = collections.OrderedDict()
+            for n in self.forced_test_list:
+                opts[n] = self.test_list[n]
+        else:
+            opts = self.test_list  # pylint: disable=redefined-variable-type
+
+        for k, v in six.iteritems(opts):
+            if all(f(k, v) for f in self.filters):
+                yield k, v
+
 
 def load_test_profile(filename):
     """Load a python module and return it's profile attribute.
@@ -460,9 +424,11 @@  def run(profiles, logger, backend, concurrency):
     """
     chunksize = 1
 
-    for p in profiles:
-        p.prepare_test_list()
-    log = LogManager(logger, sum(len(p.test_list) for p in profiles))
+    # The logger needs to know how many tests are running. Because of filters
+    # there's no way to do that without making a concrete list out of the
+    # filters profiles.
+    profiles = [(p, list(p.itertests())) for p in profiles]
+    log = LogManager(logger, sum(len(l) for _, l in profiles))
 
     def test(name, test, profile, this_pool=None):
         """Function to call test.execute from map"""
@@ -472,32 +438,34 @@  def run(profiles, logger, backend, concurrency):
         if profile.monitoring.abort_needed:
             this_pool.terminate()
 
-    def run_threads(pool, profile, filterby=None):
+    def run_threads(pool, profile, test_list, filterby=None):
         """ Open a pool, close it, and join it """
-        iterable = six.iteritems(profile.test_list)
         if filterby:
-            iterable = (x for x in iterable if filterby(x))
+            # Although filterby could be attached to TestProfile as a filter,
+            # it would have to be removed when run_threads exits, requiring
+            # more code, and adding side-effects
+            test_list = (x for x in test_list if filterby(x))
 
         pool.imap(lambda pair: test(pair[0], pair[1], profile, pool),
-                  iterable, chunksize)
-        pool.close()
-        pool.join()
+                  test_list, chunksize)
 
-    def run_profile(profile):
+    def run_profile(profile, test_list):
         """Run an individual profile."""
         profile.setup()
         if concurrency is ConcurrentMode.full:
-            run_threads(multi, profile)
+            run_threads(multi, profile, test_list)
         elif concurrency is ConcurrentMode.none:
-            run_threads(single, profile)
+            run_threads(single, profile, test_list)
         else:
             assert concurrency is ConcurrentMode.some
             # Filter and return only thread safe tests to the threaded pool
-            run_threads(multi, profile, lambda x: x[1].run_concurrent)
+            run_threads(multi, profile, test_list,
+                        lambda x: x[1].run_concurrent)
 
             # Filter and return the non thread safe tests to the single
             # pool
-            run_threads(single, profile, lambda x: not x[1].run_concurrent)
+            run_threads(single, profile, test_list,
+                        lambda x: not x[1].run_concurrent)
         profile.teardown()
 
     # Multiprocessing.dummy is a wrapper around Threading that provides a
@@ -509,7 +477,11 @@  def run(profiles, logger, backend, concurrency):
 
     try:
         for p in profiles:
-            run_profile(p)
+            run_profile(*p)
+
+        for pool in [single, multi]:
+            pool.close()
+            pool.join()
     finally:
         log.get().summary()
 
diff --git a/framework/programs/print_commands.py b/framework/programs/print_commands.py
index 5811cd2..8accb2b 100644
--- a/framework/programs/print_commands.py
+++ b/framework/programs/print_commands.py
@@ -97,8 +97,7 @@  def main(input_):
     piglit_dir = os.path.dirname(os.path.realpath(sys.argv[0]))
     os.chdir(piglit_dir)
 
-    profile_.prepare_test_list()
-    for name, test in six.iteritems(profile_.test_list):
+    for name, test in profile_.itertests():
         assert isinstance(test, Test)
         print(args.format_string.format(
             name=name,
diff --git a/unittests/framework/test_profile.py b/unittests/framework/test_profile.py
index ea4ee70..9ac0d21 100644
--- a/unittests/framework/test_profile.py
+++ b/unittests/framework/test_profile.py
@@ -211,12 +211,6 @@  class TestTestProfile(object):
             assert fixture.test_list == new.test_list
             assert fixture.test_list is not new.test_list
 
-        def test_prepare_test_list(self, fixture):
-            """The prepare_test_list method doesn't affect both."""
-            new = fixture.copy()
-            new.prepare_test_list()
-            assert new.test_list != fixture.test_list
-
 
 class TestTestDict(object):
     """Tests for the TestDict object."""