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