Merge lp:~thomir-deactivatedaccount/autopilot/trunk-fix-non-unicode-app-output into lp:autopilot

[Thomi Richards]

Fix a bug where autopilot didn't do the right thing with non-unicode valid bytestrings.

Fix a bug where autopilot didn't do the right thing with non-unicode valid bytestrings.

[Christopher Lee (veebers)]

LGTM

review: Approve

[Leo Arias (elopio)]

Am I too late?

72 + self.assertEqual(1, 2)

This is not clear. Why not just call self.fail('Fail so we can check that the details are properly collected.')

[Leo Arias (elopio)]

review: Needs Fixing

[Christopher Lee (veebers)]

Awesome, LGTM

review: Approve

[Christopher Lee (veebers)]

LGTM

review: Approve

=== modified file 'autopilot/application/_launcher.py'
--- autopilot/application/_launcher.py	2014-01-29 21:30:34 +0000
+++ autopilot/application/_launcher.py	2014-02-11 23:29:33 +0000
@@ -24,6 +24,7 @@
 import logging
 import os
 import psutil
+import six
 import subprocess
 import signal
 from testtools.content import content_from_file, text_content
@@ -141,8 +142,14 @@
             'process-return-code',
             text_content(str(return_code))
         )
-        self.case_addDetail('process-stdout', text_content(stdout))
-        self.case_addDetail('process-stderr', text_content(stderr))
+        self.case_addDetail(
+            'process-stdout',
+            text_content(stdout)
+        )
+        self.case_addDetail(
+            'process-stderr',
+            text_content(stderr)
+        )
 
 
 def launch_process(application, args, capture_output=False, **kwargs):
@@ -335,14 +342,18 @@
 
 def _kill_process(process):
     """Kill the process, and return the stdout, stderr and return code."""
-    stdout = ""
-    stderr = ""
+    stdout_parts = []
+    stderr_parts = []
     logger.info("waiting for process to exit.")
     _attempt_kill_pid(process.pid)
     for _ in Timeout.default():
         tmp_out, tmp_err = process.communicate()
-        stdout += tmp_out
-        stderr += tmp_err
+        if isinstance(tmp_out, six.binary_type):
+            tmp_out = tmp_out.decode('utf-8', errors='replace')
+        if isinstance(tmp_err, six.binary_type):
+            tmp_err = tmp_err.decode('utf-8', errors='replace')
+        stdout_parts.append(tmp_out)
+        stderr_parts.append(tmp_err)
         if not _is_process_running(process.pid):
             break
     else:
@@ -351,7 +362,7 @@
             "10 seconds."
         )
         _attempt_kill_pid(process.pid, signal.SIGKILL)
-    return stdout, stderr, process.returncode
+    return u''.join(stdout_parts), u''.join(stderr_parts), process.returncode
 
 
 def _raise_if_not_empty(kwargs):

=== modified file 'autopilot/content.py'
--- autopilot/content.py	2014-01-13 01:59:28 +0000
+++ autopilot/content.py	2014-02-11 23:29:33 +0000
@@ -24,7 +24,7 @@
 import io
 
 import logging
-from testtools.content import content_from_stream
+from testtools.content import ContentType, content_from_stream
 
 logger = logging.getLogger(__name__)
 
@@ -70,6 +70,10 @@
         file path will be used instead.
     """
     def make_content():
-        content_obj = content_from_stream(stream, buffer_now=True)
+        content_obj = content_from_stream(
+            stream,
+            ContentType('text', 'plain', {'charset': 'iso8859-1'}),
+            buffer_now=True
+        )
         test_case.addDetail(content_name, content_obj)
     test_case.addCleanup(make_content)

=== modified file 'autopilot/tests/functional/test_ap_apps.py'
--- autopilot/tests/functional/test_ap_apps.py	2014-01-20 19:50:53 +0000
+++ autopilot/tests/functional/test_ap_apps.py	2014-02-11 23:29:33 +0000
@@ -27,8 +27,14 @@
 import six
 from mock import patch
 from tempfile import mktemp
-from testtools.matchers import raises, LessThan
+from testtools.matchers import (
+    LessThan,
+    Not,
+    Raises,
+    raises,
+)
 from textwrap import dedent
+import unittest
 
 from autopilot.testcase import AutopilotTestCase
 from autopilot.introspection import (
@@ -322,6 +328,32 @@
         app_proxy = self.launch_test_application(wrapper_path, app_type='qt')
         self.assertTrue(app_proxy is not None)
 
+    @unittest.expectedFailure
+    def test_can_handle_non_unicode_stdout_and_stderr(self):
+        path = self.write_script(dedent("""\
+            #!%s
+            # -*- coding: utf-8 -*-
+            from PyQt4.QtGui import QMainWindow, QApplication
+            from sys import argv, stdout, stderr
+
+            app = QApplication(argv)
+            win = QMainWindow()
+            win.show()
+            stdout.write('Hello\x88stdout')
+            stdout.flush()
+            stderr.write('Hello\x88stderr')
+            stderr.flush()
+            app.exec_()
+            """ % sys.executable))
+        self.launch_test_application(path, app_type='qt')
+        details_dict = self.getDetails()
+        for name, content_obj in details_dict.items():
+            self.assertThat(
+                lambda: content_obj.as_text(),
+                Not(Raises())
+            )
+        self.assertEqual(1, 2)
+
 
 class GtkTests(ApplicationTests):
 

=== modified file 'autopilot/tests/unit/test_application_launcher.py'
--- autopilot/tests/unit/test_application_launcher.py	2014-01-29 01:22:56 +0000
+++ autopilot/tests/unit/test_application_launcher.py	2014-02-11 23:29:33 +0000
@@ -103,7 +103,7 @@
         app_launcher = NormalApplicationLauncher(mock_addDetail)
 
         with patch.object(
-            _l, '_kill_process', return_value=("stdout", "stderr", 0)
+            _l, '_kill_process', return_value=(u"stdout", u"stderr", 0)
         ):
             app_launcher._kill_process_and_attach_logs(0)
 

=== modified file 'autopilot/tests/unit/test_debug.py'
--- autopilot/tests/unit/test_debug.py	2014-01-10 00:58:41 +0000
+++ autopilot/tests/unit/test_debug.py	2014-02-11 23:29:33 +0000
@@ -23,7 +23,11 @@
 import six
 from tempfile import NamedTemporaryFile
 from testtools import TestCase
-from testtools.matchers import Equals
+from testtools.matchers import (
+    Equals,
+    Not,
+    Raises,
+)
 
 
 class CaseAddDetailToNormalAddDetailDecoratorTests(TestCase):
@@ -191,6 +195,23 @@
             self.assertThat(args[0], Equals(temp_file.name))
             self.assertThat(args[1].as_text(), Equals(six.u("World\n")))
 
+    def test_can_follow_file_with_binary_content(self):
+        if six.PY2:
+            open_args = dict(bufsize=0)
+        else:
+            open_args = dict(buffering=0)
+        with NamedTemporaryFile(**open_args) as temp_file:
+            log_debug_object = d.LogFileDebugObject(
+                self.fake_caseAddDetail,
+                temp_file.name
+            )
+            log_debug_object.setUp()
+            temp_file.write(six.b("Hello\x88World"))
+            log_debug_object.cleanUp()
+
+        args, _ = self.fake_caseAddDetail.call_args
+        self.assertThat(args[1].as_text, Not(Raises()))
+
     def test_can_create_syslog_follower(self):
         debug_obj = d.SyslogDebugObject(Mock())
         self.assertThat(debug_obj.log_path, Equals("/var/log/syslog"))