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?

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
> >
> > @@ -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?

It made this code easier to test. Without it, I can only test it (afaik)
like so:

  bin/test test_runner

I cannot, for example, do:

  bin/test -t 'test_(runner|checkwatches)|externalbugtracker.txt'

because sys.argv contains stuff that LaunchpadScript tries to digest. I
have to override that, and that's what test_args does.

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

In the case of the job runner, it's because Twisted is being shoe-horned
into an API that was designed before Twisted was introduced. In
checkwatches, it's like that because it's a transitionary API, almost a
learning point. Also, our scripts are expected to inherit from
LaunchpadScript (or, at least, that's the legacy we're working with),
not something suitable for twistd.

Is it such a bad pattern to start and stop the reactor? Other than it
doesn't work reliably. If it did work perfectly, what would be the
problem?

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

Okay, cool. I don't think we necessarily need to go down that path for
everything, or that we can't back up again if it's wrong. It's just
useful to be able to shoot something off into a sub-process when it's
badly behaved, or needs a lot of tear-down which a sub-process can do
for us more cheaply.

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

It's because it does super(ZopeTestResult, result) later on. I could
change it to not care, then later do:

    if isinstance(result, ZopeTestResult):
        # Skip all the Zope-specific result stuff
        # by passing a super() of the result.
        result = super(ZopeTestResult, result)

But, for non-Zope test results, this could cause confusion (duplicated
stdout/stderr reporting, for example).

For example, the Zope test result object is responsible for calling
testSetUp() and testTearDown(), and for handling post-mortem
debugging. If another test result object did something similar, it would
make no sense to do it in the parent process.

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

I copied this from subunit. I guess it's to avoid running onexit
handlers, but perhaps there are other reasons. Ah, maybe SystemExit is
caught and just logged as a test failure.

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

Done.

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

Thanks for the review :)