Launchpad itself

Code review comment for lp:~allenap/launchpad/isolate-tests

  1. isolate-tests
  2. Merge into devel
Revision history for this message
Gavin Panella (allenap) wrote :

On Wed, 10 Mar 2010 14:15:31 -0000
Björn Tillenius <email address hidden> wrote:

...
> > ZopeIsolatedTestCase is now known as ZopeIsolatedTest. The original
> > name form came from subunit.IsolatedTestCase, which does a very
> > similar job. I'm happy to rename it though.
>
> I would probably have SubProcess, or Fork, in the name, to make it more
> clear in what way it provides the isolation.

It's now ZopeTestInSubProcess.

...
> > > I'm a bit interested in how this works with layers. Would you run away
> > > screaming if I asked you to add some tests demonstrating this? :)

Have a look at the new unit test I've added.

Basically, layer setUp() and tearDown() get called in the parent,
testSetUp() and testTearDown() in the child. For the test case,
__init__() is called in the parent, setUp(), tearDown() and test
methods are called in the child.

> > > It'd be interesting to see in which process the setUp(), testSetUp(),
> > > tearDown(), and testTearDown() methods of the layer are executed. I'm
> > > wondering if we can see subtle errors due to doing the wrong things in
> > > there.

Yes, there probably will be incompatibilities with some layers :-/

...
> > If it's mixed-in with a TestSuite I think it gets more complicated.

I haven't written tests for this yet. I may not do so (and restrict
ZopeTestInSubProcess to just TestCase for now).

...
> > > Also, how this this TestClass related to TwistedLayer? Oh right, that
> > > leads me to another question. You say that the reactor can't be
> > > restarted easily. But can it be shared between tests, just like the
> > > component architecture?
> >
> > That's an interesting idea. reactor.run() blocks, but I don't know of
> > any reason why it couldn't be started in a thread. I might get sent to
> > hell for it, but at least it's warmer than here.
>
> If it could, than maybe it would be better to use a layer instead, since
> that what we use elsewhere.

I haven't thought about this yet, but I will.

> Some other questions popped into my mind. What happens if you specify
> -D, and a test in the forked process fails?

This works as normal; you get a pdb prompt in the right place in the
child process.

> How well does this play with profiling? Do you still get all profiling
> information if you specify --profile=cProfile?

Not tried this yet. Not actually sure how to test this; it's a very
long time since I did any profiling.

So, I still have some work to do!

Gavin.

review3.diff
1=== modified file 'lib/lp/services/job/tests/test_runner.py'
2--- lib/lp/services/job/tests/test_runner.py 2010-03-09 14:28:11 +0000
3+++ lib/lp/services/job/tests/test_runner.py 2010-03-11 16:36:43 +0000
4@@ -27,7 +27,7 @@
5 from lp.services.job.model.job import Job
6 from lp.services.job.runner import (
7 BaseRunnableJob, JobCronScript, JobRunner, TwistedJobRunner)
8-from lp.testing import TestCaseWithFactory, ZopeIsolatedTest
9+from lp.testing import TestCaseWithFactory, ZopeTestInSubProcess
10 from lp.testing.mail_helpers import pop_notifications
11
12
13@@ -301,7 +301,7 @@
14 self.entries.append(input)
15
16
17-class TestTwistedJobRunner(ZopeIsolatedTest, TestCaseWithFactory):
18+class TestTwistedJobRunner(ZopeTestInSubProcess, TestCaseWithFactory):
19
20 layer = LaunchpadZopelessLayer
21
22@@ -326,7 +326,7 @@
23 self.assertIn('Job ran too long.', oops.value)
24
25
26-class TestJobCronScript(ZopeIsolatedTest, TestCaseWithFactory):
27+class TestJobCronScript(ZopeTestInSubProcess, TestCaseWithFactory):
28
29 layer = LaunchpadZopelessLayer
30
31
32=== modified file 'lib/lp/testing/__init__.py'
33--- lib/lp/testing/__init__.py 2010-03-09 14:28:11 +0000
34+++ lib/lp/testing/__init__.py 2010-03-11 16:36:43 +0000
35@@ -29,7 +29,7 @@
36 'validate_mock_class',
37 'WindmillTestCase',
38 'with_anonymous_login',
39- 'ZopeIsolatedTest',
40+ 'ZopeTestInSubProcess',
41 ]
42
43 import copy
44@@ -590,7 +590,7 @@
45 self.client.open(url=u'http://launchpad.dev:8085')
46
47
48-class ZopeIsolatedTest:
49+class ZopeTestInSubProcess:
50 """Run tests in a sub-process, respecting Zope idiosyncrasies.
51
52 Use this as a mixin with an interesting `TestCase` or `TestSuite`
53@@ -631,7 +631,7 @@
54 # obtain the result.
55 result = testtools.MultiTestResult(
56 result, subunit.TestProtocolClient(fdwrite))
57- super(ZopeIsolatedTest, self).run(result)
58+ super(ZopeTestInSubProcess, self).run(result)
59 fdwrite.flush()
60 sys.stdout.flush()
61 sys.stderr.flush()
62
63=== added file 'lib/lp/testing/tests/test_zope_test_in_subprocess.py'
64--- lib/lp/testing/tests/test_zope_test_in_subprocess.py 1970-01-01 00:00:00 +0000
65+++ lib/lp/testing/tests/test_zope_test_in_subprocess.py 2010-03-11 21:36:45 +0000
66@@ -0,0 +1,118 @@
67+# Copyright 2010 Canonical Ltd. This software is licensed under the
68+# GNU Affero General Public License version 3 (see the file LICENSE).
69+
70+"""Test `lp.testing.ZopeTestInSubProcess`."""
71+
72+__metaclass__ = type
73+
74+import functools
75+import os
76+import unittest
77+
78+from lp.testing import ZopeTestInSubProcess
79+
80+
81+def record_pid(method):
82+ """Decorator that records the pid at method invocation.
83+
84+ Will probably only DTRT with class methods or bound instance
85+ methods.
86+ """
87+ @functools.wraps(method)
88+ def wrapper(self, *args, **kwargs):
89+ setattr(self, 'pid_in_%s' % method.__name__, os.getpid())
90+ return method(self, *args, **kwargs)
91+ return wrapper
92+
93+
94+class TestZopeTestInSubProcessLayer:
95+ """Helper to test `ZopeTestInSubProcess`.
96+
97+ Asserts that layers are set up and torn down in the expected way,
98+ namely that setUp() and tearDown() are called in the parent
99+ process, and testSetUp() and testTearDown() are called in the
100+ child process.
101+
102+ The assertions for tearDown() and testTearDown() must be done here
103+ because the test case runs before these methods are called. In the
104+ interests of symmetry and clarity, the assertions for setUp() and
105+ testSetUp() are done here too.
106+ """
107+
108+ @record_pid
109+ def __init__(self, test):
110+ self.test = test
111+ # These are needed to satisfy the requirements of the
112+ # byzantine Zope layer machinery.
113+ self.__name__ = self.__class__.__name__
114+ self.__bases__ = self.__class__.__bases__
115+
116+ @record_pid
117+ def setUp(self):
118+ # Runs in the parent process.
119+ assert self.pid_in___init__ == self.pid_in_setUp, (
120+ "layer.setUp() not called in parent process.")
121+
122+ @record_pid
123+ def tearDown(self):
124+ # Runs in the parent process.
125+ assert self.pid_in___init__ == self.pid_in_tearDown, (
126+ "layer.tearDown() not called in parent process.")
127+
128+ @record_pid
129+ def testSetUp(self):
130+ # Runs in the child process.
131+ assert self.pid_in___init__ != self.pid_in_testSetUp, (
132+ "layer.testSetUp() called in parent process.")
133+
134+ @record_pid
135+ def testTearDown(self):
136+ # Runs in the child process.
137+ assert self.pid_in_testSetUp == self.pid_in_testTearDown, (
138+ "layer.testTearDown() not called in same process as testSetUp().")
139+
140+
141+class TestZopeTestInSubProcess(ZopeTestInSubProcess, unittest.TestCase):
142+ """Test `ZopeTestInSubProcess`.
143+
144+ Assert that setUp(), test() and tearDown() are called in the child
145+ process.
146+ """
147+
148+ @record_pid
149+ def __init__(self, method_name='runTest'):
150+ # Runs in the parent process.
151+ super(TestZopeTestInSubProcess, self).__init__(method_name)
152+ self.layer = TestZopeTestInSubProcessLayer(self)
153+
154+ @record_pid
155+ def setUp(self):
156+ # Runs in the child process.
157+ super(TestZopeTestInSubProcess, self).setUp()
158+ self.failUnlessEqual(
159+ self.layer.pid_in_testSetUp, self.pid_in_setUp,
160+ "setUp() not called in same process as layer.testSetUp().")
161+
162+ @record_pid
163+ def test(self):
164+ # Runs in the child process.
165+ self.failUnlessEqual(
166+ self.pid_in_setUp, self.pid_in_test,
167+ "test method not run in same process as setUp().")
168+
169+ @record_pid
170+ def tearDown(self):
171+ # Runs in the child process.
172+ super(TestZopeTestInSubProcess, self).tearDown()
173+ self.failUnlessEqual(
174+ self.pid_in_setUp, self.pid_in_tearDown,
175+ "tearDown() not run in same process as setUp().")
176+
177+
178+def test_suite():
179+ suite = unittest.TestSuite()
180+ loader = unittest.TestLoader()
181+ suite.addTests(
182+ loader.loadTestsFromTestCase(
183+ TestZopeTestInSubProcess))
184+ return suite

« Back to merge proposal