Series is Reviewed-by: Ilia Mirkin <imir...@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.
On Mon, Mar 30, 2015 at 2:54 PM, Dylan Baker <baker.dyla...@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.ba...@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) > + > + > +@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']) > + > + > +@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 > + > + > +@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']) > + > + > +@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']) > + > + > +@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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/piglit _______________________________________________ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit