[Piglit] [PATCH v3 8/8] profile.py: Don't allow accidental reassignments of TestDict keys
Dylan Baker
baker.dylan.c at gmail.com
Fri Mar 20 13:19:32 PDT 2015
On Fri, Mar 20, 2015 at 03:35:36PM -0400, Ilia Mirkin wrote:
> 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]
It's a corner case. I guess we could use a list like a stack and just
push True onto the stack each time you open a with and pop one each time
you close it. That would fix this since an empty list is Falsy.
>
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20150320/568d9ef3/attachment-0001.sig>
More information about the Piglit
mailing list