[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:26:44 PDT 2015
Okay, I turned it into a stack locally, it's not a big change, but I can
push out a v4 if you care, though it will probably be Monday before I
have any more time.
Dylan
On Fri, Mar 20, 2015 at 01:19:32PM -0700, Dylan Baker wrote:
> 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/865c79f5/attachment.sig>
More information about the Piglit
mailing list