Merge ~cjwatson/launchpad:refactor-zope-test-in-subprocess into launchpad:master

Proposed by Colin Watson
Status: Merged
Approved by: Colin Watson
Approved revision: 138cb5524f816c4405403598bdb9c13eca42a1e8
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: ~cjwatson/launchpad:refactor-zope-test-in-subprocess
Merge into: launchpad:master
Diff against target: 370 lines (+88/-77)
4 files modified
lib/lp/bugs/scripts/checkwatches/tests/test_core.py (+9/-6)
lib/lp/services/job/tests/test_runner.py (+4/-3)
lib/lp/testing/__init__.py (+44/-40)
lib/lp/testing/tests/test_run_isolated_test.py (+31/-28)
Reviewer Review Type Date Requested Status
Ioana Lasc Approve
Review via email: mp+387645@code.launchpad.net

Commit message

Refactor ZopeTestInSubProcess as a RunTest variant

Description of the change

In terms of the testtools API, lp.testing.ZopeTestInSubProcess really makes more sense as a version of RunTest (used via `run_tests_with`), so turn it into lp.testing.RunIsolatedTest.

The layer's testSetUp and testTearDown methods are now called in the parent process rather than in the child. Layers are responsible for dealing with their own isolation, and none of Launchpad's layers need subprocess isolation; furthermore, running layer methods in child processes caused problems for some upcoming changes to DatabaseLayer.

To post a comment you must log in.
Revision history for this message
Ioana Lasc (ilasc) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/lib/lp/bugs/scripts/checkwatches/tests/test_core.py b/lib/lp/bugs/scripts/checkwatches/tests/test_core.py
2index ba87699..b66b03e 100644
3--- a/lib/lp/bugs/scripts/checkwatches/tests/test_core.py
4+++ b/lib/lp/bugs/scripts/checkwatches/tests/test_core.py
5@@ -1,4 +1,4 @@
6-# Copyright 2009 Canonical Ltd. This software is licensed under the
7+# Copyright 2009-2020 Canonical Ltd. This software is licensed under the
8 # GNU Affero General Public License version 3 (see the file LICENSE).
9 """Checkwatches unit tests."""
10
11@@ -43,8 +43,9 @@ from lp.registry.interfaces.product import IProductSet
12 from lp.services.config import config
13 from lp.services.log.logger import BufferLogger
14 from lp.testing import (
15+ RunIsolatedTest,
16+ TestCase,
17 TestCaseWithFactory,
18- ZopeTestInSubProcess,
19 )
20 from lp.testing.dbuser import switch_dbuser
21 from lp.testing.layers import LaunchpadZopelessLayer
22@@ -388,8 +389,7 @@ class TestSerialScheduler(TestSchedulerBase, unittest.TestCase):
23 self.scheduler.run()
24
25
26-class TestTwistedThreadScheduler(
27- TestSchedulerBase, ZopeTestInSubProcess, unittest.TestCase):
28+class TestTwistedThreadScheduler(TestSchedulerBase, TestCase):
29 """Test TwistedThreadScheduler.
30
31 By default, updateBugTrackers() runs jobs serially, but a
32@@ -397,7 +397,10 @@ class TestTwistedThreadScheduler(
33 for running several jobs in parallel, is TwistedThreadScheduler.
34 """
35
36+ run_tests_with = RunIsolatedTest
37+
38 def setUp(self):
39+ super(TestTwistedThreadScheduler, self).setUp()
40 self.scheduler = checkwatches.TwistedThreadScheduler(
41 num_threads=5, install_signal_handlers=False)
42
43@@ -451,8 +454,7 @@ class CheckwatchesMasterForThreads(CheckwatchesMaster):
44 return [(ExternalBugTrackerForThreads(self.output_file), bug_watches)]
45
46
47-class TestTwistedThreadSchedulerInPlace(
48- ZopeTestInSubProcess, TestCaseWithFactory):
49+class TestTwistedThreadSchedulerInPlace(TestCaseWithFactory):
50 """Test TwistedThreadScheduler in place.
51
52 As in, driving as much of the bug watch machinery as is possible
53@@ -460,6 +462,7 @@ class TestTwistedThreadSchedulerInPlace(
54 """
55
56 layer = LaunchpadZopelessLayer
57+ run_tests_with = RunIsolatedTest
58
59 def test(self):
60 # Prepare test data.
61diff --git a/lib/lp/services/job/tests/test_runner.py b/lib/lp/services/job/tests/test_runner.py
62index 62a2376..3e2e19d 100644
63--- a/lib/lp/services/job/tests/test_runner.py
64+++ b/lib/lp/services/job/tests/test_runner.py
65@@ -1,4 +1,4 @@
66-# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
67+# Copyright 2009-2020 Canonical Ltd. This software is licensed under the
68 # GNU Affero General Public License version 3 (see the file LICENSE).
69
70 """Tests for job-running facilities."""
71@@ -52,8 +52,8 @@ from lp.services.timeout import (
72 )
73 from lp.services.webapp import errorlog
74 from lp.testing import (
75+ RunIsolatedTest,
76 TestCaseWithFactory,
77- ZopeTestInSubProcess,
78 )
79 from lp.testing.fakemethod import FakeMethod
80 from lp.testing.layers import LaunchpadZopelessLayer
81@@ -616,10 +616,11 @@ class LeaseHeldJob(StaticJobSource):
82 raise LeaseHeld()
83
84
85-class TestTwistedJobRunner(ZopeTestInSubProcess, TestCaseWithFactory):
86+class TestTwistedJobRunner(TestCaseWithFactory):
87
88 # Needs AMQP
89 layer = LaunchpadZopelessLayer
90+ run_tests_with = RunIsolatedTest
91
92 def setUp(self):
93 super(TestTwistedJobRunner, self).setUp()
94diff --git a/lib/lp/testing/__init__.py b/lib/lp/testing/__init__.py
95index 81e86f1..cc14cc8 100644
96--- a/lib/lp/testing/__init__.py
97+++ b/lib/lp/testing/__init__.py
98@@ -1,4 +1,4 @@
99-# Copyright 2009-2019 Canonical Ltd. This software is licensed under the
100+# Copyright 2009-2020 Canonical Ltd. This software is licensed under the
101 # GNU Affero General Public License version 3 (see the file LICENSE).
102
103 from __future__ import absolute_import, print_function
104@@ -40,6 +40,7 @@ __all__ = [
105 'run_script',
106 'run_with_login',
107 'run_with_storm_debug',
108+ 'RunIsolatedTest',
109 'StormStatementRecorder',
110 'test_tales',
111 'TestCase',
112@@ -53,7 +54,6 @@ __all__ = [
113 'with_person_logged_in',
114 'ws_object',
115 'YUIUnitTestCase',
116- 'ZopeTestInSubProcess',
117 ]
118
119 from contextlib import contextmanager
120@@ -125,7 +125,6 @@ from zope.security.proxy import (
121 isinstance as zope_isinstance,
122 removeSecurityProxy,
123 )
124-from zope.testrunner.runner import TestResult as ZopeTestResult
125
126 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
127 from lp.app.interfaces.security import IAuthorization
128@@ -1197,30 +1196,45 @@ def _harvest_yui_test_files(file_path):
129 yield os.path.join(dirpath, filename)
130
131
132-class ZopeTestInSubProcess:
133+class _StartedTestResult(testtools.TestResultDecorator):
134+ """A TestResult for a test that has already been started.
135+
136+ The startTest and stopTest methods do nothing. This is used by
137+ RunIsolatedTest, which calls them in the parent process instead.
138+ """
139+
140+ def startTest(self, test):
141+ pass
142+
143+ def stopTest(self, test):
144+ pass
145+
146+
147+class RunIsolatedTest(testtools.RunTest):
148 """Run tests in a sub-process, respecting Zope idiosyncrasies.
149
150- Use this as a mixin with an interesting `TestCase` to isolate
151- tests with side-effects. Each and every test *method* in the test
152- case is run in a new, forked, sub-process. This will slow down
153- your tests, so use it sparingly. However, when you need to, for
154- example, start the Twisted reactor (which cannot currently be
155- safely stopped and restarted in process) it is invaluable.
156-
157- This is basically a reimplementation of subunit's
158- `IsolatedTestCase` or `IsolatedTestSuite`, but adjusted to work
159- with Zope. In particular, Zope's TestResult object is responsible
160- for calling testSetUp() and testTearDown() on the selected layer.
161+ Set this as `run_tests_with` on an interesting `TestCase` to isolate
162+ tests with side-effects. Each and every test *method* in the test case
163+ is run in a new, forked, sub-process. This will slow down your tests,
164+ so use it sparingly. However, when you need to, for example, start the
165+ Twisted reactor (which cannot currently be safely stopped and restarted
166+ in process) it is invaluable.
167+
168+ This is basically a reimplementation of subunit's `IsolatedTestCase` or
169+ `IsolatedTestSuite`, but adjusted to work with Zope, ensuring that layer
170+ methods (testSetUp and testTearDown) are called in the parent process.
171 """
172
173 def run(self, result):
174- # The result must be an instance of Zope's TestResult because
175- # we construct a super() of it later on. Other result classes
176- # could be supported with a more general approach, but it's
177- # unlikely that any one approach is going to work for every
178- # class. It's better to fail early and draw attention here.
179- assert isinstance(result, ZopeTestResult), (
180- "result must be a Zope result object, not %r." % (result, ))
181+ result.startTest(self.case)
182+ try:
183+ return self._run_started(
184+ _StartedTestResult(
185+ testtools.ExtendedToOriginalDecorator(result)))
186+ finally:
187+ result.stopTest(self.case)
188+
189+ def _run_started(self, result):
190 pread, pwrite = os.pipe()
191 # We flush __stdout__ and __stderr__ at this point in order to avoid
192 # bug 986429; they get copied in full when we fork, which means that
193@@ -1238,13 +1252,10 @@ class ZopeTestInSubProcess:
194 # Child.
195 os.close(pread)
196 fdwrite = os.fdopen(pwrite, 'wb', 1)
197- # Send results to both the Zope result object (so that
198- # layer setup and teardown are done properly, etc.) and to
199- # the subunit stream client so that the parent process can
200- # obtain the result.
201- result = testtools.MultiTestResult(
202- result, subunit.TestProtocolClient(fdwrite))
203- super(ZopeTestInSubProcess, self).run(result)
204+ # Send results to the subunit stream client so that the parent
205+ # process can obtain the result.
206+ super(RunIsolatedTest, self).run(
207+ subunit.TestProtocolClient(fdwrite))
208 fdwrite.flush()
209 # See note above about flushing.
210 sys.__stdout__.flush()
211@@ -1257,17 +1268,10 @@ class ZopeTestInSubProcess:
212 # Parent.
213 os.close(pwrite)
214 fdread = os.fdopen(pread, 'rb')
215- # Skip all the Zope-specific result stuff by using a
216- # super() of the result. This is because the Zope result
217- # object calls testSetUp() and testTearDown() on the
218- # layer, and handles post-mortem debugging. These things
219- # do not make sense in the parent process. More
220- # immediately, it also means that the results are not
221- # reported twice; they are reported on stdout by the child
222- # process, so they need to be suppressed here.
223- result = super(ZopeTestResult, result)
224- # Accept the result from the child process.
225- protocol = subunit.TestProtocolServer(result)
226+ # Accept the result from the child process, but don't write a
227+ # duplicate copy to stdout.
228+ protocol = subunit.TestProtocolServer(
229+ result, stream=subunit.DiscardStream())
230 protocol.readFrom(fdread)
231 fdread.close()
232 os.waitpid(pid, 0)
233diff --git a/lib/lp/testing/tests/test_zope_test_in_subprocess.py b/lib/lp/testing/tests/test_run_isolated_test.py
234similarity index 71%
235rename from lib/lp/testing/tests/test_zope_test_in_subprocess.py
236rename to lib/lp/testing/tests/test_run_isolated_test.py
237index 9f78f82..7071df8 100644
238--- a/lib/lp/testing/tests/test_zope_test_in_subprocess.py
239+++ b/lib/lp/testing/tests/test_run_isolated_test.py
240@@ -1,11 +1,11 @@
241-# Copyright 2010 Canonical Ltd. This software is licensed under the
242+# Copyright 2010-2020 Canonical Ltd. This software is licensed under the
243 # GNU Affero General Public License version 3 (see the file LICENSE).
244
245-"""Test `lp.testing.ZopeTestInSubProcess`.
246+"""Test `lp.testing.RunIsolatedTest`.
247
248 How does it do this?
249
250-A `TestCase`, mixed-in with `ZopeTestInSubProcess`, is run by the Zope
251+A `TestCase`, using `run_tests_with = RunIsolatedTest`, is run by the Zope
252 test runner. This test case sets its own layer, to keep track of the
253 PIDs when certain methods are called. It also records pids for its own
254 methods. Assertions are made as these methods are called to ensure that
255@@ -18,9 +18,11 @@ __metaclass__ = type
256
257 import functools
258 import os
259-import unittest
260
261-from lp.testing import ZopeTestInSubProcess
262+from lp.testing import (
263+ RunIsolatedTest,
264+ TestCase,
265+ )
266
267
268 def record_pid(method):
269@@ -36,13 +38,12 @@ def record_pid(method):
270 return wrapper
271
272
273-class TestZopeTestInSubProcessLayer:
274- """Helper to test `ZopeTestInSubProcess`.
275+class TestRunIsolatedTestLayer:
276+ """Helper to test `RunIsolatedTest`.
277
278 Asserts that layers are set up and torn down in the expected way,
279- namely that setUp() and tearDown() are called in the parent
280- process, and testSetUp() and testTearDown() are called in the
281- child process.
282+ namely that setUp(), testSetUp(), testTearDown(), and tearDown() are
283+ called in the parent process.
284
285 The assertions for tearDown() and testTearDown() must be done here
286 because the test case runs before these methods are called. In the
287@@ -50,7 +51,7 @@ class TestZopeTestInSubProcessLayer:
288 testSetUp() are done here too.
289
290 This layer expects to be *instantiated*, which is not the norm for
291- Zope layers. See `TestZopeTestInSubProcess` for its use.
292+ Zope layers. See `TestRunIsolatedTest` for its use.
293 """
294
295 @record_pid
296@@ -68,15 +69,15 @@ class TestZopeTestInSubProcessLayer:
297
298 @record_pid
299 def testSetUp(self):
300- # Runs in the child process.
301- assert self.pid_in___init__ != self.pid_in_testSetUp, (
302- "layer.testSetUp() called in parent process.")
303+ # Runs in the parent process.
304+ assert self.pid_in___init__ == self.pid_in_testSetUp, (
305+ "layer.testSetUp() not called in parent process.")
306
307 @record_pid
308 def testTearDown(self):
309- # Runs in the child process.
310- assert self.pid_in_testSetUp == self.pid_in_testTearDown, (
311- "layer.testTearDown() not called in same process as testSetUp().")
312+ # Runs in the parent process.
313+ assert self.pid_in___init__ == self.pid_in_testTearDown, (
314+ "layer.testTearDown() not called in parent process.")
315
316 @record_pid
317 def tearDown(self):
318@@ -85,31 +86,33 @@ class TestZopeTestInSubProcessLayer:
319 "layer.tearDown() not called in parent process.")
320
321
322-class TestZopeTestInSubProcess(ZopeTestInSubProcess, unittest.TestCase):
323- """Test `ZopeTestInSubProcess`.
324+class TestRunIsolatedTest(TestCase):
325+ """Test `RunIsolatedTest`.
326
327 Assert that setUp(), test() and tearDown() are called in the child
328 process.
329
330 Sets its own layer attribute. This layer is then responsible for
331 recording the PID at interesting moments. Specifically,
332- layer.testSetUp() must be called in the same process as
333- test.setUp().
334+ test.setUp(), test.test(), and test.tearDown() must all be called in
335+ the same child process.
336 """
337
338+ run_tests_with = RunIsolatedTest
339+
340 @record_pid
341 def __init__(self, method_name='runTest'):
342 # Runs in the parent process.
343- super(TestZopeTestInSubProcess, self).__init__(method_name)
344- self.layer = TestZopeTestInSubProcessLayer()
345+ super(TestRunIsolatedTest, self).__init__(method_name)
346+ self.layer = TestRunIsolatedTestLayer()
347
348 @record_pid
349 def setUp(self):
350 # Runs in the child process.
351- super(TestZopeTestInSubProcess, self).setUp()
352- self.assertEqual(
353- self.layer.pid_in_testSetUp, self.pid_in_setUp,
354- "setUp() not called in same process as layer.testSetUp().")
355+ super(TestRunIsolatedTest, self).setUp()
356+ self.assertNotEqual(
357+ self.layer.pid_in___init__, self.pid_in_setUp,
358+ "setUp() called in parent process.")
359
360 @record_pid
361 def test(self):
362@@ -121,7 +124,7 @@ class TestZopeTestInSubProcess(ZopeTestInSubProcess, unittest.TestCase):
363 @record_pid
364 def tearDown(self):
365 # Runs in the child process.
366- super(TestZopeTestInSubProcess, self).tearDown()
367+ super(TestRunIsolatedTest, self).tearDown()
368 self.assertEqual(
369 self.pid_in_setUp, self.pid_in_tearDown,
370 "tearDown() not run in same process as setUp().")

Subscribers

People subscribed via source and target branches

to status/vote changes: