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

Proposed by Thomi Richards
Status: Merged
Approved by: Christopher Lee
Approved revision: 429
Merged at revision: 433
Proposed branch: lp:~thomir-deactivatedaccount/autopilot/trunk-fix-non-unicode-app-output
Merge into: lp:autopilot
Diff against target: 192 lines (+80/-12)
5 files modified
autopilot/application/_launcher.py (+18/-7)
autopilot/content.py (+6/-2)
autopilot/tests/functional/test_ap_apps.py (+33/-1)
autopilot/tests/unit/test_application_launcher.py (+1/-1)
autopilot/tests/unit/test_debug.py (+22/-1)
To merge this branch: bzr merge lp:~thomir-deactivatedaccount/autopilot/trunk-fix-non-unicode-app-output
Reviewer Review Type Date Requested Status
Christopher Lee (community) Approve
Leo Arias (community) Needs Fixing
Review via email: mp+205522@code.launchpad.net

Commit message

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

Description of the change

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

To post a comment you must log in.
Revision history for this message
Christopher Lee (veebers) wrote :

LGTM

review: Approve
Revision history for this message
Leo Arias (elopio) wrote :

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.')

Revision history for this message
Leo Arias (elopio) :
review: Needs Fixing
Revision history for this message
Christopher Lee (veebers) wrote :

Awesome, LGTM

review: Approve
Revision history for this message
Christopher Lee (veebers) wrote :

LGTM

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
24import logging24import logging
25import os25import os
26import psutil26import psutil
27import six
27import subprocess28import subprocess
28import signal29import signal
29from testtools.content import content_from_file, text_content30from testtools.content import content_from_file, text_content
@@ -141,8 +142,14 @@
141 'process-return-code',142 'process-return-code',
142 text_content(str(return_code))143 text_content(str(return_code))
143 )144 )
144 self.case_addDetail('process-stdout', text_content(stdout))145 self.case_addDetail(
145 self.case_addDetail('process-stderr', text_content(stderr))146 'process-stdout',
147 text_content(stdout)
148 )
149 self.case_addDetail(
150 'process-stderr',
151 text_content(stderr)
152 )
146153
147154
148def launch_process(application, args, capture_output=False, **kwargs):155def launch_process(application, args, capture_output=False, **kwargs):
@@ -335,14 +342,18 @@
335342
336def _kill_process(process):343def _kill_process(process):
337 """Kill the process, and return the stdout, stderr and return code."""344 """Kill the process, and return the stdout, stderr and return code."""
338 stdout = ""345 stdout_parts = []
339 stderr = ""346 stderr_parts = []
340 logger.info("waiting for process to exit.")347 logger.info("waiting for process to exit.")
341 _attempt_kill_pid(process.pid)348 _attempt_kill_pid(process.pid)
342 for _ in Timeout.default():349 for _ in Timeout.default():
343 tmp_out, tmp_err = process.communicate()350 tmp_out, tmp_err = process.communicate()
344 stdout += tmp_out351 if isinstance(tmp_out, six.binary_type):
345 stderr += tmp_err352 tmp_out = tmp_out.decode('utf-8', errors='replace')
353 if isinstance(tmp_err, six.binary_type):
354 tmp_err = tmp_err.decode('utf-8', errors='replace')
355 stdout_parts.append(tmp_out)
356 stderr_parts.append(tmp_err)
346 if not _is_process_running(process.pid):357 if not _is_process_running(process.pid):
347 break358 break
348 else:359 else:
@@ -351,7 +362,7 @@
351 "10 seconds."362 "10 seconds."
352 )363 )
353 _attempt_kill_pid(process.pid, signal.SIGKILL)364 _attempt_kill_pid(process.pid, signal.SIGKILL)
354 return stdout, stderr, process.returncode365 return u''.join(stdout_parts), u''.join(stderr_parts), process.returncode
355366
356367
357def _raise_if_not_empty(kwargs):368def _raise_if_not_empty(kwargs):
358369
=== 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 @@
24import io24import io
2525
26import logging26import logging
27from testtools.content import content_from_stream27from testtools.content import ContentType, content_from_stream
2828
29logger = logging.getLogger(__name__)29logger = logging.getLogger(__name__)
3030
@@ -70,6 +70,10 @@
70 file path will be used instead.70 file path will be used instead.
71 """71 """
72 def make_content():72 def make_content():
73 content_obj = content_from_stream(stream, buffer_now=True)73 content_obj = content_from_stream(
74 stream,
75 ContentType('text', 'plain', {'charset': 'iso8859-1'}),
76 buffer_now=True
77 )
74 test_case.addDetail(content_name, content_obj)78 test_case.addDetail(content_name, content_obj)
75 test_case.addCleanup(make_content)79 test_case.addCleanup(make_content)
7680
=== 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 @@
27import six27import six
28from mock import patch28from mock import patch
29from tempfile import mktemp29from tempfile import mktemp
30from testtools.matchers import raises, LessThan30from testtools.matchers import (
31 LessThan,
32 Not,
33 Raises,
34 raises,
35)
31from textwrap import dedent36from textwrap import dedent
37import unittest
3238
33from autopilot.testcase import AutopilotTestCase39from autopilot.testcase import AutopilotTestCase
34from autopilot.introspection import (40from autopilot.introspection import (
@@ -322,6 +328,32 @@
322 app_proxy = self.launch_test_application(wrapper_path, app_type='qt')328 app_proxy = self.launch_test_application(wrapper_path, app_type='qt')
323 self.assertTrue(app_proxy is not None)329 self.assertTrue(app_proxy is not None)
324330
331 @unittest.expectedFailure
332 def test_can_handle_non_unicode_stdout_and_stderr(self):
333 path = self.write_script(dedent("""\
334 #!%s
335 # -*- coding: utf-8 -*-
336 from PyQt4.QtGui import QMainWindow, QApplication
337 from sys import argv, stdout, stderr
338
339 app = QApplication(argv)
340 win = QMainWindow()
341 win.show()
342 stdout.write('Hello\x88stdout')
343 stdout.flush()
344 stderr.write('Hello\x88stderr')
345 stderr.flush()
346 app.exec_()
347 """ % sys.executable))
348 self.launch_test_application(path, app_type='qt')
349 details_dict = self.getDetails()
350 for name, content_obj in details_dict.items():
351 self.assertThat(
352 lambda: content_obj.as_text(),
353 Not(Raises())
354 )
355 self.assertEqual(1, 2)
356
325357
326class GtkTests(ApplicationTests):358class GtkTests(ApplicationTests):
327359
328360
=== 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 @@
103 app_launcher = NormalApplicationLauncher(mock_addDetail)103 app_launcher = NormalApplicationLauncher(mock_addDetail)
104104
105 with patch.object(105 with patch.object(
106 _l, '_kill_process', return_value=("stdout", "stderr", 0)106 _l, '_kill_process', return_value=(u"stdout", u"stderr", 0)
107 ):107 ):
108 app_launcher._kill_process_and_attach_logs(0)108 app_launcher._kill_process_and_attach_logs(0)
109109
110110
=== 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 @@
23import six23import six
24from tempfile import NamedTemporaryFile24from tempfile import NamedTemporaryFile
25from testtools import TestCase25from testtools import TestCase
26from testtools.matchers import Equals26from testtools.matchers import (
27 Equals,
28 Not,
29 Raises,
30)
2731
2832
29class CaseAddDetailToNormalAddDetailDecoratorTests(TestCase):33class CaseAddDetailToNormalAddDetailDecoratorTests(TestCase):
@@ -191,6 +195,23 @@
191 self.assertThat(args[0], Equals(temp_file.name))195 self.assertThat(args[0], Equals(temp_file.name))
192 self.assertThat(args[1].as_text(), Equals(six.u("World\n")))196 self.assertThat(args[1].as_text(), Equals(six.u("World\n")))
193197
198 def test_can_follow_file_with_binary_content(self):
199 if six.PY2:
200 open_args = dict(bufsize=0)
201 else:
202 open_args = dict(buffering=0)
203 with NamedTemporaryFile(**open_args) as temp_file:
204 log_debug_object = d.LogFileDebugObject(
205 self.fake_caseAddDetail,
206 temp_file.name
207 )
208 log_debug_object.setUp()
209 temp_file.write(six.b("Hello\x88World"))
210 log_debug_object.cleanUp()
211
212 args, _ = self.fake_caseAddDetail.call_args
213 self.assertThat(args[1].as_text, Not(Raises()))
214
194 def test_can_create_syslog_follower(self):215 def test_can_create_syslog_follower(self):
195 debug_obj = d.SyslogDebugObject(Mock())216 debug_obj = d.SyslogDebugObject(Mock())
196 self.assertThat(debug_obj.log_path, Equals("/var/log/syslog"))217 self.assertThat(debug_obj.log_path, Equals("/var/log/syslog"))

Subscribers

People subscribed via source and target branches