[Piglit] [PATCH 1/5] framework/profile: Convert TestDict to a collections.MutableMapping

[Dylan Baker]

MutableMapping is an abstract base class that makes writing dict-like
objects easier, by providing some of the boilerplate code. This is the
recommended way to make a dict-like object, as opposed to subclassing
dict. This is because subclassing built-ins is perilous, sometimes they
call their protocol methods (or magic methods), sometimes they don't.

This change uncovered such a bug, TestDict.update would silently
overwrite other tests when calling update, without there being an code
to explicitly do that. This patch additionally fixes the test that trips
the bug and adds a test the explicitly cover it.

This is groundwork for the next patch.

Signed-off-by: Dylan Baker <dylanx.c.baker at intel.com>
---
 framework/profile.py       | 39 ++++++++++++++++++++++++---------------
 unittests/profile_tests.py | 22 +++++++++++++++++++---
 2 files changed, 43 insertions(+), 18 deletions(-)

diff --git a/framework/profile.py b/framework/profile.py
index ef1c993..6342eae 100644
--- a/framework/profile.py
+++ b/framework/profile.py
@@ -29,12 +29,13 @@ are represented by a TestProfile or a TestProfile derived object.
 from __future__ import (
     absolute_import, division, print_function, unicode_literals
 )
-import os
-import multiprocessing
-import multiprocessing.dummy
-import importlib
+import collections
 import contextlib
+import importlib
 import itertools
+import multiprocessing
+import multiprocessing.dummy
+import os
 
 import six
 
@@ -50,7 +51,7 @@ __all__ = [
 ]
 
 
-class TestDict(dict):  # pylint: disable=too-few-public-methods
+class TestDict(collections.MutableMapping):
     """A special kind of dict for tests.
 
     This dict lowers the names of keys by default
@@ -61,7 +62,7 @@ class TestDict(dict):  # pylint: disable=too-few-public-methods
         # 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)
+        self.__container = dict(*args, **kwargs)
 
     def __setitem__(self, key, value):
         """Enforce types on set operations.
@@ -77,7 +78,7 @@ class TestDict(dict):  # pylint: disable=too-few-public-methods
 
         """
         # keys should be strings
-        if not isinstance(key, six.string_types):
+        if not isinstance(key, six.text_type):
             raise exceptions.PiglitFatalError(
                 "TestDict keys must be strings, but was {}".format(type(key)))
 
@@ -92,13 +93,14 @@ class TestDict(dict):  # pylint: disable=too-few-public-methods
         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:
-            if self[key] != value:
+        if not self.__allow_reassignment and key in self.__container:
+            if self.__container[key] != value:
                 error = (
                     'Further, the two tests are not the same,\n'
                     'The original test has this command:   "{0}"\n'
                     'The new test has this command:        "{1}"'.format(
-                        ' '.join(self[key].command), ' '.join(value.command))
+                        ' '.join(self.__container[key].command),
+                        ' '.join(value.command))
                 )
             else:
                 error = "and both tests are the same."
@@ -107,15 +109,21 @@ class TestDict(dict):  # pylint: disable=too-few-public-methods
                 "A test has already been assigned the name: {}\n{}".format(
                     key, error))
 
-        super(TestDict, self).__setitem__(key, value)
+        self.__container[key] = value
 
     def __getitem__(self, key):
         """Lower the value before returning."""
-        return super(TestDict, self).__getitem__(key.lower())
+        return self.__container[key.lower()]
 
     def __delitem__(self, key):
         """Lower the value before returning."""
-        return super(TestDict, self).__delitem__(key.lower())
+        del self.__container[key.lower()]
+
+    def __len__(self):
+        return len(self.__container)
+
+    def __iter__(self):
+        return iter(self.__container)
 
     @property
     @contextlib.contextmanager
@@ -442,6 +450,7 @@ def merge_test_profiles(profiles):
 
     """
     profile = load_test_profile(profiles.pop())
-    for p in profiles:
-        profile.update(load_test_profile(p))
+    with profile.allow_reassignment:
+        for p in profiles:
+            profile.update(load_test_profile(p))
     return profile
diff --git a/unittests/profile_tests.py b/unittests/profile_tests.py
index 48e9d96..20e4d37 100644
--- a/unittests/profile_tests.py
+++ b/unittests/profile_tests.py
@@ -34,7 +34,7 @@ except ImportError:
 import nose.tools as nt
 
 from . import utils
-from framework import grouptools, dmesg, profile, exceptions, options
+from framework import grouptools, dmesg, profile, exceptions, options, exceptions
 from framework.test import GleanTest
 
 # Don't print sys.stderr to the console
@@ -107,9 +107,10 @@ def test_testprofile_update_test_list():
     profile2.test_list[group1] = utils.Test(['test3'])
     profile2.test_list[group2] = utils.Test(['test2'])
 
-    profile1.update(profile2)
+    with profile1.allow_reassignment:
+        profile1.update(profile2)
 
-    nt.assert_dict_equal(profile1.test_list, profile2.test_list)
+    nt.assert_dict_equal(dict(profile1.test_list), dict(profile2.test_list))
 
 
 class TestPrepareTestListMatches(object):
@@ -359,3 +360,18 @@ def test_testprofile_allow_reassignemnt_stacked():
         test['a'] = utils.Test(['bar'])
 
     nt.ok_(test['a'].command == ['bar'])
+
+
+ at nt.raises(exceptions.PiglitFatalError)
+def test_testdict_update_reassignment():
+    """profile.TestDict.update: Does not implictly allow reassignment"""
+    test1 = utils.Test(['test1'])
+    test2 = utils.Test(['test2'])
+
+    td1 = profile.TestDict()
+    td1['test1'] = test1
+
+    td2 = profile.TestDict()
+    td2['test1'] = test2
+
+    td1.update(td2)
-- 
2.8.2