Merge lp:~cjwatson/click/fix-tests-installation into lp:click/devel
- fix-tests-installation
- Merge into devel
Status: | Merged |
---|---|
Approved by: | Michael Vogt |
Approved revision: | 500 |
Merged at revision: | 509 |
Proposed branch: | lp:~cjwatson/click/fix-tests-installation |
Merge into: | lp:click/devel |
Diff against target: |
422 lines (+95/-56) 11 files modified
click/tests/integration/helpers.py (+13/-7) click/tests/integration/test_build_core_apps.py (+11/-6) click/tests/integration/test_chroot.py (+4/-5) click/tests/integration/test_frameworks.py (+10/-11) click/tests/integration/test_hook.py (+7/-3) click/tests/integration/test_install.py (+5/-5) click/tests/integration/test_signatures.py (+32/-16) debian/changelog (+10/-0) debian/control (+1/-1) debian/tests/control (+1/-1) debian/tests/run-tests.sh (+1/-1) |
To merge this branch: | bzr merge lp:~cjwatson/click/fix-tests-installation |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Michael Vogt | Approve | ||
PS Jenkins bot (community) | continuous-integration | Approve | |
Review via email: mp+226077@code.launchpad.net |
This proposal supersedes a proposal from 2014-07-09.
Commit message
Move integration tests to a location where they won't end up being installed into inappropriate places on the system module path (LP: #1337696).
Description of the change
The integration tests are being installed to /usr/lib/
This made the integration tests visible to pyflakes/pep8, which necessitated some follow-up tweaks.
PS Jenkins bot (ps-jenkins) wrote : | # |
- 494. By Colin Watson
-
Hack around unittest.skipIf decorator ordering, which caused some bits of the integration test framework to try to run without sufficient dependencies installed.
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:493
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild:
http://
- 495. By Colin Watson
-
Don't fail if ping does not exist (as when running the unit tests in an environment without sufficient dependencies to run the integration tests).
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:493
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:495
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Michael Vogt (mvo) wrote : | # |
Thanks for this branch, it looks good, great that they are also covered by pep8/pyflakes now!
Some (small) inline comment below.
As a more general question: if the goal was to simply not install them, we could have changed setup.py to something like:
- packages=
+ packages=
But I assume moving the tests into the click/tests/
Thanks,
Michael
- 496. By Colin Watson
-
merge trunk
- 497. By Colin Watson
-
More PEP-8 failures.
- 498. By Colin Watson
-
Call skipTest in ClickTestCase.setUp rather than using allow_integration helper everywhere.
- 499. By Colin Watson
-
Use is_root helper in another place.
- 500. By Colin Watson
-
Rearrange integration test skip logic to do everything in setUpClass rather than via decorators, for clarity of ordering.
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:500
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Colin Watson (cjwatson) wrote : | # |
Thanks; I've addressed your comments.
I indeed felt it better to have the tests live under click/tests/. It's true that we could probably now consolidate some duplicated code, but I think that probably ought to be done in a separate branch.
Michael Vogt (mvo) wrote : | # |
Thanks, this looks great.
Preview Diff
1 | === renamed directory 'tests/integration' => 'click/tests/integration' |
2 | === modified file 'click/tests/integration/helpers.py' |
3 | --- tests/integration/helpers.py 2014-08-03 13:10:48 +0000 |
4 | +++ click/tests/integration/helpers.py 2014-09-04 21:07:10 +0000 |
5 | @@ -27,13 +27,17 @@ |
6 | import unittest |
7 | |
8 | |
9 | -def is_root(): |
10 | - return os.getuid() == 0 |
11 | - |
12 | - |
13 | -def has_network(): |
14 | - return subprocess.call( |
15 | - ["ping", "-c1", "archive.ubuntu.com"]) == 0 |
16 | +def require_root(): |
17 | + if os.getuid() != 0: |
18 | + raise unittest.SkipTest("This test needs to run as root") |
19 | + |
20 | + |
21 | +def require_network(): |
22 | + try: |
23 | + if subprocess.call(["ping", "-c1", "archive.ubuntu.com"]) != 0: |
24 | + raise unittest.SkipTest("Need network") |
25 | + except Exception: |
26 | + pass |
27 | |
28 | |
29 | @contextlib.contextmanager |
30 | @@ -50,6 +54,8 @@ |
31 | |
32 | @classmethod |
33 | def setUpClass(cls): |
34 | + if "TEST_INTEGRATION" not in os.environ: |
35 | + raise unittest.SkipTest("Skipping integration tests") |
36 | cls.click_binary = os.environ.get("CLICK_BINARY", "/usr/bin/click") |
37 | |
38 | def setUp(self): |
39 | |
40 | === modified file 'click/tests/integration/test_build_core_apps.py' |
41 | --- tests/integration/test_build_core_apps.py 2014-08-03 13:10:48 +0000 |
42 | +++ click/tests/integration/test_build_core_apps.py 2014-09-04 21:07:10 +0000 |
43 | @@ -20,12 +20,13 @@ |
44 | import os |
45 | import shutil |
46 | import subprocess |
47 | -import unittest |
48 | + |
49 | +from six import with_metaclass |
50 | |
51 | from .helpers import ( |
52 | chdir, |
53 | - has_network, |
54 | - is_root, |
55 | + require_network, |
56 | + require_root, |
57 | ClickTestCase, |
58 | ) |
59 | |
60 | @@ -63,9 +64,13 @@ |
61 | return type.__new__(cls, name, bases, dct) |
62 | |
63 | |
64 | -@unittest.skipIf(not is_root(), "This tests needs to run as root") |
65 | -@unittest.skipIf(not has_network(), "Need network") |
66 | -class TestBuildCoreApps(ClickTestCase, metaclass=AddBranchTestFunctions): |
67 | +class TestBuildCoreApps(with_metaclass(AddBranchTestFunctions, ClickTestCase)): |
68 | + |
69 | + @classmethod |
70 | + def setUpClass(cls): |
71 | + super(TestBuildCoreApps, cls).setUpClass() |
72 | + require_root() |
73 | + require_network() |
74 | |
75 | def _run_in_chroot(self, cmd): |
76 | """Run the given cmd in a click chroot""" |
77 | |
78 | === modified file 'click/tests/integration/test_chroot.py' |
79 | --- tests/integration/test_chroot.py 2014-08-03 13:10:48 +0000 |
80 | +++ click/tests/integration/test_chroot.py 2014-09-04 21:07:10 +0000 |
81 | @@ -16,22 +16,21 @@ |
82 | """Integration tests for the click chroot feature.""" |
83 | |
84 | import subprocess |
85 | -import unittest |
86 | |
87 | from .helpers import ( |
88 | - has_network, |
89 | - is_root, |
90 | + require_network, |
91 | + require_root, |
92 | ClickTestCase, |
93 | ) |
94 | |
95 | |
96 | -@unittest.skipIf(not is_root(), "This tests needs to run as root") |
97 | -@unittest.skipIf(not has_network(), "Need network") |
98 | class TestChroot(ClickTestCase): |
99 | |
100 | @classmethod |
101 | def setUpClass(cls): |
102 | super(TestChroot, cls).setUpClass() |
103 | + require_root() |
104 | + require_network() |
105 | cls.arch = subprocess.check_output( |
106 | ["dpkg", "--print-architecture"], universal_newlines=True).strip() |
107 | subprocess.check_call([ |
108 | |
109 | === modified file 'click/tests/integration/test_frameworks.py' |
110 | --- tests/integration/test_frameworks.py 2014-06-26 12:00:09 +0000 |
111 | +++ click/tests/integration/test_frameworks.py 2014-09-04 21:07:10 +0000 |
112 | @@ -17,18 +17,17 @@ |
113 | |
114 | import os |
115 | import subprocess |
116 | -import unittest |
117 | - |
118 | -from .helpers import ( |
119 | - ClickTestCase, |
120 | -) |
121 | - |
122 | - |
123 | -@unittest.skipIf( |
124 | - (not os.path.exists("/usr/share/click/frameworks") or |
125 | - not os.listdir("/usr/share/click/frameworks")), |
126 | - "Please install ubuntu-sdk-libs") |
127 | + |
128 | +from .helpers import ClickTestCase |
129 | + |
130 | + |
131 | class TestFrameworks(ClickTestCase): |
132 | + def setUp(self): |
133 | + super(TestFrameworks, self).setUp() |
134 | + if (not os.path.exists("/usr/share/click/frameworks") or |
135 | + not os.listdir("/usr/share/click/frameworks")): |
136 | + self.skipTest("Please install ubuntu-sdk-libs") |
137 | + |
138 | def test_framework_list(self): |
139 | output = subprocess.check_output([ |
140 | self.click_binary, "framework", "list"], universal_newlines=True) |
141 | |
142 | === modified file 'click/tests/integration/test_hook.py' |
143 | --- tests/integration/test_hook.py 2014-08-22 17:01:08 +0000 |
144 | +++ click/tests/integration/test_hook.py 2014-09-04 21:07:10 +0000 |
145 | @@ -18,16 +18,20 @@ |
146 | import os |
147 | import subprocess |
148 | from textwrap import dedent |
149 | -import unittest |
150 | |
151 | from .helpers import ( |
152 | - is_root, |
153 | ClickTestCase, |
154 | + require_root, |
155 | ) |
156 | |
157 | |
158 | -@unittest.skipIf(not is_root(), "This tests needs to run as root") |
159 | class TestHook(ClickTestCase): |
160 | + |
161 | + @classmethod |
162 | + def setUpClass(cls): |
163 | + super(TestHook, cls).setUpClass() |
164 | + require_root() |
165 | + |
166 | def _make_hook(self, name): |
167 | hook_fname = "/usr/share/click/hooks/%s.hook" % name |
168 | canary_fname = os.path.join(self.temp_dir, "canary.sh") |
169 | |
170 | === modified file 'click/tests/integration/test_install.py' |
171 | --- tests/integration/test_install.py 2014-08-22 17:01:08 +0000 |
172 | +++ click/tests/integration/test_install.py 2014-09-04 21:07:10 +0000 |
173 | @@ -15,11 +15,12 @@ |
174 | |
175 | """Integration tests for the click install feature.""" |
176 | |
177 | -import os |
178 | import subprocess |
179 | -import unittest |
180 | |
181 | -from .helpers import ClickTestCase |
182 | +from .helpers import ( |
183 | + require_root, |
184 | + ClickTestCase, |
185 | +) |
186 | |
187 | |
188 | def add_user(name): |
189 | @@ -31,13 +32,12 @@ |
190 | subprocess.check_call(["userdel", "-r", name]) |
191 | |
192 | |
193 | -@unittest.skipIf( |
194 | - os.getuid() != 0, "This tests needs to run as root") |
195 | class TestClickInstall(ClickTestCase): |
196 | |
197 | @classmethod |
198 | def setUpClass(cls): |
199 | super(TestClickInstall, cls).setUpClass() |
200 | + require_root() |
201 | cls.USER_1 = add_user("click-test-user-1") |
202 | cls.USER_2 = add_user("click-test-user-2") |
203 | |
204 | |
205 | === modified file 'click/tests/integration/test_signatures.py' |
206 | --- tests/integration/test_signatures.py 2014-08-22 17:01:08 +0000 |
207 | +++ click/tests/integration/test_signatures.py 2014-09-04 21:07:10 +0000 |
208 | @@ -20,20 +20,21 @@ |
209 | import shutil |
210 | import subprocess |
211 | import tarfile |
212 | -import unittest |
213 | from textwrap import dedent |
214 | |
215 | from .helpers import ( |
216 | - is_root, |
217 | + require_root, |
218 | ClickTestCase, |
219 | ) |
220 | |
221 | + |
222 | def makedirs(path): |
223 | try: |
224 | os.makedirs(path) |
225 | except OSError: |
226 | pass |
227 | |
228 | + |
229 | def get_keyid_from_gpghome(gpg_home): |
230 | """Return the public keyid of a given gpg home dir""" |
231 | output = subprocess.check_output( |
232 | @@ -56,7 +57,7 @@ |
233 | def sign(self, filepath, signature_type="origin"): |
234 | """Sign the click at filepath""" |
235 | env = copy.copy(os.environ) |
236 | - env["GNUPGHOME"] = os.path.abspath(self.gpghome) |
237 | + env["GNUPGHOME"] = os.path.abspath(self.gpghome) |
238 | subprocess.check_call( |
239 | ["debsigs", |
240 | "--sign=%s" % signature_type, |
241 | @@ -74,7 +75,7 @@ |
242 | <Selection> |
243 | <Required Type="origin" File="{filename}" id="{keyid}"/> |
244 | </Selection> |
245 | - |
246 | + |
247 | <Verification> |
248 | <Required Type="origin" File="{filename}" id="{keyid}"/> |
249 | </Verification> |
250 | @@ -86,7 +87,8 @@ |
251 | self.pubkey_path = ( |
252 | "/usr/share/debsig/keyrings/%s/origin.pub" % self.keyid) |
253 | makedirs(os.path.dirname(self.pubkey_path)) |
254 | - shutil.copy(os.path.join(self.gpghome, "pubring.gpg"), self.pubkey_path) |
255 | + shutil.copy( |
256 | + os.path.join(self.gpghome, "pubring.gpg"), self.pubkey_path) |
257 | |
258 | def uninstall_signature_policy(self): |
259 | # FIXME: update debsig-verify so that it can work from a different |
260 | @@ -96,8 +98,13 @@ |
261 | os.remove(self.pubkey_path) |
262 | |
263 | |
264 | -@unittest.skipIf(not is_root(), "This tests needs to run as root") |
265 | class ClickSignaturesTestCase(ClickTestCase): |
266 | + |
267 | + @classmethod |
268 | + def setUpClass(cls): |
269 | + super(ClickSignaturesTestCase, cls).setUpClass() |
270 | + require_root() |
271 | + |
272 | def assertClickNoSignatureError(self, cmd_args): |
273 | with self.assertRaises(subprocess.CalledProcessError) as cm: |
274 | output = subprocess.check_output( |
275 | @@ -120,8 +127,13 @@ |
276 | self.assertIn(expected_error_message, output) |
277 | |
278 | |
279 | -@unittest.skipIf(not is_root(), "This tests needs to run as root") |
280 | class TestSignatureVerificationNoSignature(ClickSignaturesTestCase): |
281 | + |
282 | + @classmethod |
283 | + def setUpClass(cls): |
284 | + super(TestSignatureVerificationNoSignature, cls).setUpClass() |
285 | + require_root() |
286 | + |
287 | def test_debsig_verify_no_sig(self): |
288 | name = "org.example.debsig-no-sig" |
289 | path_to_click = self._make_click(name, framework="") |
290 | @@ -145,8 +157,13 @@ |
291 | "--user=%s" % user, name]) |
292 | |
293 | |
294 | -@unittest.skipIf(not is_root(), "This tests needs to run as root") |
295 | class TestSignatureVerification(ClickSignaturesTestCase): |
296 | + |
297 | + @classmethod |
298 | + def setUpClass(cls): |
299 | + super(TestSignatureVerification, cls).setUpClass() |
300 | + require_root() |
301 | + |
302 | def setUp(self): |
303 | super(TestSignatureVerification, self).setUp() |
304 | self.user = os.environ.get("USER", "root") |
305 | @@ -176,7 +193,7 @@ |
306 | [self.click_binary, "list", "--user=%s" % self.user], |
307 | universal_newlines=True) |
308 | self.assertIn(name, output) |
309 | - |
310 | + |
311 | def test_debsig_install_signature_not_in_keyring(self): |
312 | name = "org.example.debsig-no-keyring-sig" |
313 | path_to_click = self._make_click(name, framework="") |
314 | @@ -245,7 +262,7 @@ |
315 | # NOTE: that right now this will not be caught by debsig-verify |
316 | # but later in audit() by debian.debfile.DebFile() |
317 | subprocess.check_call(["ar", |
318 | - "-r", |
319 | + "-r", |
320 | "-b", "data.tar.gz", |
321 | path_to_click, |
322 | new_data]) |
323 | @@ -274,12 +291,12 @@ |
324 | new_data = self.make_nasty_data_tar("bz2") |
325 | # replace data.tar.gz with data.tar.bz2 and ensure this is caught |
326 | subprocess.check_call(["ar", |
327 | - "-d", |
328 | + "-d", |
329 | path_to_click, |
330 | "data.tar.gz", |
331 | ]) |
332 | subprocess.check_call(["ar", |
333 | - "-r", |
334 | + "-r", |
335 | path_to_click, |
336 | new_data]) |
337 | output = subprocess.check_output( |
338 | @@ -290,7 +307,7 @@ |
339 | "control.tar.gz", |
340 | "_gpgorigin", |
341 | "data.tar.bz2", |
342 | - ]) |
343 | + ]) |
344 | with self.assertRaises(subprocess.CalledProcessError) as cm: |
345 | output = subprocess.check_output( |
346 | [self.click_binary, "install", path_to_click], |
347 | @@ -305,7 +322,7 @@ |
348 | # this test is probably not really needed, it tries to trick |
349 | # the system by prepending a valid signature that is not |
350 | # in the keyring. But given that debsig-verify only reads |
351 | - # the first packet of any given _gpg$foo signature its |
352 | + # the first packet of any given _gpg$foo signature it's |
353 | # equivalent to test_debsig_install_signature_not_in_keyring test |
354 | name = "org.example.debsig-replaced-data-prepend-sig-click" |
355 | path_to_click = self._make_click(name, framework="") |
356 | @@ -313,7 +330,7 @@ |
357 | new_data = self.make_nasty_data_tar("gz") |
358 | # replace data.tar.gz |
359 | subprocess.check_call(["ar", |
360 | - "-r", |
361 | + "-r", |
362 | path_to_click, |
363 | new_data, |
364 | ]) |
365 | @@ -345,4 +362,3 @@ |
366 | [self.click_binary, "list", "--user=%s" % self.user], |
367 | universal_newlines=True) |
368 | self.assertNotIn(name, output) |
369 | - |
370 | |
371 | === modified file 'debian/changelog' |
372 | --- debian/changelog 2014-09-02 08:35:59 +0000 |
373 | +++ debian/changelog 2014-09-04 21:07:10 +0000 |
374 | @@ -1,3 +1,13 @@ |
375 | +click (0.4.32) UNRELEASED; urgency=medium |
376 | + |
377 | + * Move integration tests to a location where they won't end up being |
378 | + installed into inappropriate places on the system module path |
379 | + (LP: #1337696). |
380 | + * Use six.with_metaclass for TestBuildCoreApps, so that it doesn't make |
381 | + pyflakes angry when running tests under Python 2. |
382 | + |
383 | + -- Colin Watson <cjwatson@ubuntu.com> Thu, 04 Sep 2014 14:46:16 +0100 |
384 | + |
385 | click (0.4.31.3) utopic; urgency=medium |
386 | |
387 | * Reinstate most of 0.4.31.2; instead, just always pass |
388 | |
389 | === modified file 'debian/control' |
390 | --- debian/control 2014-06-10 17:18:00 +0000 |
391 | +++ debian/control 2014-09-04 21:07:10 +0000 |
392 | @@ -3,7 +3,7 @@ |
393 | Priority: optional |
394 | Maintainer: Colin Watson <cjwatson@ubuntu.com> |
395 | Standards-Version: 3.9.5 |
396 | -Build-Depends: debhelper (>= 9~), dh-autoreconf, intltool, python3:any (>= 3.2), python3-all:any, python3-setuptools, python3-apt, python3-debian, python3-gi, python3:any (>= 3.3) | python3-mock, pep8, python3-pep8, pyflakes, python3-sphinx, pkg-config, valac, gobject-introspection (>= 0.6.7), libgirepository1.0-dev (>= 0.6.7), libglib2.0-dev (>= 2.34), gir1.2-glib-2.0, libjson-glib-dev (>= 0.10), libgee-0.8-dev, libpackagekit-glib2-dev (>= 0.7.2), python3-coverage |
397 | +Build-Depends: debhelper (>= 9~), dh-autoreconf, intltool, python3:any (>= 3.2), python3-all:any, python3-setuptools, python3-apt, python3-debian, python3-gi, python3:any (>= 3.3) | python3-mock, pep8, python3-pep8, pyflakes, python3-sphinx, pkg-config, valac, gobject-introspection (>= 0.6.7), libgirepository1.0-dev (>= 0.6.7), libglib2.0-dev (>= 2.34), gir1.2-glib-2.0, libjson-glib-dev (>= 0.10), libgee-0.8-dev, libpackagekit-glib2-dev (>= 0.7.2), python3-coverage, python3-six |
398 | Vcs-Bzr: https://code.launchpad.net/~ubuntu-managed-branches/click/click |
399 | Vcs-Browser: http://bazaar.launchpad.net/~ubuntu-managed-branches/click/click/files |
400 | X-Auto-Uploader: no-rewrite-version |
401 | |
402 | === modified file 'debian/tests/control' |
403 | --- debian/tests/control 2014-07-14 09:10:39 +0000 |
404 | +++ debian/tests/control 2014-09-04 21:07:10 +0000 |
405 | @@ -1,3 +1,3 @@ |
406 | Tests: run-tests.sh |
407 | -Depends: @, iputils-ping, click-dev, schroot, debootstrap, sudo, bzr, debsigs, debsig-verify |
408 | +Depends: @, iputils-ping, click-dev, schroot, debootstrap, sudo, bzr, debsigs, debsig-verify, python3-six |
409 | Restrictions: needs-root allow-stderr |
410 | |
411 | === modified file 'debian/tests/run-tests.sh' |
412 | --- debian/tests/run-tests.sh 2014-06-11 12:40:44 +0000 |
413 | +++ debian/tests/run-tests.sh 2014-09-04 21:07:10 +0000 |
414 | @@ -2,4 +2,4 @@ |
415 | |
416 | set -e |
417 | |
418 | -python3 -m unittest discover tests.integration |
419 | +TEST_INTEGRATION=1 python3 -m unittest discover -vv click.tests.integration |
420 | |
421 | === removed directory 'tests' |
422 | === removed file 'tests/__init__.py' |
FAILED: Continuous integration, rev:493 jenkins. qa.ubuntu. com/job/ click-devel- ci/27/ jenkins. qa.ubuntu. com/job/ click-devel- utopic- amd64-ci/ 29/console jenkins. qa.ubuntu. com/job/ click-devel- utopic- armhf-ci/ 27/console jenkins. qa.ubuntu. com/job/ click-devel- utopic- i386-ci/ 27/console
http://
Executed test runs:
FAILURE: http://
FAILURE: http://
FAILURE: http://
Click here to trigger a rebuild: s-jenkins. ubuntu- ci:8080/ job/click- devel-ci/ 27/rebuild
http://