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

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

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

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

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

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.

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

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

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.

Latest incremental diff, r10440..

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

Just a few changes required to the comments.

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

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