[Piglit] [PATCH v3 8/8] profile.py: Don't allow accidental reassignments of TestDict keys
Ilia Mirkin
imirkin at alum.mit.edu
Fri Mar 20 12:35:36 PDT 2015
This strategy won't work if I do like
with allow_reassignment:
with allow_reassignment:
pass
do stuff
_ideally_ they'd be counters, but... "don't do that"? [but you might
if function calls are involved]
On Fri, Mar 20, 2015 at 2:51 PM, Dylan Baker <baker.dylan.c at gmail.com> wrote:
> 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
>
> Signed-off-by: Dylan Baker <dylanx.c.baker at intel.com>
> ---
> framework/profile.py | 65 ++++++++++++++++++++++++++++------
> framework/tests/profile_tests.py | 76 ++++++++++++++++++++++++++++++++++++++++
> tests/quick.py | 15 ++++----
> 3 files changed, 138 insertions(+), 18 deletions(-)
>
> diff --git a/framework/profile.py b/framework/profile.py
> index 603e872..6fbbe43 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,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,14 +74,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)))
>
> - 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 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 +103,21 @@ 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.
> +
> + """
> + self.__allow_reassignment = True
> + yield
> + self.__allow_reassignment = False
> +
>
> class TestProfile(object):
> """ Class that holds a list of tests for execution
> @@ -350,6 +384,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
> @@ -366,16 +407,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..3e2a221 100644
> --- a/framework/tests/profile_tests.py
> +++ b/framework/tests/profile_tests.py
> @@ -255,3 +255,79 @@ 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'])
> 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.3
>
> _______________________________________________
> Piglit mailing list
> Piglit at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit
More information about the Piglit
mailing list