[Piglit] [PATCH v3 15/28] framework: Pull {include, exclude}_filter out of Options

Dylan Baker dylan at pnwbakers.com
Mon Oct 31 17:50:11 UTC 2016


Since these are also just special cases of filters for the standard
TestProfile filtering mechanism, and they have a lot of unique classes.
This is just a waste, the same can be achieved with a much simpler class
structure.

Signed-off-by: Dylan Baker <dylanx.c.baker at intel.com>
---
 framework/options.py                 | 137 +----------------------
 framework/profile.py                 |  44 ++++---
 framework/programs/print_commands.py |  10 +-
 framework/programs/run.py            |  30 +++--
 unittests/framework/test_options.py  | 178 +----------------------------
 unittests/framework/test_profile.py  | 126 ++++++--------------
 6 files changed, 98 insertions(+), 427 deletions(-)

diff --git a/framework/options.py b/framework/options.py
index dc97c38..db4bf76 100644
--- a/framework/options.py
+++ b/framework/options.py
@@ -28,9 +28,7 @@ is that while you can mutate
 from __future__ import (
     absolute_import, division, print_function, unicode_literals
 )
-import collections
 import os
-import re
 
 import six
 
@@ -39,129 +37,6 @@ __all__ = ['OPTIONS']
 # pylint: disable=too-few-public-methods
 
 
-_RETYPE = type(re.compile(''))
-
-
-class _ReList(collections.MutableSequence):
-    """A list-like container that only holds RegexObjects.
-
-    This class behaves identically to a list, except that all objects are
-    forced to be RegexObjects with a flag of re.IGNORECASE (2 if one inspects
-    the object).
-
-    If inputs do not match this object, they will be coerced to becoming such
-    an object, or they assignment will fail.
-
-    """
-    def __init__(self, iterable=None):
-        self._wrapped = []
-        if iterable is not None:
-            self.extend(iterable)
-
-    @staticmethod
-    def __compile(value):
-        """Ensure that the object is properly compiled.
-
-        If the object is not a RegexObject then compile it to one, setting the
-        proper flag. If it is a RegexObject, and the flag is incorrect
-        recompile it to have the proper flags. Otherwise return it.
-
-        """
-        if not isinstance(value, _RETYPE):
-            return re.compile(value, re.IGNORECASE)
-        elif value.flags != re.IGNORECASE:
-            return re.compile(value.pattern, re.IGNORECASE)
-        return value
-
-    def __getitem__(self, index):
-        return self._wrapped[index]
-
-    def __setitem__(self, index, value):
-        self._wrapped[index] = self.__compile(value)
-
-    def __delitem__(self, index):
-        del self._wrapped[index]
-
-    def __len__(self):
-        return len(self._wrapped)
-
-    def insert(self, index, value):
-        self._wrapped.insert(index, self.__compile(value))
-
-    def __eq__(self, other):
-        """Two ReList instances are the same if their wrapped list are equal."""
-        if isinstance(other, _ReList):
-            # There doesn't seem to be a better way to do this.
-            return self._wrapped == other._wrapped  # pylint: disable=protected-access
-        raise TypeError('Cannot compare _ReList and non-_ReList object')
-
-    def __ne__(self, other):
-        return not self == other
-
-    def to_json(self):
-        """Allow easy JSON serialization.
-
-        This returns the pattern (the string or unicode used to create the re)
-        of each re object in a list rather than the RegexObject itself. This is
-        critical for JSON serialization, and thanks to the piglit_encoder this
-        is all we need to serialize this class.
-
-        """
-        return [l.pattern for l in self]
-
-
-class _FilterReList(_ReList):
-    """A version of ReList that handles group madness.
-
-    Groups are printed with '/' as a separator, but internally something else
-    may be used. This version replaces '/' with '.'.
-
-    """
-    def __setitem__(self, index, value):
-        # Replace '/' with '.', this solves the problem of '/' not matching
-        # grouptools.SEPARATOR, but without needing to import grouptools
-        super(_FilterReList, self).__setitem__(index, value.replace('/', '.'))
-
-    def insert(self, index, value):
-        super(_FilterReList, self).insert(index, value.replace('/', '.'))
-
-
-class _ReListDescriptor(object):
-    """A Descriptor than ensures reassignment of _{in,ex}clude_filter is an
-    _ReList
-
-    Without this some behavior's can get very strange. This descriptor is
-    mostly hit by testing code, but may be of use outside of testing at some
-    point.
-
-    """
-    def __init__(self, name, type_=_ReList):
-        self.__name = name
-        self.__type = type_
-
-    def __get__(self, instance, cls):
-        try:
-            return getattr(instance, self.__name)
-        except AttributeError as e:
-            new = _ReList()
-            try:
-                setattr(instance, self.__name, new)
-            except Exception:
-                raise e
-            return new
-
-    def __set__(self, instance, value):
-        assert isinstance(value, (collections.Sequence, collections.Set))
-        if isinstance(value, self.__type):
-            setattr(instance, self.__name, value)
-        else:
-            setattr(instance, self.__name, self.__type(value))
-
-    def __delete__(self, instance):
-        raise NotImplementedError('Cannot delete {} from {}'.format(
-            self.__name, instance.__class__))
-
-
 class _Options(object):  # pylint: disable=too-many-instance-attributes
     """Contains all options for a piglit run.
 
@@ -172,9 +47,6 @@ class _Options(object):  # pylint: disable=too-many-instance-attributes
 
     Options are as follows:
     execute -- False for dry run
-    include_filter -- list of compiled regex which include exclusively tests
-                      that match
-    exclude_filter -- list of compiled regex which exclude tests that match
     valgrind -- True if valgrind is to be used
     dmesg -- True if dmesg checking is desired. This forces concurrency off
     monitored -- True if monitoring is desired. This forces concurrency off
@@ -182,13 +54,8 @@ class _Options(object):  # pylint: disable=too-many-instance-attributes
     deqp_mustpass -- True to enable the use of the deqp mustpass list feature.
     """
 
-    include_filter = _ReListDescriptor('_include_filter', type_=_FilterReList)
-    exclude_filter = _ReListDescriptor('_exclude_filter', type_=_FilterReList)
-
     def __init__(self):
         self.execute = True
-        self._include_filter = _ReList()
-        self._exclude_filter = _ReList()
         self.valgrind = False
         self.dmesg = False
         self.monitored = False
@@ -216,9 +83,5 @@ class _Options(object):  # pylint: disable=too-many-instance-attributes
             if not key.startswith('_'):
                 yield key, values
 
-        # Handle the attributes that have a descriptor separately
-        yield 'include_filter', self.include_filter
-        yield 'exclude_filter', self.exclude_filter
-
 
 OPTIONS = _Options()
diff --git a/framework/profile.py b/framework/profile.py
index 2e5c084..f47282c 100644
--- a/framework/profile.py
+++ b/framework/profile.py
@@ -37,6 +37,7 @@ import itertools
 import multiprocessing
 import multiprocessing.dummy
 import os
+import re
 try:
     import enum
 except ImportError:
@@ -44,7 +45,7 @@ except ImportError:
 
 import six
 
-from framework import grouptools, exceptions, options
+from framework import grouptools, exceptions
 from framework.dmesg import get_dmesg
 from framework.log import LogManager
 from framework.monitoring import Monitoring
@@ -52,6 +53,7 @@ from framework.test.base import Test
 
 __all__ = [
     'ConcurrentMode',
+    'RegexFilter',
     'TestProfile',
     'load_test_profile',
 ]
@@ -64,6 +66,32 @@ class ConcurrentMode(enum.Enum):
     full = 2
 
 
+class RegexFilter(object):
+    """An object to be passed to TestProfile.filter.
+
+    Arguments:
+    filters -- a list of regex compiled objects.
+    """
+
+    def __init__(self, filters, inverse=False):
+        self.filters = [re.compile(f) for f in filters]
+        self.inverse = inverse
+
+    def __call__(self, name, _):  # pylint: disable=invalid-name
+        # This needs to match the signature (name, test), since it doesn't need
+        # the test instance use _.
+
+        # If self.filters is empty then return True, we don't want to remove
+        # any tests from the run.
+        if not self.filters:
+            return True
+
+        if not self.inverse:
+            return any(r.search(name) for r in self.filters)
+        else:
+            return not any(r.search(name) for r in self.filters)
+
+
 class TestDict(collections.MutableMapping):
     """A special kind of dict for tests.
 
@@ -264,22 +292,10 @@ class TestProfile(object):
         runs it's own filters plus the filters in the self.filters name
 
         """
-        def matches_any_regexp(x, re_list):
-            return any(r.search(x) for r in re_list)
-
-        # The extra argument is needed to match check_all's API
-        def test_matches(path, test):
-            """Filter for user-specified restrictions"""
-            return ((not options.OPTIONS.include_filter or
-                     matches_any_regexp(path, options.OPTIONS.include_filter))
-                    and not matches_any_regexp(path, options.OPTIONS.exclude_filter))
-
-        filters = self.filters + [test_matches]
-
         def check_all(item):
             """ Checks group and test name against all filters """
             path, test = item
-            for f in filters:
+            for f in self.filters:
                 if not f(path, test):
                     return False
             return True
diff --git a/framework/programs/print_commands.py b/framework/programs/print_commands.py
index 6e68eb5..5811cd2 100644
--- a/framework/programs/print_commands.py
+++ b/framework/programs/print_commands.py
@@ -86,15 +86,17 @@ def main(input_):
                         help="Path to results folder")
     args = parser.parse_args(input_)
 
-    options.OPTIONS.exclude_filter = args.exclude_tests
-    options.OPTIONS.include_filter = args.include_tests
+    profile_ = profile.load_test_profile(args.testProfile)
+
+    if args.exclude_tests:
+        profile_.filters.append(profile.RegexFilter(args.exclude_tests))
+    if args.include_tests:
+        profile_.filters.append(profile.RegexFilter(args.include_tests))
 
     # Change to the piglit's path
     piglit_dir = os.path.dirname(os.path.realpath(sys.argv[0]))
     os.chdir(piglit_dir)
 
-    profile_ = profile.load_test_profile(args.testProfile)
-
     profile_.prepare_test_list()
     for name, test in six.iteritems(profile_.test_list):
         assert isinstance(test, Test)
diff --git a/framework/programs/run.py b/framework/programs/run.py
index e9192bb..e1b3da3 100644
--- a/framework/programs/run.py
+++ b/framework/programs/run.py
@@ -23,12 +23,13 @@ from __future__ import (
     absolute_import, division, print_function, unicode_literals
 )
 import argparse
-import sys
+import ctypes
 import os
 import os.path as path
-import time
-import ctypes
+import re
 import shutil
+import sys
+import time
 
 import six
 
@@ -223,6 +224,8 @@ def _create_metadata(args, name):
     opts['profile'] = args.test_profile
     opts['log_level'] = args.log_level
     opts['concurrent'] = args.concurrent.name
+    opts['include_filter'] = args.include_tests
+    opts['exclude_filter'] = args.exclude_tests
     if args.platform:
         opts['platform'] = args.platform
 
@@ -277,8 +280,6 @@ def run(input_):
         args.concurrent = profile.ConcurrentMode.none
 
     # Pass arguments into Options
-    options.OPTIONS.exclude_filter = args.exclude_tests
-    options.OPTIONS.include_filter = args.include_tests
     options.OPTIONS.execute = args.execute
     options.OPTIONS.valgrind = args.valgrind
     options.OPTIONS.dmesg = args.dmesg
@@ -335,6 +336,13 @@ def run(input_):
         for p in profiles:
             p.monitoring = args.monitored
 
+    for p in profiles:
+        if args.exclude_tests:
+            p.filters.append(profile.RegexFilter(args.exclude_tests),
+                             inverse=True)
+        if args.include_tests:
+            p.filters.append(profile.RegexFilter(args.include_tests))
+
     time_elapsed = TimeAttribute(start=time.time())
 
     profile.run(profiles, args.log_level, backend, args.concurrent)
@@ -366,8 +374,6 @@ def resume(input_):
     _disable_windows_exception_messages()
 
     results = backends.load(args.results_path)
-    options.OPTIONS.exclude_filter = results.options['exclude_filter']
-    options.OPTIONS.include_filter = results.options['include_filter']
     options.OPTIONS.execute = results.options['execute']
     options.OPTIONS.valgrind = results.options['valgrind']
     options.OPTIONS.dmesg = results.options['dmesg']
@@ -407,7 +413,15 @@ def resume(input_):
         if options.OPTIONS.monitored:
             p.monitoring = options.OPTIONS.monitored
 
-        p.filters.append(lambda n, _: n not in exclude_tests)
+        if exclude_tests:
+            p.filters.append(lambda n, _: n not in exclude_tests)
+        if results.options['exclude_filter']:
+            p.filters.append(
+                profile.RegexFilter(results.options['exclude_filter'],
+                                    inverse=True))
+        if results.options['include_filter']:
+            p.filters.append(
+                profile.RegexFilter(results.options['include_filter']))
 
     # This is resumed, don't bother with time since it won't be accurate anyway
     profile.run(
diff --git a/unittests/framework/test_options.py b/unittests/framework/test_options.py
index 65ff946..bf296c1 100644
--- a/unittests/framework/test_options.py
+++ b/unittests/framework/test_options.py
@@ -23,9 +23,6 @@
 from __future__ import (
     absolute_import, division, print_function, unicode_literals
 )
-import re
-
-import pytest
 
 from framework import options
 
@@ -33,180 +30,6 @@ from framework import options
 # pylint: disable=invalid-name
 # pylint: disable=no-self-use
 
-_RETYPE = type(re.compile(''))
-
-
-def test_ReList_iterable_argument():
-    """options._ReList: handles an iterable argument correctly"""
-    test = options._ReList(['foo'])
-    assert isinstance(test[0], _RETYPE)
-
-
-class TestReList(object):
-    """Tests for the ReList class.
-
-    These particular tests don't mutate the state of ReList, and thus can be
-    run with the same instance over and over, other tests that do mutate the
-    state need a per test ReList instance.
-
-    """
-    @classmethod
-    def setup_class(cls):
-        cls.test = options._ReList(['foo'])
-
-    def test_eq(self):
-        """Test options._ReList.__eq__."""
-        test1 = ['foo']
-        test2 = options._ReList(['foo'])
-
-        with pytest.raises(TypeError):
-            assert self.test == test1
-
-        assert self.test == test2
-
-    def test_ne(self):
-        """Test hoptions._ReList.__ne__."""
-        test1 = ['bar']
-        test2 = options._ReList(['bar'])
-
-        with pytest.raises(TypeError):
-            assert self.test != test1
-
-        assert self.test != test2
-
-    def test_getitem(self):
-        """options._ReList.__getitem__: returns expected value."""
-        assert isinstance(self.test[0], _RETYPE)
-
-    def test_flags(self):
-        """options._ReList.__getitem__: sets flags correctly."""
-        assert self.test[0].flags & re.IGNORECASE != 0
-
-    def test_len(self):
-        """options._ReList.len: returns expected values."""
-        assert len(self.test) == 1
-
-    def test_to_json(self):
-        """options._ReList.to_json: returns expected values."""
-        assert self.test.to_json() == ['foo']
-
-
-class TestReListMutate(object):
-    """Tests for ReList that mutate state."""
-    test = None
-
-    def setup(self):
-        self.test = options._ReList(['foo'])
-
-    def test_relist_insert(self):
-        """options._ReList.len: inserts value as expected"""
-        obj = re.compile('bar', re.IGNORECASE)
-
-        self.test.insert(0, obj)
-
-        assert self.test[0] == obj
-
-    def test_relist_delitem(self):
-        """options._ReList.len: removes value as expected"""
-        del self.test[0]
-
-        assert len(self.test) == 0
-
-    def test_relist_setitem(self):
-        """options._ReList.__setitem__: replaces values"""
-        sentinel = re.compile('bar')
-        self.test[0] = sentinel
-
-        # The pattern must be tested because the flags on the re object might
-        # require it to be recompiled, thus they might not be the same object,
-        # or even be equal according to python (though they are for the
-        # purposes of this test)
-        assert self.test[0].pattern == sentinel.pattern
-
-
-class TestReListDescriptor(object):
-    """Test the ReListDescriptor class.
-
-    Since this class is a descriptor it needs to be attached to an object at
-    the class level.
-
-    """
-    test = None
-
-    @classmethod
-    def setup_class(cls):
-        """Create a test object."""
-        class _Test(object):
-            desc = options._ReListDescriptor('test_desc')
-            notexists = options._ReListDescriptor('test_notexists')
-
-            def __init__(self):
-                self.test_desc = options._ReList()
-
-        cls._test = _Test
-
-    def setup(self):
-        self.test = self._test()
-
-    def test_get_exists(self):
-        """options._ReListDescriptor.__get__: Returns value if it exists."""
-        assert self.test.desc == self.test.test_desc
-
-    def test_get_not_exists(self):
-        """options._ReListDescriptor.__get__: Returns new _ReList if it doesn't
-        exists."""
-        assert self.test.notexists == self.test.test_notexists  # pylint: disable=no-member
-
-    def test_get_not_exists_fail(self, mocker):
-        """options._ReListDescriptor.__get__: Raises AttributError if name
-        doesn't exist and can't be created."""
-        mocker.patch('framework.options.setattr',
-                     mocker.Mock(side_effect=Exception),
-                     create=True)
-
-        with pytest.raises(AttributeError):
-            self.test.notexists  # pylint: disable=pointless-statement
-
-    def test_set_relist(self):
-        """options._ReListDescriptor.__set__: assigns an ReList without
-        copying."""
-        val = options._ReList(['foo'])
-        self.test.desc = val
-        assert self.test.desc is val
-
-    def test_set_other(self):
-        """options._ReListDescriptor.__set__: converts other types to ReList"""
-        val = options._ReList(['foo'])
-        self.test.desc = ['foo']
-        assert self.test.desc == val
-
-    def test_delete(self):
-        """options._ReListDescriptor.__delete___: raises NotImplementedError"""
-        with pytest.raises(NotImplementedError):
-            del self.test.desc
-
-
-class TestFilterReList(object):
-    """Tests for FilterReList.
-
-    provides a unique instance per test, which protects against state mutation.
-
-    """
-    test = None
-
-    def setup(self):
-        self.test = options._FilterReList(['foo'])
-
-    def test_setitem(self):
-        """options._FilterReList.__setitem__: replaces '/' with '.'."""
-        self.test[0] = 'foo/bar'
-        assert self.test[0].pattern == 'foo.bar'
-
-    def test_filterrelist_insert(self):
-        """options._FilterReList.insert: replaces '/' with '.'."""
-        self.test.insert(0, 'foo/bar')
-        assert self.test[0].pattern == 'foo.bar'
-
 
 def test_options_clear():
     """options.Options.clear(): resests options values to init state."""
@@ -215,7 +38,6 @@ def test_options_clear():
     test = options._Options()
     test.execute = False
     test.sync = True
-    test.exclude_filter.append('foo')
     test.clear()
 
     assert list(iter(baseline)) == list(iter(test))
diff --git a/unittests/framework/test_profile.py b/unittests/framework/test_profile.py
index f2aa5b5..ea4ee70 100644
--- a/unittests/framework/test_profile.py
+++ b/unittests/framework/test_profile.py
@@ -23,11 +23,6 @@
 from __future__ import (
     absolute_import, division, print_function, unicode_literals
 )
-import copy
-try:
-    from unittest import mock
-except ImportError:
-    import mock
 
 import pytest
 import six
@@ -35,7 +30,6 @@ import six
 from framework import dmesg
 from framework import exceptions
 from framework import grouptools
-from framework import options
 from framework import profile
 from framework.test.gleantest import GleanTest
 from . import utils
@@ -101,86 +95,6 @@ class TestTestProfile(object):
         profile_.dmesg = False
         assert isinstance(profile_.dmesg, dmesg.DummyDmesg)
 
-    class TestPrepareTestList(object):
-        """Create tests for TestProfile.prepare_test_list filtering."""
-
-        @classmethod
-        def setup_class(cls):
-            cls.opts = None
-            cls.data = None
-            cls.__patcher = mock.patch('framework.profile.options.OPTIONS',
-                                       new_callable=options._Options)
-
-        def setup(self):
-            """Setup each test."""
-            self.data = profile.TestDict()
-            self.data[grouptools.join('group1', 'test1')] = \
-                utils.Test(['thingy'])
-            self.data[grouptools.join('group1', 'group3', 'test2')] = \
-                utils.Test(['thing'])
-            self.data[grouptools.join('group3', 'test5')] = \
-                utils.Test(['other'])
-            self.data[grouptools.join('group4', 'Test9')] = \
-                utils.Test(['is_caps'])
-            self.opts = self.__patcher.start()
-
-        def teardown(self):
-            self.__patcher.stop()
-
-        def test_matches_filter_mar_1(self):
-            """profile.TestProfile.prepare_test_list: 'not env.filter or
-            matches_any_regex()' env.filter is False.
-
-            Nothing should be filtered.
-            """
-            profile_ = profile.TestProfile()
-            profile_.test_list = self.data
-            profile_.prepare_test_list()
-
-            assert dict(profile_.test_list) == dict(self.data)
-
-        def test_matches_filter_mar_2(self):
-            """profile.TestProfile.prepare_test_list: 'not env.filter or
-            matches_any_regex()' mar is False.
-            """
-            self.opts.include_filter = ['test5']
-
-            profile_ = profile.TestProfile()
-            profile_.test_list = self.data
-            profile_.prepare_test_list()
-
-            baseline = {
-                grouptools.join('group3', 'test5'): utils.Test(['other'])}
-
-            assert dict(profile_.test_list) == baseline
-
-        def test_matches_exclude_mar(self):
-            """profile.TestProfile.prepare_test_list: 'not
-            matches_any_regexp()'.
-            """
-            self.opts.exclude_filter = ['test5']
-
-            baseline = copy.deepcopy(self.data)
-            del baseline[grouptools.join('group3', 'test5')]
-
-            profile_ = profile.TestProfile()
-            profile_.test_list = self.data
-            profile_.prepare_test_list()
-
-            assert dict(profile_.test_list) == dict(baseline)
-
-        def test_matches_include_caps(self):
-            """profile.TestProfile.prepare_test_list: matches capitalized
-            tests.
-            """
-            self.opts.exclude_filter = ['test9']
-
-            profile_ = profile.TestProfile()
-            profile_.test_list = self.data
-            profile_.prepare_test_list()
-
-            assert grouptools.join('group4', 'Test9') not in profile_.test_list
-
     class TestGroupManager(object):
         """Tests for TestProfile.group_manager."""
 
@@ -386,3 +300,43 @@ class TestTestDict(object):
                 test['a'] = utils.Test(['bar'])
 
             assert test['a'].command == ['bar']
+
+
+class TestRegexFilter(object):
+    """Tests for the RegexFilter class."""
+
+    class TestNormal(object):
+        """Tests for inverse set to False (default)."""
+
+        def test_empty(self):
+            """Returns True when no filters are provided."""
+            test = profile.RegexFilter([])
+            assert test('foobob', None)
+
+        def test_matches(self):
+            """Returns True when the test matches any regex."""
+            test = profile.RegexFilter([r'foo', r'bar'])
+            assert test('foobob', None)
+
+        def test_not_matches(self):
+            """Returns True when the test matches any regex."""
+            test = profile.RegexFilter([r'fob', r'bar'])
+            assert not test('foobob', None)
+
+    class TestInverse(object):
+        """Tests for inverse set to True."""
+
+        def test_empty(self):
+            """Returns True when no filters are provided."""
+            test = profile.RegexFilter([], inverse=True)
+            assert test('foobob', None)
+
+        def test_matches(self):
+            """Returns False when the test matches any regex."""
+            test = profile.RegexFilter([r'foo', r'bar'], inverse=True)
+            assert not test('foobob', None)
+
+        def test_not_matches(self):
+            """Returns False when the test matches any regex."""
+            test = profile.RegexFilter([r'fob', r'bar'], inverse=True)
+            assert test('foobob', None)
-- 
git-series 0.8.10


More information about the Piglit mailing list