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 <[email protected]> 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 <[email protected]> > > --- > > 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) > > + > > + > > [email protected](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']) > > + > > + > > [email protected](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 > > + > > + > > [email protected](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']) > > + > > + > > [email protected](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 > > [email protected] > > http://lists.freedesktop.org/mailman/listinfo/piglit
signature.asc
Description: Digital signature
_______________________________________________ Piglit mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/piglit
