Merge lp:~allenap/launchpad/isolate-tests into lp:launchpad

[Gavin Panella]

New class, ZopeTestInSubProcess, a mixin to run tests in forked sub-processes.

Implement a new class - ZopeIsolatedTestCase - that can be used as a mixin with more interesting TestCase subclasses. It runs each test method in a forked sub-process, passing the results back to the parent process with subunit. This is needed to safely run tests that need to, for example, start the Twisted reactor, which cannot be safely restarted in the same process once stopped.

This branch also uses ZopeIsolatedTestCase with a couple of job runner tests. Additionally, one of these tests, TestJobCronScript, has been fixed to override the command line arguments passed to the script class (otherwise sys.argv is used, which is stuffed with arguments passed to bin/test).

[Björn Tillenius (bjornt)]

On Mon, Mar 08, 2010 at 06:14:32PM -0000, Gavin Panella wrote:
> Gavin Panella has proposed merging lp:~allenap/launchpad/isolate-tests into lp:launchpad/devel.
>
> Requested reviews:
> Canonical Launchpad Engineering (launchpad): code
> Related bugs:
> #491870 Use Twisted's thread support instead of the threading module in checkwatches
> https://bugs.launchpad.net/bugs/491870
>
>
> Implement a new class - ZopeIsolatedTestCase - that can be used as a
> mixin with more interesting TestCase subclasses. It runs each test
> method in a forked sub-process, passing the results back to the parent
> process with subunit. This is needed to safely run tests that need to,
> for example, start the Twisted reactor, which cannot be safely restarted
> in the same process once stopped.
>
> This branch also uses ZopeIsolatedTestCase with a couple of job runner
> tests. Additionally, one of these tests, TestJobCronScript, has been
> fixed to override the command line arguments passed to the script class
> (otherwise sys.argv is used, which is stuffed with arguments passed to
> bin/test).

    vote needsinfo

If the job runner tests ran fine previously, why is this
ZopeIsolatedTestCase needed?

> === modified file 'lib/lp/services/job/tests/test_runner.py'
> --- lib/lp/services/job/tests/test_runner.py 2010-02-24 19:52:01 +0000
> +++ lib/lp/services/job/tests/test_runner.py 2010-03-08 18:14:32 +0000

> @@ -300,7 +301,7 @@
> self.entries.append(input)
>
>
> -class TestTwistedJobRunner(TestCaseWithFactory):
> +class TestTwistedJobRunner(ZopeIsolatedTestCase, TestCaseWithFactory):
>
> layer = LaunchpadZopelessLayer
>
> @@ -325,7 +326,7 @@
> self.assertIn('Job ran too long.', oops.value)
>
>
> -class TestJobCronScript(TestCaseWithFactory):
> +class TestJobCronScript(ZopeIsolatedTestCase, TestCaseWithFactory):

Will we ever use ZopeIsolatedTestCase without mixing in
TestCaseWithFactory?

> === modified file 'lib/lp/testing/__init__.py'
> --- lib/lp/testing/__init__.py 2010-02-17 20:54:36 +0000
> +++ lib/lp/testing/__init__.py 2010-03-08 18:14:32 +0000
> @@ -586,6 +591,56 @@
> self.client.open(url=u'http://launchpad.dev:8085')
>
>
> +class ZopeIsolatedTestCase(unittest.TestCase):
> + """Run tests in a sub-process, respecting Zope idiosyncrasies.
> +
> + Use this as a baseclass for test cases, or as a mixin with an
> + interesting `TestCase` subclass.
> +
> + This is basically a reimplementation of subunit's
> + `IsolatedTestCase`, but adjusted to work with Zope. In particular,
> + Zope's TestResult object is responsible for calling testSetUp()
> + and testTearDown() on the selected layer.
> + """

I can see why you named it ZopeIsolatedTestCase, but is that really a
good name? I mean, don't TestCase classes in general run their tests in
an isolated environment?

> +
> + def run(self, result):
> + assert isinstance(result, ZopeTestResult), (
> + "result must be a Zope result object, not %r." % (result,))
> + pread, pwrite = os.pipe()
> + pid = os.fork()
> + if pid == 0:
> + # Child.
> + os.close(pread)
> + ...

review: Needs Information

[Gavin Panella (allenap)]

> On Mon, Mar 08, 2010 at 06:14:32PM -0000, Gavin Panella wrote:
> > Gavin Panella has proposed merging lp:~allenap/launchpad/isolate-tests into
> lp:launchpad/devel.
> >
> > Requested reviews:
> > Canonical Launchpad Engineering (launchpad): code
> > Related bugs:
> > #491870 Use Twisted's thread support instead of the threading module in
> checkwatches
> > https://bugs.launchpad.net/bugs/491870
> >
> >
> > Implement a new class - ZopeIsolatedTestCase - that can be used as a
> > mixin with more interesting TestCase subclasses. It runs each test
> > method in a forked sub-process, passing the results back to the parent
> > process with subunit. This is needed to safely run tests that need to,
> > for example, start the Twisted reactor, which cannot be safely restarted
> > in the same process once stopped.
> >
> > This branch also uses ZopeIsolatedTestCase with a couple of job runner
> > tests. Additionally, one of these tests, TestJobCronScript, has been
> > fixed to override the command line arguments passed to the script class
> > (otherwise sys.argv is used, which is stuffed with arguments passed to
> > bin/test).
>
> vote needsinfo
>
> If the job runner tests ran fine previously, why is this
> ZopeIsolatedTestCase needed?

Jono once told me - and an IRC discussion between Aaron, Jono and
myself yesterday reconfirmed this - that reactor restarting is not
supported, and is considered by Twisted developers to not be a working
feature right now. They want it to work, but it's not high on the
list. Jono suggested subunit.IsolatedTestCase as a possible solution.

Apart from that, I discovered than one of my new tests for
checkwatches ran fine on its own, or when run alongside many other
tests, but hung indefinitely when run in the same process as the job
runner tests.

> > === modified file 'lib/lp/services/job/tests/test_runner.py'
> > --- lib/lp/services/job/tests/test_runner.py 2010-02-24 19:52:01 +0000
> > +++ lib/lp/services/job/tests/test_runner.py 2010-03-08 18:14:32 +0000
>
> > @@ -300,7 +301,7 @@
> > self.entries.append(input)
> >
> >
> > -class TestTwistedJobRunner(TestCaseWithFactory):
> > +class TestTwistedJobRunner(ZopeIsolatedTestCase, TestCaseWithFactory):
> >
> > layer = LaunchpadZopelessLayer
> >
> > @@ -325,7 +326,7 @@
> > self.assertIn('Job ran too long.', oops.value)
> >
> >
> > -class TestJobCronScript(TestCaseWithFactory):
> > +class TestJobCronScript(ZopeIsolatedTestCase, TestCaseWithFactory):
>
> Will we ever use ZopeIsolatedTestCase without mixing in
> TestCaseWithFactory?

Yes, possibly. I've already made some changes to this branch since the
merge proposal went out, one of which was to not inherit from
anything. The reason is that it can be mixed in with TestSuite (and
its subclasses) too, without alteration, to provide process isolation
for a whole suite. I've updated the docs to explain the behaviour.

Full diff of changes since the proposal:
  http://paste.ubuntu.com/391864/

> > === modified file 'lib/lp/testing/__init__.py'
> > --- lib/lp/testing/__init__.py 2010-02-17 20:54:36 +0000
> > +++ lib/lp/testing/__init__.py 2010-03-08 18:14:32 +0000
> > @@ -586,6 ...

[Jonathan Lange (jml)]

2010/3/9 Gavin Panella <email address hidden>:
>> On Mon, Mar 08, 2010 at 06:14:32PM -0000, Gavin Panella wrote:
>> > Gavin Panella has proposed merging lp:~allenap/launchpad/isolate-tests into
>> lp:launchpad/devel.
>> >
>> > Requested reviews:
>> >   Canonical Launchpad Engineering (launchpad): code
>> > Related bugs:
>> >   #491870 Use Twisted's thread support instead of the threading module in
>> checkwatches
>> >   https://bugs.launchpad.net/bugs/491870
>> >
>> >
>> > Implement a new class - ZopeIsolatedTestCase - that can be used as a
>> > mixin with more interesting TestCase subclasses. It runs each test
>> > method in a forked sub-process, passing the results back to the parent
>> > process with subunit. This is needed to safely run tests that need to,
>> > for example, start the Twisted reactor, which cannot be safely restarted
>> >    in the same process once stopped.
>> >
>> > This branch also uses ZopeIsolatedTestCase with a couple of job runner
>> > tests. Additionally, one of these tests, TestJobCronScript, has been
>> > fixed to override the command line arguments passed to the script class
>> > (otherwise sys.argv is used, which is stuffed with arguments passed to
>> > bin/test).
>>
>>     vote needsinfo
>>
>> If the job runner tests ran fine previously, why is this
>> ZopeIsolatedTestCase needed?
>
> Jono once told me - and an IRC discussion between Aaron, Jono and
> myself yesterday reconfirmed this - that reactor restarting is not
> supported, and is considered by Twisted developers to not be a working
> feature right now. They want it to work, but it's not high on the
> list. Jono suggested subunit.IsolatedTestCase as a possible solution.
>

To be clear, this kind of solution (run tests in their own
subprocesses) is hardly ever used within the Twisted world, since it's
considered good practice there for asynchronous APIs to return
Deferreds, and to test things at that level. It's extremely rare to
have application code that needs to start and stop the reactor.

jml

[Björn Tillenius (bjornt)]

On Tue, Mar 09, 2010 at 04:23:44PM -0000, Gavin Panella wrote:
> > > -class TestJobCronScript(TestCaseWithFactory):
> > > +class TestJobCronScript(ZopeIsolatedTestCase, TestCaseWithFactory):
> >
> > Will we ever use ZopeIsolatedTestCase without mixing in
> > TestCaseWithFactory?
>
> Yes, possibly. I've already made some changes to this branch since the
> merge proposal went out, one of which was to not inherit from
> anything. The reason is that it can be mixed in with TestSuite (and
> its subclasses) too, without alteration, to provide process isolation
> for a whole suite. I've updated the docs to explain the behaviour.
>
> Full diff of changes since the proposal:
> http://paste.ubuntu.com/391864/

In the future, could you please attach the changes instead? Makes it
easier to review and comment.

>
> > > === modified file 'lib/lp/testing/__init__.py'
> > > --- lib/lp/testing/__init__.py 2010-02-17 20:54:36 +0000
> > > +++ lib/lp/testing/__init__.py 2010-03-08 18:14:32 +0000
> > > @@ -586,6 +591,56 @@
> > > self.client.open(url=u'http://launchpad.dev:8085')
> > >
> > >
> > > +class ZopeIsolatedTestCase(unittest.TestCase):
> > > + """Run tests in a sub-process, respecting Zope idiosyncrasies.
> > > +
> > > + Use this as a baseclass for test cases, or as a mixin with an
> > > + interesting `TestCase` subclass.
> > > +
> > > + This is basically a reimplementation of subunit's
> > > + `IsolatedTestCase`, but adjusted to work with Zope. In particular,
> > > + Zope's TestResult object is responsible for calling testSetUp()
> > > + and testTearDown() on the selected layer.
> > > + """
> >
> > I can see why you named it ZopeIsolatedTestCase, but is that really a
> > good name? I mean, don't TestCase classes in general run their tests in
> > an isolated environment?
>
> 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.

> > > + def run(self, result):
> > > + assert isinstance(result, ZopeTestResult), (
> > > + "result must be a Zope result object, not %r." % (result,))
> > > + pread, pwrite = os.pipe()
> > > + pid = os.fork()
> > > + if pid == 0:
> > > + # Child.
> > > + os.close(pread)
> > > + fdwrite = os.fdopen(pwrite, 'w', 1)
> > > + # Send results to both the Zope result object (so that
> > > + # layer setup and teardown are done properly, etc.) and to
> > > + # the subunit stream client so that the parent process can
> > > + # obtain the result.
> > > + result = testtools.MultiTestResult(
> > > + result, subunit.TestProtocolClient(fdwrite))
> > > + super(ZopeIsolatedTestCase, self).run(result)
> > > + fdwrite.flush()
> > > + sys.stdout.flush()
> > > + sys.stderr.flush()
> > > + # Exit hard.
> > > + os._exit(0)
> > > + else:
> > > +...

[Gavin Panella (allenap)]

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.

[Gavin Panella (allenap)]

> On Wed, 10 Mar 2010 14:15:31 -0000
> Björn Tillenius <email address hidden> wrote:
>
> ...
> > > 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).

I've removed the suggestion that it can be mixed-in with a TestSuite,
with the following commit message:

  Don't advertise ZopeTestInSubProcess's use with a TestSuite. When
  one suite is added to another with addTests() the *tests* are
  copied, not the suite, so behaviour in a custom TestSuite is
  lost. This is too fragile and likely to cause confusion.

> ...
> > > > 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.

Now that this works, and seems to work well in the situations I'm
using it (I am also using it in a dependent branch for checkwatches),
I'm reluctant to spend more time on figuring out how to do it in a
layer.

Both the job runner code and my checkwatches code start and stop the
reactor as part of their operation, so trying to fudge them to use an
already-started reactor skews the test and means more moving parts.

Forking is cheap as chips so, although I warn in the docstring that it
will slow down tests, I doubt it will make much of a difference,
especially when testing network code, database code, etc. I haven't
measured it though.

...
> > 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.

It looks like no profiling information is collected from the
sub-process. I'm not sure how to fix this. How important is this to
you? Can this branch be landed without that support, with a bug filed
to record that it needs to be added?

Gavin.

[Björn Tillenius (bjornt)]

On Fri, Mar 12, 2010 at 11:27:26AM -0000, Gavin Panella wrote:
> > On Wed, 10 Mar 2010 14:15:31 -0000
> > Björn Tillenius <email address hidden> wrote:
> >
> > ...
> > > > 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).
>
> I've removed the suggestion that it can be mixed-in with a TestSuite,
> with the following commit message:
>
> Don't advertise ZopeTestInSubProcess's use with a TestSuite. When
> one suite is added to another with addTests() the *tests* are
> copied, not the suite, so behaviour in a custom TestSuite is
> lost. This is too fragile and likely to cause confusion.

Ok.

> > > > > 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.
>
> Now that this works, and seems to work well in the situations I'm
> using it (I am also using it in a dependent branch for checkwatches),
> I'm reluctant to spend more time on figuring out how to do it in a
> layer.

Ok.

> > > 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.
>
> It looks like no profiling information is collected from the
> sub-process. I'm not sure how to fix this. How important is this to
> you? Can this branch be landed without that support, with a bug filed
> to record that it needs to be added?

As long as it's not used too much, it's not crucial to have it.

However, I don't see any tests for this. Oh, now I found them, they just
don't look like tests. Hmm. Some more comments would help, to understand
why, and what's being tested. I would have expected a test that manually
calls the run() method. Would that be hard? It would mean that you would
get much better failure messages, in case of something went wrong.
Having tests in setUp and tearDown isn't ideal.

Sorry, but you'll have to ask someone else to carry on reviewing this
branch, since I'm unavailable next week.

    vote abstain

--
Björn Tillenius | https://launchpad.net/~bjornt

review: Abstain

[Gavin Panella (allenap)]

Björn Tillenius wrote:
> > > > 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.
> >
> > It looks like no profiling information is collected from the
> > sub-process. I'm not sure how to fix this. How important is this to
> > you? Can this branch be landed without that support, with a bug filed
> > to record that it needs to be added?
>
> As long as it's not used too much, it's not crucial to have it.
>
> However, I don't see any tests for this. Oh, now I found them, they just
> don't look like tests. Hmm. Some more comments would help, to understand
> why, and what's being tested. I would have expected a test that manually
> calls the run() method. Would that be hard? It would mean that you would
> get much better failure messages, in case of something went wrong.

I've looked into calling the run() method manually, but it means that we
would only be able to test layer.testSetUp(), test.setUp(), the test
method itself, test.tearDown() and layer.testTearDown(). I guess that's
not bad, but it misses out on layer.setUp() and layer.tearDown(), which
are quite important. The way it's structured at the moment means that
everything gets tested, and tested with the real test objects (i.e. the
runner and result) that zope.testing provides.

I have tried to replicate the zope.testing set up - by using Runner from
zope.testing.testresult.runner - but it's fiddly and adds a ton of noise
to the tests and the output. And I haven't yet figured out how to get it
to work properly.

> Having tests in setUp and tearDown isn't ideal.

It's not ideal, but failures here get reported as test failures, rather
than causing a complete test run failure. Unfortunately, failures in
layer methods do cause the run to fail.

To summarise, I'd like to leave the tests as they are. They demonstrate
the operation of ZopeTestInSubProcess right from layer set-up to
tear-down, which I think is valuable. I'll add some more comments to
make it clearer to future developers.

> Sorry, but you'll have to ask someone else to carry on reviewing this
> branch, since I'm unavailable next week.

Thank you for asking so many good questions :) Have a good break.

[Gavin Panella (allenap)]

Latest incremental diff, r10440..

[Jonathan Lange (jml)]

On Mon, Mar 15, 2010 at 1:30 PM, Gavin Panella
<email address hidden> wrote:
> Latest incremental diff, r10440..
>
> === modified file 'lib/lp/services/job/runner.py'
> --- lib/lp/services/job/runner.py 2010-02-25 12:07:15 +0000
> +++ lib/lp/services/job/runner.py 2010-03-15 13:26:40 +0000
> @@ -401,9 +401,10 @@
> class JobCronScript(LaunchpadCronScript):
> """Base class for scripts that run jobs."""
>
> - def __init__(self, runner_class=JobRunner):
> + def __init__(self, runner_class=JobRunner, test_args=None):
> self.dbuser = getattr(config, self.config_name).dbuser
> - super(JobCronScript, self).__init__(self.config_name, self.dbuser)
> + super(JobCronScript, self).__init__(
> + self.config_name, self.dbuser, test_args)
> self.runner_class = runner_class
>
> def main(self):
>

What's the motivation for this change?

> === modified file 'lib/lp/services/job/tests/test_runner.py'
> --- lib/lp/services/job/tests/test_runner.py 2010-02-24 19:52:01 +0000
> +++ lib/lp/services/job/tests/test_runner.py 2010-03-15 13:26:40 +0000
> @@ -11,23 +11,24 @@
> from unittest import TestLoader
>
> import transaction
> -from canonical.testing import LaunchpadZopelessLayer
> +
> from zope.component import getUtility
> from zope.error.interfaces import IErrorReportingUtility
> from zope.interface import implements
>
> +from canonical.launchpad.webapp import errorlog
> +from canonical.launchpad.webapp.interfaces import (
> + DEFAULT_FLAVOR, IStoreSelector, MAIN_STORE)
> +from canonical.testing import LaunchpadZopelessLayer
> +
> from lp.code.interfaces.branchmergeproposal import (
> - IUpdatePreviewDiffJobSource,)
> -from lp.testing.mail_helpers import pop_notifications
> -from lp.services.job.runner import (
> - JobCronScript, JobRunner, BaseRunnableJob, TwistedJobRunner
> -)
> + IUpdatePreviewDiffJobSource)
> from lp.services.job.interfaces.job import JobStatus, IRunnableJob
> from lp.services.job.model.job import Job
> -from lp.testing import TestCaseWithFactory
> -from canonical.launchpad.webapp import errorlog
> -from canonical.launchpad.webapp.interfaces import (
> - IStoreSelector, MAIN_STORE, DEFAULT_FLAVOR)
> +from lp.services.job.runner import (
> + BaseRunnableJob, JobCronScript, JobRunner, TwistedJobRunner)
> +from lp.testing import TestCaseWithFactory, ZopeTestInSubProcess
> +from lp.testing.mail_helpers import pop_notifications
>
>
> class NullJob(BaseRunnableJob):
> @@ -300,7 +301,7 @@
> self.entries.append(input)
>
>
> -class TestTwistedJobRunner(TestCaseWithFactory):
> +class TestTwistedJobRunner(ZopeTestInSubProcess, TestCaseWithFactory):
>
> layer = LaunchpadZopelessLayer
>
> @@ -325,7 +326,7 @@
> self.assertIn('Job ran too long.', oops.value)
>
>
> -class TestJobCronScript(TestCaseWithFactory):
> +class TestJobCronScript(ZopeTestInSubProcess, TestCaseWithFactory):
>
> layer = LaunchpadZopelessLayer
>
> @@ -351,7 +352,8 @@
> source_interface = IUpdatePreviewDiffJobSource
>
> def __init__(self):
> - super(JobCronScriptSubclass, self).__init__(DummyRunner)
> + super(Job...

[Jonathan Lange (jml)]

Just a few changes required to the comments.

review: Needs Fixing

[Gavin Panella (allenap)]

> On Mon, Mar 15, 2010 at 1:30 PM, Gavin Panella
> <email address hidden> wrote:
> > Latest incremental diff, r10440..
> >
> > === modified file 'lib/lp/services/job/runner.py'
> > --- lib/lp/services/job/runner.py 2010-02-25 12:07:15 +0000
> > +++ lib/lp/services/job/runner.py 2010-03-15 13:26:40 +0000
> > @@ -401,9 +401,10 @@
> > class JobCronScript(LaunchpadCronScript):
> > """Base class for scripts that run jobs."""
> >
> > - def __init__(self, runner_class=JobRunner):
> > + def __init__(self, runner_class=JobRunner, test_args=None):
> > self.dbuser = getattr(config, self.config_name).dbuser
> > - super(JobCronScript, self).__init__(self.config_name, self.dbuser)
> > + super(JobCronScript, self).__init__(
> > + self.config_name, self.dbuser, test_args)
> > self.runner_class = runner_class
> >
> > def main(self):
> >
>
> What's the motivation for this change?

It makes testing easier. I'll explain more below.

>
> > === modified file 'lib/lp/services/job/tests/test_runner.py'
> > --- lib/lp/services/job/tests/test_runner.py 2010-02-24 19:52:01 +0000
> > +++ lib/lp/services/job/tests/test_runner.py 2010-03-15 13:26:40 +0000
> > @@ -11,23 +11,24 @@
> > from unittest import TestLoader
> >
> > import transaction
> > -from canonical.testing import LaunchpadZopelessLayer
> > +
> > from zope.component import getUtility
> > from zope.error.interfaces import IErrorReportingUtility
> > from zope.interface import implements
> >
> > +from canonical.launchpad.webapp import errorlog
> > +from canonical.launchpad.webapp.interfaces import (
> > + DEFAULT_FLAVOR, IStoreSelector, MAIN_STORE)
> > +from canonical.testing import LaunchpadZopelessLayer
> > +
> > from lp.code.interfaces.branchmergeproposal import (
> > - IUpdatePreviewDiffJobSource,)
> > -from lp.testing.mail_helpers import pop_notifications
> > -from lp.services.job.runner import (
> > - JobCronScript, JobRunner, BaseRunnableJob, TwistedJobRunner
> > -)
> > + IUpdatePreviewDiffJobSource)
> > from lp.services.job.interfaces.job import JobStatus, IRunnableJob
> > from lp.services.job.model.job import Job
> > -from lp.testing import TestCaseWithFactory
> > -from canonical.launchpad.webapp import errorlog
> > -from canonical.launchpad.webapp.interfaces import (
> > - IStoreSelector, MAIN_STORE, DEFAULT_FLAVOR)
> > +from lp.services.job.runner import (
> > + BaseRunnableJob, JobCronScript, JobRunner, TwistedJobRunner)
> > +from lp.testing import TestCaseWithFactory, ZopeTestInSubProcess
> > +from lp.testing.mail_helpers import pop_notifications
> >
> >
> > class NullJob(BaseRunnableJob):
> > @@ -300,7 +301,7 @@
> > self.entries.append(input)
> >
> >
> > -class TestTwistedJobRunner(TestCaseWithFactory):
> > +class TestTwistedJobRunner(ZopeTestInSubProcess, TestCaseWithFactory):
> >
> > layer = LaunchpadZopelessLayer
> >
> > @@ -325,7 +326,7 @@
> > self.assertIn('Job ran too long.', oops.value)
> >
> >
> > -class TestJobCronScript(TestCaseWithFactory):
> > +class TestJobCronScript(ZopeTestInSubProcess, TestCaseWithFactory):
> >
> > layer = LaunchpadZopelessLayer
> >
> >...

[Jonathan Lange (jml)]

On Tue, Mar 16, 2010 at 4:58 PM, Gavin Panella
<email address hidden> wrote:
>> On Mon, Mar 15, 2010 at 1:30 PM, Gavin Panella
>> <email address hidden> wrote:
>> > Latest incremental diff, r10440..
>> >
>> > === modified file 'lib/lp/services/job/runner.py'
>> > --- lib/lp/services/job/runner.py     2010-02-25 12:07:15 +0000
>> > +++ lib/lp/services/job/runner.py     2010-03-15 13:26:40 +0000
>> > @@ -401,9 +401,10 @@
>> >  class JobCronScript(LaunchpadCronScript):
>> >      """Base class for scripts that run jobs."""
>> >
>> > -    def __init__(self, runner_class=JobRunner):
>> > +    def __init__(self, runner_class=JobRunner, test_args=None):
>> >          self.dbuser = getattr(config, self.config_name).dbuser
>> > -        super(JobCronScript, self).__init__(self.config_name, self.dbuser)
>> > +        super(JobCronScript, self).__init__(
>> > +            self.config_name, self.dbuser, test_args)
>> >          self.runner_class = runner_class
>> >
>> >      def main(self):
>> >
>>
>> What's the motivation for this change?
>
> It makes testing easier. I'll explain more below.
>

>>
>> > === modified file 'lib/lp/services/job/tests/test_runner.py'
>> > --- lib/lp/services/job/tests/test_runner.py  2010-02-24 19:52:01 +0000
>> > +++ lib/lp/services/job/tests/test_runner.py  2010-03-15 13:26:40 +0000
>> > @@ -11,23 +11,24 @@
>> >  from unittest import TestLoader
>> >
>> >  import transaction
>> > -from canonical.testing import LaunchpadZopelessLayer
>> > +
>> >  from zope.component import getUtility
>> >  from zope.error.interfaces import IErrorReportingUtility
>> >  from zope.interface import implements
>> >
>> > +from canonical.launchpad.webapp import errorlog
>> > +from canonical.launchpad.webapp.interfaces import (
>> > +    DEFAULT_FLAVOR, IStoreSelector, MAIN_STORE)
>> > +from canonical.testing import LaunchpadZopelessLayer
>> > +
>> >  from lp.code.interfaces.branchmergeproposal import (
>> > -    IUpdatePreviewDiffJobSource,)
>> > -from lp.testing.mail_helpers import pop_notifications
>> > -from lp.services.job.runner import (
>> > -    JobCronScript, JobRunner, BaseRunnableJob, TwistedJobRunner
>> > -)
>> > +    IUpdatePreviewDiffJobSource)
>> >  from lp.services.job.interfaces.job import JobStatus, IRunnableJob
>> >  from lp.services.job.model.job import Job
>> > -from lp.testing import TestCaseWithFactory
>> > -from canonical.launchpad.webapp import errorlog
>> > -from canonical.launchpad.webapp.interfaces import (
>> > -    IStoreSelector, MAIN_STORE, DEFAULT_FLAVOR)
>> > +from lp.services.job.runner import (
>> > +    BaseRunnableJob, JobCronScript, JobRunner, TwistedJobRunner)
>> > +from lp.testing import TestCaseWithFactory, ZopeTestInSubProcess
>> > +from lp.testing.mail_helpers import pop_notifications
>> >
>> >
>> >  class NullJob(BaseRunnableJob):
>> > @@ -300,7 +301,7 @@
>> >          self.entries.append(input)
>> >
>> >
>> > -class TestTwistedJobRunner(TestCaseWithFactory):
>> > +class TestTwistedJobRunner(ZopeTestInSubProcess, TestCaseWithFactory):
>> >
>> >      layer = LaunchpadZopelessLayer
>> >
>> > @@ -325,7 +326,7 @@
>> >          self.assertIn('Job ran too long.', oops.value)
>> >
>> >
>> > -class...

[Jonathan Lange (jml)]

review: Approve

=== modified file 'lib/lp/services/job/runner.py'
--- lib/lp/services/job/runner.py	2010-02-25 12:07:15 +0000
+++ lib/lp/services/job/runner.py	2010-03-16 18:02:40 +0000
@@ -401,9 +401,10 @@
 class JobCronScript(LaunchpadCronScript):
     """Base class for scripts that run jobs."""
 
-    def __init__(self, runner_class=JobRunner):
+    def __init__(self, runner_class=JobRunner, test_args=None):
         self.dbuser = getattr(config, self.config_name).dbuser
-        super(JobCronScript, self).__init__(self.config_name, self.dbuser)
+        super(JobCronScript, self).__init__(
+            self.config_name, self.dbuser, test_args)
         self.runner_class = runner_class
 
     def main(self):

=== modified file 'lib/lp/services/job/tests/test_runner.py'
--- lib/lp/services/job/tests/test_runner.py	2010-02-24 19:52:01 +0000
+++ lib/lp/services/job/tests/test_runner.py	2010-03-16 18:02:40 +0000
@@ -11,23 +11,24 @@
 from unittest import TestLoader
 
 import transaction
-from canonical.testing import LaunchpadZopelessLayer
+
 from zope.component import getUtility
 from zope.error.interfaces import IErrorReportingUtility
 from zope.interface import implements
 
+from canonical.launchpad.webapp import errorlog
+from canonical.launchpad.webapp.interfaces import (
+    DEFAULT_FLAVOR, IStoreSelector, MAIN_STORE)
+from canonical.testing import LaunchpadZopelessLayer
+
 from lp.code.interfaces.branchmergeproposal import (
-    IUpdatePreviewDiffJobSource,)
-from lp.testing.mail_helpers import pop_notifications
-from lp.services.job.runner import (
-    JobCronScript, JobRunner, BaseRunnableJob, TwistedJobRunner
-)
+    IUpdatePreviewDiffJobSource)
 from lp.services.job.interfaces.job import JobStatus, IRunnableJob
 from lp.services.job.model.job import Job
-from lp.testing import TestCaseWithFactory
-from canonical.launchpad.webapp import errorlog
-from canonical.launchpad.webapp.interfaces import (
-    IStoreSelector, MAIN_STORE, DEFAULT_FLAVOR)
+from lp.services.job.runner import (
+    BaseRunnableJob, JobCronScript, JobRunner, TwistedJobRunner)
+from lp.testing import TestCaseWithFactory, ZopeTestInSubProcess
+from lp.testing.mail_helpers import pop_notifications
 
 
 class NullJob(BaseRunnableJob):
@@ -300,7 +301,7 @@
         self.entries.append(input)
 
 
-class TestTwistedJobRunner(TestCaseWithFactory):
+class TestTwistedJobRunner(ZopeTestInSubProcess, TestCaseWithFactory):
 
     layer = LaunchpadZopelessLayer
 
@@ -325,7 +326,7 @@
         self.assertIn('Job ran too long.', oops.value)
 
 
-class TestJobCronScript(TestCaseWithFactory):
+class TestJobCronScript(ZopeTestInSubProcess, TestCaseWithFactory):
 
     layer = LaunchpadZopelessLayer
 
@@ -351,7 +352,8 @@
             source_interface = IUpdatePreviewDiffJobSource
 
             def __init__(self):
-                super(JobCronScriptSubclass, self).__init__(DummyRunner)
+                super(JobCronScriptSubclass, self).__init__(
+                    DummyRunner, test_args=[])
                 self.logger = ListLogger()
 
         old_errorlog = errorlog.globalErrorUtility

=== modified file 'lib/lp/testing/__init__.py'
--- lib/lp/testing/__init__.py	2010-03-01 03:06:02 +0000
+++ lib/lp/testing/__init__.py	2010-03-16 18:02:40 +0000
@@ -29,6 +29,7 @@
     'validate_mock_class',
     'WindmillTestCase',
     'with_anonymous_login',
+    'ZopeTestInSubProcess',
     ]
 
 import copy
@@ -38,6 +39,8 @@
 from pprint import pformat
 import shutil
 import subprocess
+import subunit
+import sys
 import tempfile
 import time
 
@@ -62,6 +65,7 @@
 from zope.interface.verify import verifyClass, verifyObject
 from zope.security.proxy import (
     isinstance as zope_isinstance, removeSecurityProxy)
+from zope.testing.testrunner.runner import TestResult as ZopeTestResult
 
 from canonical.launchpad.webapp import errorlog
 from canonical.config import config
@@ -586,6 +590,70 @@
         self.client.open(url=u'http://launchpad.dev:8085')
 
 
+class ZopeTestInSubProcess:
+    """Run tests in a sub-process, respecting Zope idiosyncrasies.
+
+    Use this as a mixin with an interesting `TestCase` to isolate
+    tests with side-effects. Each and every test *method* in the test
+    case is run in a new, forked, sub-process. This will slow down
+    your tests, so use it sparingly. However, when you need to, for
+    example, start the Twisted reactor (which cannot currently be
+    safely stopped and restarted in process) it is invaluable.
+
+    This is basically a reimplementation of subunit's
+    `IsolatedTestCase` or `IsolatedTestSuite`, but adjusted to work
+    with Zope. In particular, Zope's TestResult object is responsible
+    for calling testSetUp() and testTearDown() on the selected layer.
+    """
+
+    def run(self, result):
+        # The result must be an instance of Zope's TestResult because
+        # we construct a super() of it later on. Other result classes
+        # could be supported with a more general approach, but it's
+        # unlikely that any one approach is going to work for every
+        # class. It's better to fail early and draw attention here.
+        assert isinstance(result, ZopeTestResult), (
+            "result must be a Zope result object, not %r." % (result,))
+        pread, pwrite = os.pipe()
+        pid = os.fork()
+        if pid == 0:
+            # Child.
+            os.close(pread)
+            fdwrite = os.fdopen(pwrite, 'w', 1)
+            # Send results to both the Zope result object (so that
+            # layer setup and teardown are done properly, etc.) and to
+            # the subunit stream client so that the parent process can
+            # obtain the result.
+            result = testtools.MultiTestResult(
+                result, subunit.TestProtocolClient(fdwrite))
+            super(ZopeTestInSubProcess, self).run(result)
+            fdwrite.flush()
+            sys.stdout.flush()
+            sys.stderr.flush()
+            # Exit hard to avoid running onexit handlers and to avoid
+            # anything that could suppress SystemExit; this exit must
+            # not be prevented.
+            os._exit(0)
+        else:
+            # Parent.
+            os.close(pwrite)
+            fdread = os.fdopen(pread, 'rU')
+            # Skip all the Zope-specific result stuff by using a
+            # super() of the result. This is because the Zope result
+            # object calls testSetUp() and testTearDown() on the
+            # layer, and handles post-mortem debugging. These things
+            # do not make sense in the parent process. More
+            # immediately, it also means that the results are not
+            # reported twice; they are reported on stdout by the child
+            # process, so they need to be suppressed here.
+            result = super(ZopeTestResult, result)
+            # Accept the result from the child process.
+            protocol = subunit.TestProtocolServer(result)
+            protocol.readFrom(fdread)
+            fdread.close()
+            os.waitpid(pid, 0)
+
+
 def capture_events(callable_obj, *args, **kwargs):
     """Capture the events emitted by a callable.
 

=== added file 'lib/lp/testing/tests/test_zope_test_in_subprocess.py'
--- lib/lp/testing/tests/test_zope_test_in_subprocess.py	1970-01-01 00:00:00 +0000
+++ lib/lp/testing/tests/test_zope_test_in_subprocess.py	2010-03-16 18:02:40 +0000
@@ -0,0 +1,131 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test `lp.testing.ZopeTestInSubProcess`.
+
+How does it do this?
+
+A `TestCase`, mixed-in with `ZopeTestInSubProcess`, is run by the Zope
+test runner. This test case sets its own layer, to keep track of the
+PIDs when certain methods are called. It also records pids for its own
+methods. Assertions are made as these methods are called to ensure that
+they are running in the correct process - the parent or the child.
+
+Recording of the PIDs is handled using the `record_pid` decorator.
+"""
+
+__metaclass__ = type
+
+import functools
+import os
+import unittest
+
+from lp.testing import ZopeTestInSubProcess
+
+
+def record_pid(method):
+    """Decorator that records the pid at method invocation.
+
+    Will probably only DTRT with class methods or bound instance
+    methods.
+    """
+    @functools.wraps(method)
+    def wrapper(self, *args, **kwargs):
+        setattr(self, 'pid_in_%s' % method.__name__, os.getpid())
+        return method(self, *args, **kwargs)
+    return wrapper
+
+
+class TestZopeTestInSubProcessLayer:
+    """Helper to test `ZopeTestInSubProcess`.
+
+    Asserts that layers are set up and torn down in the expected way,
+    namely that setUp() and tearDown() are called in the parent
+    process, and testSetUp() and testTearDown() are called in the
+    child process.
+
+    The assertions for tearDown() and testTearDown() must be done here
+    because the test case runs before these methods are called. In the
+    interests of symmetry and clarity, the assertions for setUp() and
+    testSetUp() are done here too.
+
+    This layer expects to be *instantiated*, which is not the norm for
+    Zope layers. See `TestZopeTestInSubProcess` for its use.
+    """
+
+    @record_pid
+    def __init__(self):
+        # These are needed to satisfy the requirements of the
+        # byzantine Zope layer machinery.
+        self.__name__ = self.__class__.__name__
+        self.__bases__ = self.__class__.__bases__
+
+    @record_pid
+    def setUp(self):
+        # Runs in the parent process.
+        assert self.pid_in___init__ == self.pid_in_setUp, (
+            "layer.setUp() not called in parent process.")
+
+    @record_pid
+    def testSetUp(self):
+        # Runs in the child process.
+        assert self.pid_in___init__ != self.pid_in_testSetUp, (
+            "layer.testSetUp() called in parent process.")
+
+    @record_pid
+    def testTearDown(self):
+        # Runs in the child process.
+        assert self.pid_in_testSetUp == self.pid_in_testTearDown, (
+            "layer.testTearDown() not called in same process as testSetUp().")
+
+    @record_pid
+    def tearDown(self):
+        # Runs in the parent process.
+        assert self.pid_in___init__ == self.pid_in_tearDown, (
+            "layer.tearDown() not called in parent process.")
+
+
+class TestZopeTestInSubProcess(ZopeTestInSubProcess, unittest.TestCase):
+    """Test `ZopeTestInSubProcess`.
+
+    Assert that setUp(), test() and tearDown() are called in the child
+    process.
+
+    Sets its own layer attribute. This layer is then responsible for
+    recording the PID at interesting moments. Specifically,
+    layer.testSetUp() must be called in the same process as
+    test.setUp().
+    """
+
+    @record_pid
+    def __init__(self, method_name='runTest'):
+        # Runs in the parent process.
+        super(TestZopeTestInSubProcess, self).__init__(method_name)
+        self.layer = TestZopeTestInSubProcessLayer()
+
+    @record_pid
+    def setUp(self):
+        # Runs in the child process.
+        super(TestZopeTestInSubProcess, self).setUp()
+        self.failUnlessEqual(
+            self.layer.pid_in_testSetUp, self.pid_in_setUp,
+            "setUp() not called in same process as layer.testSetUp().")
+
+    @record_pid
+    def test(self):
+        # Runs in the child process.
+        self.failUnlessEqual(
+            self.pid_in_setUp, self.pid_in_test,
+            "test method not run in same process as setUp().")
+
+    @record_pid
+    def tearDown(self):
+        # Runs in the child process.
+        super(TestZopeTestInSubProcess, self).tearDown()
+        self.failUnlessEqual(
+            self.pid_in_setUp, self.pid_in_tearDown,
+            "tearDown() not run in same process as setUp().")
+
+
+def test_suite():
+    return unittest.TestLoader().loadTestsFromName(__name__)