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 <[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
