profile.py: Don't allow accidental reassignments of TestDict keys

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.

The error message will compare the tests by command, if the two tests
have different commands then the commands will be printed for easy
visual comparison. This helps to identify whether a test is being added
twice (in a loop for example) or if two different tests are being
assigned the same name.

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
v4: - Use a incremented value instead of True false for the context
      manager, this allows the allow_reassignment contextmanager to be
      nested.  (Ilia)
v5: - Add comparison methods to the Test class
    - Use said comparisons to make the error reporting more verbose

Signed-off-by: Dylan Baker <dylanx.c.baker@intel.com>
Reviewed-by: Ilia Mirkin <imirkin@alum.mit.edu> (v4)

4 files changed, 183 insertions, 19 deletions

diff --git a/framework/profile.py b/framework/profile.py
index 1384bbdfe..d5d140c4c 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,23 @@ __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):
+        # This counter is incremented once when the allow_reassignment context
+        # 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)
+
     def __setitem__(self, key, value):
         """Enforce types on set operations.
 
@@ -67,14 +77,37 @@ 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))
-
-        super(TestDict, self).__setitem__(key.lower(), 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)))
+
+        # 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:
+            if self[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))
+                )
+            else:
+                error = "and both tests are the same."
+
+            raise TestDictError(
+                "A test has already been asigned the name: {}\n{}".format(
+                    key, error))
+
+        super(TestDict, self).__setitem__(key, value)
 
     def __getitem__(self, key):
         """Lower the value before returning."""
@@ -84,6 +117,25 @@ 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.
+
+        It is safe to nest this contextmanager.
+
+        It is not safe to use this context manager in a threaded application
+
+        """
+        self.__allow_reassignment += 1
+        yield
+        self.__allow_reassignment -= 1
+
 
 class TestProfile(object):
     """ Class that holds a list of tests for execution
@@ -354,6 +406,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
@@ -370,16 +429,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/test/base.py b/framework/test/base.py
index efc20cbe3..8a4bf99b7 100644
--- a/framework/test/base.py
+++ b/framework/test/base.py
@@ -337,6 +337,12 @@ class Test(object):
 
         return error
 
+    def __eq__(self, other):
+        return self.command == other.command
+
+    def __ne__(self, other):
+        return not self == other
+
 
 class WindowResizeMixin(object):
     """ Mixin class that deals with spurious window resizes
diff --git a/framework/tests/profile_tests.py b/framework/tests/profile_tests.py
index 74a255e80..95f8ddafb 100644
--- a/framework/tests/profile_tests.py
+++ b/framework/tests/profile_tests.py
@@ -255,3 +255,99 @@ def test_testprofile_groupmanager_name_str():
         g('abc')
 
     nt.ok_(grouptools.join('foo', 'abc') in prof.test_list)
+
+
+@nt.raises(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'])
+
+
+@nt.raises(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
+
+
+@nt.raises(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'])
+
+
+@nt.raises(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'])
+
+
+@utils.no_error
+def test_testprofile_allow_reassignemnt_stacked():
+    """profile.TestDict.allow_reassignment: check stacking cornercase.
+
+    There is an odd corner case in the original (obvious) implmentation of this
+    function, If one opens two context managers and then returns from the inner
+    one assignment will not be allowed, even though one is still inside the
+    first context manager.
+
+    """
+    test = profile.TestDict()
+    test['a'] = utils.Test(['foo'])
+    with test.allow_reassignment:
+        with test.allow_reassignment:
+            pass
+        test['a'] = utils.Test(['bar'])
+
+    nt.ok_(test['a'].command == ['bar'])
diff --git a/tests/quick.py b/tests/quick.py
index 5393a2be9..d0aca0215 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)