[Piglit] [PATCH 7/7] profile.py: Don't allow accidental reassignments of TestDict keys

[Dylan Baker]

On Thu, Mar 05, 2015 at 02:42:51PM -0500, Ilia Mirkin wrote:
> On Thu, Mar 5, 2015 at 2:34 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.
> >
> > Signed-off-by: Dylan Baker <dylanx.c.baker at intel.com>
> > ---
> >  framework/profile.py             | 63 ++++++++++++++++++++++++++++-----
> >  framework/tests/profile_tests.py | 75 ++++++++++++++++++++++++++++++++++++++++
> >  tests/quick.py                   | 15 ++++----
> >  3 files changed, 138 insertions(+), 15 deletions(-)
> >
> > diff --git a/framework/profile.py b/framework/profile.py
> > index e8e8ba1..edd1d90 100644
> > --- a/framework/profile.py
> > +++ b/framework/profile.py
> > @@ -48,12 +48,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,19 +75,49 @@ class TestDict(dict):  # pylint: disable=too-few-public-methods
> >          filesystems.
> >
> >          """
> > -        assert isinstance(key, basestring), \
> > -               "Keys must be strings, but was {}".format(type(key))
> > +        # 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 Trees or Tests
> >          # 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))
> > +        if not isinstance(value, (Test, types.NoneType)):
> > +            raise TestDictError("Values must be either a TestDict or a Test, "
> > +                                "but was {}".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 not self.__allow_reassignment:
> > +            # If there is alread a test of that value in the tree it is an error
> > +            if isinstance(value, Test) and key in self:
> 
> why do you check the type of 'value'? What else could it be?
> 
> > +                raise TestDictError(
> > +                    "A test has already been asigned the name {}".format(key))
> > +
> > +        super(TestDict, self).__setitem__(key, value)
> 
>   -ilia

It's a rebase artifact, I'll drop it. In fact, there are a couple of
other rebase artifacts in this patch. I'll roll a v2 shortly.
-------------- 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/20150305/a7f2053b/attachment.sig>