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.
Christopher Lee (veebers) wrote :

LGTM

review: Approve
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.')

Leo Arias (elopio) :
review: Needs Fixing
Christopher Lee (veebers) wrote :

Awesome, LGTM

review: Approve
Christopher Lee (veebers) wrote :

LGTM

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'autopilot/application/_launcher.py'
2--- autopilot/application/_launcher.py 2014-01-29 21:30:34 +0000
3+++ autopilot/application/_launcher.py 2014-02-11 23:29:33 +0000
4@@ -24,6 +24,7 @@
5 import logging
6 import os
7 import psutil
8+import six
9 import subprocess
10 import signal
11 from testtools.content import content_from_file, text_content
12@@ -141,8 +142,14 @@
13 'process-return-code',
14 text_content(str(return_code))
15 )
16- self.case_addDetail('process-stdout', text_content(stdout))
17- self.case_addDetail('process-stderr', text_content(stderr))
18+ self.case_addDetail(
19+ 'process-stdout',
20+ text_content(stdout)
21+ )
22+ self.case_addDetail(
23+ 'process-stderr',
24+ text_content(stderr)
25+ )
26
27
28 def launch_process(application, args, capture_output=False, **kwargs):
29@@ -335,14 +342,18 @@
30
31 def _kill_process(process):
32 """Kill the process, and return the stdout, stderr and return code."""
33- stdout = ""
34- stderr = ""
35+ stdout_parts = []
36+ stderr_parts = []
37 logger.info("waiting for process to exit.")
38 _attempt_kill_pid(process.pid)
39 for _ in Timeout.default():
40 tmp_out, tmp_err = process.communicate()
41- stdout += tmp_out
42- stderr += tmp_err
43+ if isinstance(tmp_out, six.binary_type):
44+ tmp_out = tmp_out.decode('utf-8', errors='replace')
45+ if isinstance(tmp_err, six.binary_type):
46+ tmp_err = tmp_err.decode('utf-8', errors='replace')
47+ stdout_parts.append(tmp_out)
48+ stderr_parts.append(tmp_err)
49 if not _is_process_running(process.pid):
50 break
51 else:
52@@ -351,7 +362,7 @@
53 "10 seconds."
54 )
55 _attempt_kill_pid(process.pid, signal.SIGKILL)
56- return stdout, stderr, process.returncode
57+ return u''.join(stdout_parts), u''.join(stderr_parts), process.returncode
58
59
60 def _raise_if_not_empty(kwargs):
61
62=== modified file 'autopilot/content.py'
63--- autopilot/content.py 2014-01-13 01:59:28 +0000
64+++ autopilot/content.py 2014-02-11 23:29:33 +0000
65@@ -24,7 +24,7 @@
66 import io
67
68 import logging
69-from testtools.content import content_from_stream
70+from testtools.content import ContentType, content_from_stream
71
72 logger = logging.getLogger(__name__)
73
74@@ -70,6 +70,10 @@
75 file path will be used instead.
76 """
77 def make_content():
78- content_obj = content_from_stream(stream, buffer_now=True)
79+ content_obj = content_from_stream(
80+ stream,
81+ ContentType('text', 'plain', {'charset': 'iso8859-1'}),
82+ buffer_now=True
83+ )
84 test_case.addDetail(content_name, content_obj)
85 test_case.addCleanup(make_content)
86
87=== modified file 'autopilot/tests/functional/test_ap_apps.py'
88--- autopilot/tests/functional/test_ap_apps.py 2014-01-20 19:50:53 +0000
89+++ autopilot/tests/functional/test_ap_apps.py 2014-02-11 23:29:33 +0000
90@@ -27,8 +27,14 @@
91 import six
92 from mock import patch
93 from tempfile import mktemp
94-from testtools.matchers import raises, LessThan
95+from testtools.matchers import (
96+ LessThan,
97+ Not,
98+ Raises,
99+ raises,
100+)
101 from textwrap import dedent
102+import unittest
103
104 from autopilot.testcase import AutopilotTestCase
105 from autopilot.introspection import (
106@@ -322,6 +328,32 @@
107 app_proxy = self.launch_test_application(wrapper_path, app_type='qt')
108 self.assertTrue(app_proxy is not None)
109
110+ @unittest.expectedFailure
111+ def test_can_handle_non_unicode_stdout_and_stderr(self):
112+ path = self.write_script(dedent("""\
113+ #!%s
114+ # -*- coding: utf-8 -*-
115+ from PyQt4.QtGui import QMainWindow, QApplication
116+ from sys import argv, stdout, stderr
117+
118+ app = QApplication(argv)
119+ win = QMainWindow()
120+ win.show()
121+ stdout.write('Hello\x88stdout')
122+ stdout.flush()
123+ stderr.write('Hello\x88stderr')
124+ stderr.flush()
125+ app.exec_()
126+ """ % sys.executable))
127+ self.launch_test_application(path, app_type='qt')
128+ details_dict = self.getDetails()
129+ for name, content_obj in details_dict.items():
130+ self.assertThat(
131+ lambda: content_obj.as_text(),
132+ Not(Raises())
133+ )
134+ self.assertEqual(1, 2)
135+
136
137 class GtkTests(ApplicationTests):
138
139
140=== modified file 'autopilot/tests/unit/test_application_launcher.py'
141--- autopilot/tests/unit/test_application_launcher.py 2014-01-29 01:22:56 +0000
142+++ autopilot/tests/unit/test_application_launcher.py 2014-02-11 23:29:33 +0000
143@@ -103,7 +103,7 @@
144 app_launcher = NormalApplicationLauncher(mock_addDetail)
145
146 with patch.object(
147- _l, '_kill_process', return_value=("stdout", "stderr", 0)
148+ _l, '_kill_process', return_value=(u"stdout", u"stderr", 0)
149 ):
150 app_launcher._kill_process_and_attach_logs(0)
151
152
153=== modified file 'autopilot/tests/unit/test_debug.py'
154--- autopilot/tests/unit/test_debug.py 2014-01-10 00:58:41 +0000
155+++ autopilot/tests/unit/test_debug.py 2014-02-11 23:29:33 +0000
156@@ -23,7 +23,11 @@
157 import six
158 from tempfile import NamedTemporaryFile
159 from testtools import TestCase
160-from testtools.matchers import Equals
161+from testtools.matchers import (
162+ Equals,
163+ Not,
164+ Raises,
165+)
166
167
168 class CaseAddDetailToNormalAddDetailDecoratorTests(TestCase):
169@@ -191,6 +195,23 @@
170 self.assertThat(args[0], Equals(temp_file.name))
171 self.assertThat(args[1].as_text(), Equals(six.u("World\n")))
172
173+ def test_can_follow_file_with_binary_content(self):
174+ if six.PY2:
175+ open_args = dict(bufsize=0)
176+ else:
177+ open_args = dict(buffering=0)
178+ with NamedTemporaryFile(**open_args) as temp_file:
179+ log_debug_object = d.LogFileDebugObject(
180+ self.fake_caseAddDetail,
181+ temp_file.name
182+ )
183+ log_debug_object.setUp()
184+ temp_file.write(six.b("Hello\x88World"))
185+ log_debug_object.cleanUp()
186+
187+ args, _ = self.fake_caseAddDetail.call_args
188+ self.assertThat(args[1].as_text, Not(Raises()))
189+
190 def test_can_create_syslog_follower(self):
191 debug_obj = d.SyslogDebugObject(Mock())
192 self.assertThat(debug_obj.log_path, Equals("/var/log/syslog"))

Subscribers

People subscribed via source and target branches