Merge lp:~veebers/unity8/update_ap_tests_ready_for_py3 into lp:unity8

Proposed by Christopher Lee on 2013-11-11
Status: Rejected
Rejected by: Albert Astals Cid on 2014-02-26
Proposed branch: lp:~veebers/unity8/update_ap_tests_ready_for_py3
Merge into: lp:unity8
Diff against target: 192 lines (+68/-17)
6 files modified
.bzrignore (+3/-0)
tests/autopilot/tox.ini (+25/-0)
tests/autopilot/unity8/__init__.py (+4/-1)
tests/autopilot/unity8/shell/tests/__init__.py (+12/-13)
tests/autopilot/unity8/shell/tests/test_lock_screen.py (+18/-3)
tests/autopilot/unity8/shell/tests/test_notifications.py (+6/-0)
To merge this branch: bzr merge lp:~veebers/unity8/update_ap_tests_ready_for_py3
Reviewer Review Type Date Requested Status
Albert Astals Cid (community) Disapprove on 2014-02-26
Christopher Lee (community) Needs Fixing on 2013-11-22
PS Jenkins bot (community) continuous-integration Needs Fixing on 2013-11-19
Michał Sawicz 2013-11-11 Needs Fixing on 2013-11-19
Martin Pitt Approve on 2013-11-14
Review via email: mp+194655@code.launchpad.net

Commit message

Update the autopilot tests (and emulators) to make them compatible with python 3.

Description of the change

Update the autopilot tests (and emulators) to make them compatible with python 3.

To post a comment you must log in.
Martin Pitt (pitti) wrote :

61 - data_path = "/usr/share/unity8/mocks/data"
62 + data_path = u"/usr/share/unity8/mocks/data"

Why are these kinds of changes necessary? "foo" is already a string in Python 3, and Python 2 mostly doesn't care (and obviously the tests currently work with intermixing bytes and strings). The u"" form is deprecated in Python 3, and in fact got removed entirely in 3.0 to 3.2 before it was reintroduced in 3.3 for backwards compat support.

145 - if not isinstance(code, basestring):
147 + if not isinstance(code, str):

Is there a chance that you'll get an unicode object in the tests when running with Python 2? I suppose you ran the py2 tests with this branch and it worked, but just making double-sure.

LGTM otherwise, thanks!

review: Needs Information
Christopher Lee (veebers) wrote :

Hi Martin,

I've updated the code addressing the issues highlighted in your review.
(the str/basestring change I took from the autopilot py3 update).

I ran the tests with both py2 && py3 using tox (that is also introduced in this MR).

Martin Pitt (pitti) wrote :

I can't speak about the new tox file (I never used that, and it seems a little redundant to me), but the actual porting looks good now, thank you!

review: Approve
Christopher Lee (veebers) wrote :

The intention with the tox integration is to make it easy for people to run any tests they create or modify and ensure they run under py2 && py3. At least until we are py3 only then it should be removed.

Perhaps there is a better way to do so? I'm open to suggestions.

Martin Pitt (pitti) wrote :

Don't get me wrong, I'm not saying it's bad. Just that I haven't used it so far and thus I don't know it. I usually just call the tests directly with python2/python3 in debian/tests/, debian/rules, "do-release" scripts, or wherever appropriate.

PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:524
http://jenkins.qa.ubuntu.com/job/unity8-ci/1643/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty/734
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty-touch/722/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-trusty/245
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-trusty-amd64-ci/166
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-trusty-armhf-ci/167
        deb: http://jenkins.qa.ubuntu.com/job/unity8-trusty-armhf-ci/167/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-trusty-i386-ci/166
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-trusty/671
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/734
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/734/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/722
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/722/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-mako/3351/console
    SUCCESS: http://s-jenkins:8080/job/touch-flash-device/1440

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity8-ci/1643/rebuild

review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Michał Sawicz (saviq) wrote :

Tox executes:

autopilot run unity8.tests

But that matches 0 tests.

review: Needs Fixing
525. By Christopher Lee on 2013-11-19

Merge Trunk

526. By Christopher Lee on 2013-11-19

Fix typo in args string

PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:526
http://jenkins.qa.ubuntu.com/job/unity8-ci/1675/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty/879
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty-touch/867
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-trusty/308/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-trusty-amd64-ci/198
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-trusty-armhf-ci/199
        deb: http://jenkins.qa.ubuntu.com/job/unity8-trusty-armhf-ci/199/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-trusty-i386-ci/198
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-trusty/790
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/879
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/879/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/867
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/867/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-mako/3468
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/1557

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/1675/rebuild

review: Needs Fixing (continuous-integration)
Christopher Lee (veebers) wrote :

This will need a packaging change too that will install a python3 version as well (otherwise the py3 changes are useless at this stage).

review: Needs Fixing
Martin Pitt (pitti) wrote :

I thought we wanted land this first, as we *will* cause failures during CI test running if we pull in python3-autopilot at this point. Francis wanted some package to experiment with to teach the CI infrastructure about using "autopilot" vs. "autopilot-py3" according to the dependencies, but that's not there yet.

Michał Sawicz (saviq) wrote :

I thought that was the idea, too - that's why we're doing it so that it works with py2 and py3? So that we can easily flip the switch later?

Christopher Lee (veebers) wrote :

Right, I think I need to clarify what I had intended.

I noticed that the changes made here cannot be consumed by other python3 versions of tests when I started checking the compatibility of the ubuntu-keyboard tests that use the unity8.process_helpers.
This is why I wanted to change the packaging so that the tests, and more importantly the emulators, were installed for python 2 and python 3 (so I could proceed with the u-kb stuff).

This comment made on the ui-toolkit is topical: https://code.launchpad.net/~chris.gagnon/ubuntu-ui-toolkit/autopilot-python3-package/+merge/196358/comments/454927
(effectively either split into 2 packages or insanity of dual-installation).

I get the feeling that at this stage that I should alter what I have so that it is split into 2 packages.

Michał Sawicz (saviq) wrote :

Hey Chris, any word on this?

Albert Astals Cid (aacid) wrote :

Rejecting based on Dimitri's comment

review: Disapprove

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file '.bzrignore'
2--- .bzrignore 2013-06-28 20:27:17 +0000
3+++ .bzrignore 2013-11-19 19:38:26 +0000
4@@ -8,3 +8,6 @@
5 /.settings
6 /.pydevproject
7 message.txt
8+# tox/python/autopilot
9+tests/autopilot/unity.egg-info
10+tests/autopilot/.tox
11
12=== added file 'tests/autopilot/tox.ini'
13--- tests/autopilot/tox.ini 1970-01-01 00:00:00 +0000
14+++ tests/autopilot/tox.ini 2013-11-19 19:38:26 +0000
15@@ -0,0 +1,25 @@
16+# Tox (http://tox.testrun.org/) is a tool for running tests
17+# in multiple virtualenvs. This configuration file will run the
18+# test suite on all supported python versions.
19+#
20+# To use it, install the `tox` python package:
21+# "sudo pip install tox" or "sudo apt-get install python-tox"
22+# then run "tox" from this directory.
23+
24+
25+[tox]
26+envlist = flake8, py27, py33
27+
28+[flake8]
29+exclude = .tox
30+
31+[testenv]
32+sitepackages = True
33+commands =
34+ autopilot run {posargs:unity8}
35+
36+[testenv:flake8]
37+deps =
38+ flake8
39+commands =
40+ flake8
41
42=== modified file 'tests/autopilot/unity8/__init__.py'
43--- tests/autopilot/unity8/__init__.py 2013-10-17 09:01:20 +0000
44+++ tests/autopilot/unity8/__init__.py 2013-11-19 19:38:26 +0000
45@@ -82,7 +82,10 @@
46 )
47 if not os.path.exists(binary_path):
48 try:
49- binary_path = subprocess.check_output(['which', binary]).strip()
50+ binary_path = subprocess.check_output(
51+ ['which', binary],
52+ universal_newlines=True,
53+ ).strip()
54 except subprocess.CalledProcessError as e:
55 raise RuntimeError("Unable to locate %s binary: %r" % (binary, e))
56 return binary_path
57
58=== modified file 'tests/autopilot/unity8/shell/tests/__init__.py'
59--- tests/autopilot/unity8/shell/tests/__init__.py 2013-11-01 13:59:50 +0000
60+++ tests/autopilot/unity8/shell/tests/__init__.py 2013-11-19 19:38:26 +0000
61@@ -86,12 +86,12 @@
62 @classmethod
63 def setUpClass(cls):
64 try:
65- output = subprocess.check_output([
66- "/sbin/initctl",
67- "status",
68- "unity8"
69- ], stderr=subprocess.STDOUT)
70- except subprocess.CalledProcessError, e:
71+ output = subprocess.check_output(
72+ ["/sbin/initctl", "status", "unity8"],
73+ stderr=subprocess.STDOUT,
74+ universal_newlines=True,
75+ )
76+ except subprocess.CalledProcessError as e:
77 sys.stderr.write(
78 "Error: `initctl status unity8` failed, most probably the "
79 "unity8 session could not be found:\n\n"
80@@ -219,12 +219,11 @@
81 def _patch_environment(self, key, value):
82 """Wrapper for patching env for upstart environment."""
83 try:
84- current_value = subprocess.check_output([
85- "/sbin/initctl",
86- "get-env",
87- "--global",
88- key
89- ], stderr=subprocess.STDOUT).rstrip()
90+ current_value = subprocess.check_output(
91+ ["/sbin/initctl", "get-env", "--global", key],
92+ stderr=subprocess.STDOUT,
93+ universal_newlines=True,
94+ ).rstrip()
95 except subprocess.CalledProcessError:
96 current_value = None
97
98@@ -311,7 +310,7 @@
99 "unity8",
100 binary_arg,
101 extra_args,
102- ] + ["%s=%s" % (k, v) for k, v in self._environment.iteritems()],
103+ ] + ["%s=%s" % (k, v) for k, v in self._environment.items()],
104 stderr=subprocess.STDOUT,
105 )
106
107
108=== modified file 'tests/autopilot/unity8/shell/tests/test_lock_screen.py'
109--- tests/autopilot/unity8/shell/tests/test_lock_screen.py 2013-08-23 07:38:56 +0000
110+++ tests/autopilot/unity8/shell/tests/test_lock_screen.py 2013-11-19 19:38:26 +0000
111@@ -25,12 +25,18 @@
112
113 from autopilot.matchers import Eventually
114 from autopilot.platform import model
115+import sys
116 from testtools import skipUnless
117 from testtools.matchers import Equals
118 import logging
119
120 logger = logging.getLogger(__name__)
121
122+# py2 compatible alias for py3
123+if sys.version >= '3':
124+ basestring = str
125+
126+
127 class TestLockscreen(UnityTestCase):
128
129 """Tests for the lock screen."""
130@@ -124,7 +130,10 @@
131 """
132
133 if not isinstance(code, basestring):
134- raise TypeError("'code' parameter must be a string.")
135+ raise TypeError(
136+ "'code' parameter must be a string, not %r."
137+ % type(passphrase)
138+ )
139 for num in code:
140 if not num.isdigit():
141 raise ValueError(
142@@ -141,7 +150,10 @@
143
144 """
145 if not isinstance(passphrase, basestring):
146- raise TypeError("'passphrase' parameter must be a string.")
147+ raise TypeError(
148+ "'passphrase' parameter must be a string, not %r."
149+ % type(passphrase)
150+ )
151
152 pinentryField = self.main_window.get_pinentryField()
153 self.touch.tap_object(pinentryField)
154@@ -160,7 +172,10 @@
155
156 """
157 if not isinstance(passphrase, basestring):
158- raise TypeError("'passphrase' parameter must be a string.")
159+ raise TypeError(
160+ "'passphrase' parameter must be a string, not %r."
161+ % type(passphrase)
162+ )
163
164 prompt = self.main_window.get_greeter().get_prompt()
165 self.touch.tap_object(prompt)
166
167=== modified file 'tests/autopilot/unity8/shell/tests/test_notifications.py'
168--- tests/autopilot/unity8/shell/tests/test_notifications.py 2013-11-01 13:59:50 +0000
169+++ tests/autopilot/unity8/shell/tests/test_notifications.py 2013-11-19 19:38:26 +0000
170@@ -32,9 +32,14 @@
171 import logging
172 import signal
173 import subprocess
174+import sys
175
176 logger = logging.getLogger(__name__)
177
178+# backwards compatible alias for Python 3
179+if sys.version > '3':
180+ xrange = range
181+
182
183 class NotificationsBase(UnityTestCase):
184 """Base class for all notification tests that provides helper methods."""
185@@ -266,6 +271,7 @@
186 stdout=subprocess.PIPE,
187 stderr=subprocess.PIPE,
188 close_fds=True,
189+ universal_newlines=True,
190 )
191
192 self.addCleanup(self._tidy_up_script_process)

Subscribers

People subscribed via source and target branches