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

[Ilia Mirkin]

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