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

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(JobCronScriptSubclass, self).__init__(
> + DummyRunner, test_args=[])

How's this related to the Zope isolation change?

> 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-15 13:26: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,57 @@
> 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.
> +

For the record, vast swathes of code have been written for Twisted that don't
require testing starting the reactor. Generally, starting and stopping the
reactor in application code is a bad pattern.

If we're going to go down that path, then I think having a mixin like this is
a good idea.

> + 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):
> + assert isinstance(result, ZopeTestResult), (
> + "result must be a Zope result object, not %r." % (result,))

Why? Please add a comment explaining the case.

> + 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.
> + os._exit(0)

Personally, I don't need a comment to explain what os._exit(0) does. It's more
interesting to know why we want to exit hard.

> + else:
> + # Parent.
> + os.close(pwrite)
> + fdread = os.fdopen(pread, 'rU')
> + # Accept the result from the child process. Skip all the
> + # Zope-specific result stuff by passing a super() of the
> + # result.

This comment would be more helpful if it explained why we want to skip the
zope-specific result stuff.

> + result = super(ZopeTestResult, result)
> + protocol = subunit.TestProtocolServer(result)
> + protocol.readFrom(fdread)
> + fdread.close()
> + os.waitpid(pid, 0)

Thanks for persevering. Not long to go now :)

jml