framework: move unicode conversion handling to TestResult

This implements a python descriptor pattern to handle setting the out
and err attributes, which should always be unicode.

The familiar "property" decorator is an example of a descriptor,
although it is a more terse way of doing it. And while that could have
been done here, by not making it a property we can reuse the exact same
class for both out and err, and could theoretically use it for other
attributes.

Signed-off-by: Dylan Baker <dylanx.c.baker@intel.com>

4 files changed, 105 insertions, 23 deletions

diff --git a/framework/results.py b/framework/results.py
index 40f750cf0..2753fd580 100644
--- a/framework/results.py
+++ b/framework/results.py
@@ -50,15 +50,43 @@ class Subtests(dict):
         return res
 
 
+class StringDescriptor(object):  # pylint: disable=too-few-public-methods
+    """A Shared data descriptor class for TestResult.
+
+    This provides a property that can be passed a str or unicode, but always
+    returns a unicode object.
+
+    """
+    def __init__(self, name, default=unicode()):
+        assert isinstance(default, unicode)
+        self.__name = name
+        self.__default = default
+
+    def __get__(self, instance, cls):
+        return getattr(instance, self.__name, self.__default)
+
+    def __set__(self, instance, value):
+        if isinstance(value, str):
+            setattr(instance, self.__name, value.decode('utf-8', 'replace'))
+        elif isinstance(value, unicode):
+            setattr(instance, self.__name, value)
+        else:
+            raise TypeError('{} attribute must be a str or unicode instance, '
+                            'but was {}.'.format(self.__name, type(value)))
+
+    def __delete__(self, instance):
+        raise NotImplementedError
+
+
 class TestResult(object):
     """An object represting the result of a single test."""
-    __slots__ = ['returncode', 'err', 'out', 'time', 'command', 'environment',
-                 'subtests', 'dmesg', '__result', 'images', 'traceback']
+    __slots__ = ['returncode', '_err', '_out', 'time', 'command', 'traceback',
+                 'environment', 'subtests', 'dmesg', '__result', 'images']
+    err = StringDescriptor('_err')
+    out = StringDescriptor('_out')
 
     def __init__(self, result=None):
         self.returncode = None
-        self.err = str()
-        self.out = str()
         self.time = float()
         self.command = str()
         self.environment = str()
@@ -113,10 +141,14 @@ class TestResult(object):
         status.Status object
 
         """
+        # pylint will say that assining to inst.out or inst.err is a non-slot
+        # because self.err and self.out are descriptors, methods that act like
+        # variables. Just silence pylint
+        # pylint: disable=assigning-non-slot
         inst = cls()
 
         # TODO: There's probably a more clever way to do this
-        for each in ['returncode', 'err', 'out', 'time', 'command',
+        for each in ['returncode', 'time', 'command',
                      'environment', 'result', 'dmesg']:
             if each in dict_:
                 setattr(inst, each, dict_[each])
@@ -125,6 +157,13 @@ class TestResult(object):
             for name, value in dict_['subtests'].iteritems():
                 inst.subtests[name] = value
 
+        # out and err must be set manually to avoid replacing the setter
+        if 'out' in dict_:
+            inst.out = dict_['out']
+
+        if 'err' in dict_:
+            inst.err = dict_['err']
+
         return inst
 
     def update(self, dict_):
diff --git a/framework/test/base.py b/framework/test/base.py
index 6be63cfe1..16615a170 100644
--- a/framework/test/base.py
+++ b/framework/test/base.py
@@ -318,22 +318,9 @@ class Test(object):
             else:
                 raise e
 
-        # proc.communicate() returns 8-bit strings, but we need
-        # unicode strings.  In Python 2.x, this is because we
-        # will eventually be serializing the strings as JSON,
-        # and the JSON library expects unicode.  In Python 3.x,
-        # this is because all string operations require
-        # unicode.  So translate the strings into unicode,
-        # assuming they are using UTF-8 encoding.
-        #
-        # If the subprocess output wasn't properly UTF-8
-        # encoded, we don't want to raise an exception, so
-        # translate the strings using 'replace' mode, which
-        # replaces erroneous charcters with the Unicode
-        # "replacement character" (a white question mark inside
-        # a black diamond).
-        self.result.out = out.decode('utf-8', 'replace')
-        self.result.err = err.decode('utf-8', 'replace')
+        # The setter handles the bytes/unicode conversion
+        self.result.out = out
+        self.result.err = err
         self.result.returncode = returncode
 
     def __eq__(self, other):
diff --git a/framework/tests/json_results_update_tests.py b/framework/tests/json_results_update_tests.py
index fd20e100f..3552506b4 100644
--- a/framework/tests/json_results_update_tests.py
+++ b/framework/tests/json_results_update_tests.py
@@ -646,12 +646,12 @@ class TestV4toV5(object):
 class TestV5toV6(object):
     TEST_DATA = {
         'returncode': 0,
-        'err': None,
+        'err': '',
         'environment': None,
         'command': 'foo',
         'result': 'skip',
         'time': 0.123,
-        'out': None,
+        'out': '',
     }
 
     DATA = {
diff --git a/framework/tests/results_tests.py b/framework/tests/results_tests.py
index 3c429f6bb..143ab0908 100644
--- a/framework/tests/results_tests.py
+++ b/framework/tests/results_tests.py
@@ -290,3 +290,59 @@ def test_TestResult_update_subtests():
     test = results.TestResult('pass')
     test.update({'subtest': {'result': 'incomplete'}})
     nt.eq_(test.subtests['result'], 'incomplete')
+
+
+class TestStringDescriptor(object):
+    """Test class for StringDescriptor."""
+    @classmethod
+    def setup_class(cls):
+        class Test(object):  # pylint: disable=too-few-public-methods
+            val = results.StringDescriptor('test')
+
+        cls.class_ = Test
+
+    def setup(self):
+        self.test = self.class_()
+
+    def test_get_default(self):
+        """results.StringDescriptor.__get__: returns default when unset"""
+        nt.eq_(self.test.val, u'')
+
+    def test_set_no_replace(self):
+        """results.StringDescriptor.__set__: instance is not replaced
+
+        This test might not make sense if you don't understand the difference
+        between 'is' and '==' in python. '==' is an equavalency test, while
+        'is' returns true only if the instances are the same.
+
+        What this test does is makes sure that self.test.val is not *replaced*
+        by inst, and instead the value is passed into the __set__ method.
+
+        """
+        inst = 'foo'
+        self.test.val = inst
+        nt.ok_(self.test.val is not inst)
+
+    def test_set_unicode(self):
+        """results.StringDescriptor.__set__: unicode is stored directly"""
+        inst = u'foo'
+        self.test.val = inst
+        nt.eq_(self.test.val, inst)
+
+    def test_set_str(self):
+        """results.StringDescriptor.__set__: converts strs to unicode"""
+        inst = 'foo'
+        self.test.val = inst
+        nt.eq_(self.test.val, unicode(inst))
+
+    @utils.no_error
+    def test_set_str_unicode_literals(self):
+        """results.StringDescriptor.__set__: handles unicode litterals in strs
+        """
+        inst = r'\ufffd'
+        self.test.val = inst
+
+    @nt.raises(NotImplementedError)
+    def test_delete(self):
+        """results.StringDescriptor.__delete__: raises NotImplementedError"""
+        del self.test.val