[Piglit] [PATCH v2 8/8] profile.py: Don't allow accidental reassignments of TestDict keys

Dylan Baker baker.dylan.c at gmail.com
Mon Mar 30 11:54:19 PDT 2015


A common error in all.py is that two different tests are assigned to the
same key value in profile.test_list. This change prevents a key from
being reassigned to a new value on accident, but provides a mechanism to
allow reassignment, using a context manager. This allows tests like the
image_load_store tests in quick.py to be overwritten, but requires the
author to mark that they know that they are overwriting tests, and that
it's intentional.

This also adds some tests for TestDict.

v2: - remove some rebase artifacts, this makes the code a little simpler
      and a little cleaner
v3: - rebase on master, includes grouptools separator changes
v4: - Use a incremented value instead of True false for the context
      manager, this allows the allow_reassignment contextmanager to be
      nested.  (Ilia)

Signed-off-by: Dylan Baker <dylanx.c.baker at intel.com>
---
 framework/profile.py             | 72 +++++++++++++++++++++++++-----
 framework/tests/profile_tests.py | 96 ++++++++++++++++++++++++++++++++++++++++
 tests/quick.py                   | 15 ++++---
 3 files changed, 165 insertions(+), 18 deletions(-)

diff --git a/framework/profile.py b/framework/profile.py
index 1384bbd..32d0c15 100644
--- a/framework/profile.py
+++ b/framework/profile.py
@@ -32,7 +32,6 @@ import sys
 import multiprocessing
 import multiprocessing.dummy
 import importlib
-import types
 import contextlib
 import itertools
 
@@ -48,12 +47,23 @@ __all__ = [
 ]
 
 
+class TestDictError(Exception):
+    pass
+
+
 class TestDict(dict):  # pylint: disable=too-few-public-methods
     """A special kind of dict for tests.
 
     This dict lowers the names of keys by default
 
     """
+    def __init__(self, *args, **kwargs):
+        # This counter is incremented once when the allow_reassignment context
+        # manager is opened, and decremented each time it is closed. This
+        # allows stacking of the context manager
+        self.__allow_reassignment = 0
+        super(TestDict, self).__init__(*args, **kwargs)
+
     def __setitem__(self, key, value):
         """Enforce types on set operations.
 
@@ -67,14 +77,26 @@ class TestDict(dict):  # pylint: disable=too-few-public-methods
         filesystems.
 
         """
-        assert isinstance(key, basestring), \
-            "Keys must be strings, but was {}".format(type(key))
-        # None is required to make empty assignment work:
-        # foo = Tree['a']
-        assert isinstance(value, (Test, types.NoneType)), \
-            "Values must be either a Test, but was {}".format(type(value))
+        # keys should be strings
+        if not isinstance(key, basestring):
+            raise TestDictError("Keys must be strings, but was {}".format(
+                type(key)))
+
+        # Values should either be more Tests
+        if not isinstance(value, Test):
+            raise TestDictError(
+                "Values must be a Test, but was a {}".format(type(value)))
+
+        # This must be lowered before the following test, or the test can pass
+        # in error if the key has capitals in it.
+        key = key.lower()
 
-        super(TestDict, self).__setitem__(key.lower(), value)
+        # If there is already a test of that value in the tree it is an error
+        if not self.__allow_reassignment and key in self:
+            raise TestDictError(
+                "A test has already been asigned the name: {}".format(key))
+
+        super(TestDict, self).__setitem__(key, value)
 
     def __getitem__(self, key):
         """Lower the value before returning."""
@@ -84,6 +106,25 @@ class TestDict(dict):  # pylint: disable=too-few-public-methods
         """Lower the value before returning."""
         return super(TestDict, self).__delitem__(key.lower())
 
+    @property
+    @contextlib.contextmanager
+    def allow_reassignment(self):
+        """Context manager that allows keys to be reassigned.
+
+        Normally reassignment happens in error, but sometimes one actually
+        wants to do reassignment, say to add extra options in a reduced
+        profile. This method allows reassignment, but only within its context,
+        making it an explict choice to do so.
+
+        It is safe to nest this contextmanager.
+
+        It is not safe to use this context manager in a threaded application
+
+        """
+        self.__allow_reassignment += 1
+        yield
+        self.__allow_reassignment -= 1
+
 
 class TestProfile(object):
     """ Class that holds a list of tests for execution
@@ -354,6 +395,13 @@ class TestProfile(object):
 
         yield adder
 
+    @property
+    @contextlib.contextmanager
+    def allow_reassignment(self):
+        """A convenience wrapper around self.test_list.allow_reassignment."""
+        with self.test_list.allow_reassignment:
+            yield
+
 
 def load_test_profile(filename):
     """ Load a python module and return it's profile attribute
@@ -370,16 +418,18 @@ def load_test_profile(filename):
     filename -- the name of a python module to get a 'profile' from
 
     """
-    mod = importlib.import_module('tests.{0}'.format(
-        os.path.splitext(os.path.basename(filename))[0]))
-
     try:
+        mod = importlib.import_module('tests.{0}'.format(
+            os.path.splitext(os.path.basename(filename))[0]))
         return mod.profile
     except AttributeError:
         print("Error: There is not profile attribute in module {0}."
               "Did you specify the right file?".format(filename),
               file=sys.stderr)
         sys.exit(2)
+    except TestDictError as e:
+        print("Error: {}".format(e.message), file=sys.stderr)
+        sys.exit(1)
 
 
 def merge_test_profiles(profiles):
diff --git a/framework/tests/profile_tests.py b/framework/tests/profile_tests.py
index 74a255e..95f8dda 100644
--- a/framework/tests/profile_tests.py
+++ b/framework/tests/profile_tests.py
@@ -255,3 +255,99 @@ def test_testprofile_groupmanager_name_str():
         g('abc')
 
     nt.ok_(grouptools.join('foo', 'abc') in prof.test_list)
+
+
+ at nt.raises(profile.TestDictError)
+def test_testdict_key_not_string():
+    """TestDict: If key value isn't a string an exception is raised.
+
+    This throws a few different things at the key value and expects an error to
+    be raised. It isn't a perfect test, but it was the best I could come up
+    with.
+
+    """
+    test = profile.TestDict()
+
+    for x in [None, utils.Test(['foo']), ['a'], {'a': 1}]:
+        test[x] = utils.Test(['foo'])
+
+
+ at nt.raises(profile.TestDictError)
+def test_testdict_value_not_valid():
+    """TestDict: If the value isn't a Tree, Test, or None an exception is raised.
+
+    Like the key test this isn't perfect, but it does try the common mistakes.
+
+    """
+    test = profile.TestDict()
+
+    for x in [{}, 'a']:
+        test['foo'] = x
+
+
+ at nt.raises(profile.TestDictError)
+def test_testdict_reassignment():
+    """TestDict: reassigning a key raises an exception."""
+    test = profile.TestDict()
+    test['foo'] = utils.Test(['foo'])
+    test['foo'] = utils.Test(['foo', 'bar'])
+
+
+ at nt.raises(profile.TestDictError)
+def test_testdict_reassignment_lower():
+    """TestDict: reassigning a key raises an exception (capitalization is ignored)."""
+    test = profile.TestDict()
+    test['foo'] = utils.Test(['foo'])
+    test['Foo'] = utils.Test(['foo', 'bar'])
+
+
+def test_testdict_allow_reassignment():
+    """TestDict: allow_reassignment works."""
+    test = profile.TestDict()
+    test['a'] = utils.Test(['foo'])
+    with test.allow_reassignment:
+        test['a'] = utils.Test(['bar'])
+
+    nt.ok_(test['a'].command == ['bar'])
+
+
+def test_testprofile_allow_reassignment():
+    """TestProfile: allow_reassignment wrapper works."""
+    prof = profile.TestProfile()
+    prof.test_list['a'] = utils.Test(['foo'])
+    with prof.allow_reassignment:
+        prof.test_list['a'] = utils.Test(['bar'])
+
+    nt.ok_(prof.test_list['a'].command == ['bar'])
+
+
+def test_testprofile_allow_reassignment_with_groupmanager():
+    """TestProfile: allow_reassignment wrapper works with groupmanager."""
+    testname = grouptools.join('a', 'b')
+    prof = profile.TestProfile()
+    prof.test_list[testname] = utils.Test(['foo'])
+    with prof.allow_reassignment:
+        with prof.group_manager(utils.Test, 'a') as g:
+            g(['bar'], 'b')
+
+    nt.ok_(prof.test_list[testname].command == ['bar'])
+
+
+ at utils.no_error
+def test_testprofile_allow_reassignemnt_stacked():
+    """profile.TestDict.allow_reassignment: check stacking cornercase.
+
+    There is an odd corner case in the original (obvious) implmentation of this
+    function, If one opens two context managers and then returns from the inner
+    one assignment will not be allowed, even though one is still inside the
+    first context manager.
+
+    """
+    test = profile.TestDict()
+    test['a'] = utils.Test(['foo'])
+    with test.allow_reassignment:
+        with test.allow_reassignment:
+            pass
+        test['a'] = utils.Test(['bar'])
+
+    nt.ok_(test['a'].command == ['bar'])
diff --git a/tests/quick.py b/tests/quick.py
index 5393a2b..d0aca02 100644
--- a/tests/quick.py
+++ b/tests/quick.py
@@ -15,13 +15,14 @@ GleanTest.GLOBAL_PARAMS += ["--quick"]
 with profile.group_manager(
         PiglitGLTest,
         grouptools.join('spec', 'arb_shader_image_load_store')) as g:
-    g(['arb_shader_image_load_store-coherency', '--quick'], 'coherency')
-    g(['arb_shader_image_load_store-host-mem-barrier', '--quick'],
-      'host-mem-barrier')
-    g(['arb_shader_image_load_store-max-size', '--quick'], 'max-size')
-    g(['arb_shader_image_load_store-semantics', '--quick'], 'semantics')
-    g(['arb_shader_image_load_store-shader-mem-barrier', '--quick'],
-      'shader-mem-barrier')
+    with profile.allow_reassignment:
+        g(['arb_shader_image_load_store-coherency', '--quick'], 'coherency')
+        g(['arb_shader_image_load_store-host-mem-barrier', '--quick'],
+          'host-mem-barrier')
+        g(['arb_shader_image_load_store-max-size', '--quick'], 'max-size')
+        g(['arb_shader_image_load_store-semantics', '--quick'], 'semantics')
+        g(['arb_shader_image_load_store-shader-mem-barrier', '--quick'],
+          'shader-mem-barrier')
 
 # These take too long
 profile.filter_tests(lambda n, _: '-explosion' not in n)
-- 
2.3.4



More information about the Piglit mailing list