[Piglit] [PATCH 7/7] profile.py: Don't allow accidental reassignments of TestDict keys
Dylan Baker
baker.dylan.c at gmail.com
Thu Mar 5 11:34:24 PST 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.
Signed-off-by: Dylan Baker <dylanx.c.baker at intel.com>
---
framework/profile.py | 63 ++++++++++++++++++++++++++++-----
framework/tests/profile_tests.py | 75 ++++++++++++++++++++++++++++++++++++++++
tests/quick.py | 15 ++++----
3 files changed, 138 insertions(+), 15 deletions(-)
diff --git a/framework/profile.py b/framework/profile.py
index e8e8ba1..edd1d90 100644
--- a/framework/profile.py
+++ b/framework/profile.py
@@ -48,12 +48,20 @@ __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):
+ self.__allow_reassignment = False
+ super(TestDict, self).__init__(*args, **kwargs)
+
def __setitem__(self, key, value):
"""Enforce types on set operations.
@@ -67,19 +75,49 @@ class TestDict(dict): # pylint: disable=too-few-public-methods
filesystems.
"""
- assert isinstance(key, basestring), \
- "Keys must be strings, but was {}".format(type(key))
+ # 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 Trees or Tests
# 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))
+ if not isinstance(value, (Test, types.NoneType)):
+ raise TestDictError("Values must be either a TestDict or a Test, "
+ "but was {}".format(type(value)))
- super(TestDict, self).__setitem__(key.lower(), 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()
+
+ if not self.__allow_reassignment:
+ # If there is alread a test of that value in the tree it is an error
+ if isinstance(value, Test) 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."""
return super(TestDict, self).__getitem__(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.
+
+ """
+ self.__allow_reassignment = True
+ yield
+ self.__allow_reassignment = False
+
class TestProfile(object):
""" Class that holds a list of tests for execution
@@ -335,6 +373,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
@@ -351,16 +396,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 d2878c3..23c796d 100644
--- a/framework/tests/profile_tests.py
+++ b/framework/tests/profile_tests.py
@@ -241,3 +241,78 @@ def test_testprofile_groupmanager_kwargs_overwrite():
test = prof.test_list[grouptools.join('foo', 'a')]
nt.assert_equal(test.run_concurrent, False)
+
+
+ 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."""
+ prof = profile.TestProfile()
+ prof.test_list['a/b'] = utils.Test(['foo'])
+ with prof.allow_reassignment:
+ with prof.group_manager(utils.Test, 'a') as g:
+ g(['bar'], 'b')
+
+ nt.ok_(prof.test_list['a/b'].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.1
More information about the Piglit
mailing list