[Piglit] [PATCH v2 8/8] profile.py: Don't allow accidental reassignments of TestDict keys
Dylan Baker
baker.dylan.c at gmail.com
Wed Apr 1 10:12:26 PDT 2015
On Mon, Mar 30, 2015 at 03:14:17PM -0400, Ilia Mirkin wrote:
> Series is Reviewed-by: Ilia Mirkin <imirkin at alum.mit.edu>
>
> Still unsure about whether the texture size stuff needs to be fixed
> up, but also can't bring myself to care about it.
So, I had enough coffee this morning (and I've had enough of looking at
mako generators for a while) that I wrote a patch to compare the two
Test objects. Guess what, they're not the same. One of them specifies
version 140 the other doesn't specify a version. Looking at the code in
textureSize.c it looks like if the stage is gs that the version will be
set to 150 regardless, so while the commands are different I think that
the two tests will actually end up being equivalent.
Here's the output, if you care:
The original test has this command: "/tmp/piglit-install/lib/piglit/bin/textureSize gs sampler1D -auto -fbo"
The new test has this command: "/tmp/piglit-install/lib/piglit/bin/textureSize 140 gs sampler1D -auto -fbo"
So I think that what I'm doing is in fact correct. So, I'll add the
extra more verbose error message and update the comment to mark that it
is in fact correct. Unless I'm missing something?
Dylan
>
> On Mon, Mar 30, 2015 at 2:54 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
> > 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
> >
> > _______________________________________________
> > 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/20150401/f5f34165/attachment.sig>
More information about the Piglit
mailing list